I introduced it for a reason. As you can see in code first line does checking before condition checking and synchronized block to reduce potential penalty for using synchronized block, while second check is done inside synchronized block to ensure that value from a map is not overwritten by newly created value. In most cases only first line will be executed as value will be not null so rest of a code and synchronized block will not be executed.

This is a workaround for XMPPSession not having method putIfAbsent which was introduced to reduce penalty introduced by synchronized block.

Andrzej Wójcik wrote:

I introduced it for a reason. As you can see in code first line does checking before condition checking and synchronized block to reduce potential penalty for using synchronized block, while second check is done inside synchronized block to ensure that value from a map is not overwritten by newly created value. In most cases only first line will be executed as value will be not null so rest of a code and synchronized block will not be executed.

In theory, the packets for the same user should be processed by the same thread to avoid packet reordering. This is true i theory because "the same user" may mean something different depending on whether we talk about a sender or a receiver.

I wonder if this problem really happened to you or this is just a precaution.

This is a workaround for XMPPSession not having method putIfAbsent which was introduced to reduce penalty introduced by synchronized block.

I think this is a good idea to add such a method. It might be useful in many other places as well.

I've never see this issue in Tigase XMPP Server but it happend to me writing code for other project and using such construction was a good idea.

I know that all packets for processors for user are processed by same thread but method presenceUpdate() in which this code exists is called as a part of XMPPPresenceUpdateProcessorIfc which you assumed that it can be dangerous as it is called from Presence processor thread so in this case it is possible that we are accessing value for same key from same session from two threads at the same point in time - that's why I introduced this construction to reduce potential overhead on Presence processor thread.

I know that all packets for processors for user are processed by same thread but method presenceUpdate() in which this code exists is called as a part of XMPPPresenceUpdateProcessorIfc which you assumed that it can be dangerous as it is called from Presence processor thread so in this case it is possible that we are accessing value for same key from same session from two threads at the same point in time - that's why I introduced this construction to reduce potential overhead on Presence processor thread.

Aaaa, you are right, it might be called from multiple threads. We need the new API you mentioned above then for thread safe processing.

Andrzej, please estimate work effort.

I set estimated time to 6h to create putIfAbsent and refactor this code.

Go ahead with implementation.

I added 4 new functions to XMPPResourceConnection:

public Object computeCommonSessionDataIfAbsent(String key, Function<String,Object> valueFactory);
public Object computeSessionDataIfAbsent(String key, Function<String,Object> valueFactory);
public Object putCommonSessionDataIfAbsent(String key, Object value);
public Object putSessionDataIfAbsent(String key, Object value);

Methods put.....IfAbsent() allow to set in sessionData or in common sessionData passed value if there is no value for passed key.

This is replacement for this weird looking check with synchronization.

However looking at current java.util.Map API I found 2 methods which would be even more useful, so I added similar methods to our API.

This are methods compute...IfAbsent which allow us to set value for key if there is no value but we pass implementation of Function (Java 8 - lambda) which will be used as factory for value only if there is no value for passed key. This way value object will not be created if there is value for a key. This methods return existing value for a key are generated and setted value if there was no value.

I think that this new API - compute...IfAbsent is very useful as it replaced whole checking and synchronization part and should be good in terms of performance (no object is created if not needed).

Sounds and looks very good to me.