[ARTEMIS-3949] Internally synchronize methods in ClientSession implementations

• Type: Improvement
• Status: Closed
• Priority: Major
• Resolution: Information Provided
• Affects Version/s: 2.24.0
• Assignee: Unassigned
• Reporter: Peter Machon
• Created: 22/Aug/22 09:57
• Updated: 23/Aug/22 13:45
• Resolved: 23/Aug/22 12:51

Description

ClientSessionImpl has two internal functions i.e. startCall and endCall. These function count concurrent access and throw in case of concurrent access.
They are used e.g. in ClientProducerImpl#doSend method and in the ClientSessionImpl#acknowledge method.

This forces user code to synchronize the use of the session object. That is a pain for two reasons:

1. From a user perspective it is not even clear, which methods are internally counting concurrent access. E.g. the doSend method does not even belong to the session.
2. The session object is not accessible from the user code at any time. E.g. the ClientMessageImpl internally uses the ClientSession's acknowledge method. From user code it is not necessarily clear which session the ClientMessage belongs to. Thus, it would require user code to e.g. implement their own message store just to be able to synchronize the right session.

Solution:

The ClientSessionImpl and all other internal objects like ClientProducerImpl, ClientMessageImpl, and similar have full access and knowledge about their synchronization needs. I thus suggest to implement synchronization where needed instead of leaving the user alone with this issue, where the solution actually means to reimplement a lot of functionality of the client.

e.g.

startCall();
try {
   sessionContext.sendACK(false, blockOnAcknowledge, consumer, message);
} finally {
   endCall();
}

 
could be replaced with something like

synchronized(this) {
   sessionContext.sendACK(false, blockOnAcknowledge, consumer, message);
}

 

EDIT:

Clicking through the client code, I realized that there actually is synchronization on the send method in ChannelImpl:

   // This must never called by more than one thread concurrently
   private boolean send(final Packet packet, final int reconnectID, final boolean flush, final boolean batch) {
      if (invokeInterceptors(packet, interceptors, connection) != null) {
         return false;
      }

      synchronized (sendLock) {
          ...
      }
    }

Even though, the comment explicitly says not to call this message concurrently, there is a synchronization block enclosing all the logic of the function.

Might the comment be deprecated and the concurrency warning thus too? Do I miss something?

Issue Links

• relates to: ARTEMIS-3954 add some detail to core-client thread model javadoc note