[
https://issues.apache.org/jira/browse/ARTEMIS-3949?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Peter Machon updated ARTEMIS-3949:
----------------------------------
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:
# 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?
was:
{{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}
> 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)
