ARTEMIS-5735 avoid race b/w session creation & connection failure

[michaeladada]

• By adding the session to connection failure listener only after it was registered
• And closing the session if the connection was destroyed in the meantime

[clebertsuconic]

do you have a test for this condition?

[michaeladada]

do you have a test for this condition?

Since this is a race condition, there is no easy way to test it completely.
Or do you mean specifically the part of closing of the session in case the connection is already destroyed?

[jbertram]

Static analysis might be sufficient for this.

[tabish121]

LGTM

[gemmellr]

There looks to be a persistent test failure introduced in one of the extra ported OpenWire tests. On 3 full runs org.apache.activemq.ConnectionCleanupTest.testChangeClientID() has failed each time, complaining about a session being closed whilst creating a consumer.

It does look like a strange test, accessing client internals to 'remove' and yet then reuse the same Connection, so I'm not clear whether its something real clients and applications can do and if so whether it needs to be kept working.

[jbertram]

For what it's worth, this fixes the test:

diff --git a/tests/activemq5-unit-tests/src/test/java/org/apache/activemq/ConnectionCleanupTest.java b/tests/activemq5-unit-tests/src/test/java/org/apache/activemq/ConnectionCleanupTest.java
index 2e4a2e05a0..07421e69f4 100644
--- a/tests/activemq5-unit-tests/src/test/java/org/apache/activemq/ConnectionCleanupTest.java
+++ b/tests/activemq5-unit-tests/src/test/java/org/apache/activemq/ConnectionCleanupTest.java
@@ -48,6 +48,7 @@ public class ConnectionCleanupTest extends TestCase {
       }
 
       connection.doCleanup(true);
+      connection.setWatchTopicAdvisories(false);
       connection.setClientID("test");
 
       connection.createSession(false, Session.AUTO_ACKNOWLEDGE);

[jbertram]

I believe I've found the underlying problem. I'll push a separate commit to this branch to fix it and allow review.

[jbertram]

FYI - commit was pushed.

[jbertram]

The full test-suite looks good on this.

[tabish121]

I've run this through CI a few times and the previous failures are gone. One thing I'm not sure of is that the test runs with this branch are running consistently longer but I can't see how this change would have caused that. Probably safe to squash the commits now.

[gemmellr]

It is 30 commits behind main, and Clebert did add a really slow test and tweak it later..could be it has the former change but is missing the later one.

[jbertram]

Can we leave the commits separate since they are from 2 different authors and both are non-trivial (in my opinion)?

[tabish121]

It is 30 commits behind main, and Clebert did add a really slow test and tweak it later..could be it has the former change but is missing the later one.

I figured that was likely the reasons.

[tabish121]

Can we leave the commits separate since they are from 2 different authors and both are non-trivial (in my opinion)?

We could but it means you have a known breaking commit that requires the fix that follows.

[jbertram]

Both are correlated with ARTEMIS-5735 so that should be pretty straight-forward, I think.

[gemmellr]

I would squash during 'merge' personally, its really 1 overall change, would be nicer if it was all in the same commit, and its nice not adding known-broken stuff to main. Can always still see both component commits on the PR later if looking at the change close enough to care.

[jbertram]

Squashed & merged. Thanks for the review!

[jbertram]

@michaeladada, thanks for the contribution. Nice work!

[michaeladada]

@michaeladada, thanks for the contribution. Nice work!

@jbertram , thank you and everyone else involved for taking it from the PR and making the entire process so smooth