[#1733] Fix race condition in async exception handling by combining JMS ExceptionListener and TransportListener notifications#1666
Conversation
jeanouii
force-pushed
the
fix/connection-race-condition-on-exception-and-transport-notification
branch
from
86c4c55 to
e6d8cd8
Compare
jeanouii
changed the title
jeanouii
force-pushed
the
fix/connection-race-condition-on-exception-and-transport-notification
branch
from
e6d8cd8 to
9c1d1cc
Compare
…notifications are not dropped when executor is terminated
jeanouii
force-pushed
the
fix/connection-race-condition-on-exception-and-transport-notification
branch
from
9c1d1cc to
be61f63
Compare
…onnection state before notifying listeners to avoid deadlock
jeanouii
changed the title
|
i haven't had a chance to peak at this yet but i will try and look soon |
|
@cshannon thank you. This one is tricky as well, so needs some more pair of eyes. |
| transportFailed(error); | ||
| ServiceSupport.dispose(ActiveMQConnection.this.transport); | ||
| brokerInfoReceived.countDown(); | ||
| if (!closed.get() && !closing.get()) { |
There was a problem hiding this comment.
onException() doesn't call onAsyncException() anymore.
With the exception listener "inline" here, it bypass the onAsyncException() completely. I'm not sure we have anyone overridden onAsyncException(), but it has an impact.
There was a problem hiding this comment.
This is the purpose of the change.
We can have the 2 listeners executed in their own thread, because we can't guarantee the order and we can't guarantee that the exception listener will be triggered before the transport failed.
The only to guarantee the order is to run the 2 in the same async thread.
There was a problem hiding this comment.
Yes, I got it, I'm just curious about the impact. If nobody overriddes onAsyncException() that's all good to me.
3 participants
I'm not quite sure if it's possible or not, but looks like there is a race condition where the transport failed listener is invoked before the exception listener. The test requires the exception listener to be invoked first so they need to be invoked sequentially withing the same runnable.