[jira] [Closed] (ARTEMIS-3949) Internally synchronize methods in ClientSession implementations

["Robbie Gemmell (Jira)" <ji...@apache.org>]

     [ https://issues.apache.org/jira/browse/ARTEMIS-3949?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Robbie Gemmell closed ARTEMIS-3949.
-----------------------------------
    Resolution: Information Provided

I have added some more detail to the javadoc via ARTEMIS-3954 around the session+children thread model and the related effect of setting a handle. Closing this one following the previous comments around it being as intended/expected.


> Internally synchronize methods in ClientSession implementations
> ---------------------------------------------------------------
>
>                 Key: ARTEMIS-3949
>                 URL: https://issues.apache.org/jira/browse/ARTEMIS-3949
>             Project: ActiveMQ Artemis
>          Issue Type: Improvement
>    Affects Versions: 2.24.0
>            Reporter: Peter Machon
>            Priority: Major
>
> {{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:
>  # 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.
>  # 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.
> {code:java}
> startCall();
> try {
>    sessionContext.sendACK(false, blockOnAcknowledge, consumer, message);
> } finally {
>    endCall();
> }{code}
>  
> could be replaced with something like
> {code:java}
> synchronized(this) {
>    sessionContext.sendACK(false, blockOnAcknowledge, consumer, message);
> }{code}
>  
> *EDIT:*
> Clicking through the client code, I realized that there actually is synchronization on the send method in {{{}ChannelImpl{}}}:
> {code:java}
>    // 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) {
>           ...
>       }
>     }
> {code}
> 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?



--
This message was sent by Atlassian Jira
(v8.20.10#820010)