[asanguinetti]

Context

Whenever the InactivityMonitor TransportFilter is used in a scenario that would return false from AbstractInactivityMonitor#configuredOk, then the read check Timer is created but the call to startMonitorThreads exits prematurely, and the monitorStarted flag is never set.

Consequence

The class AbstractInactivityMonitor retains a strong static reference to the read check Timer created in startConnectCheckTast.
This reference cannot be removed even by calling stop on the TransportFilter, because stopMonitorThreads checks the monitorStarted flag before doing anything.
Also, stopConnectCheckTask does not cancel the Timer nor it removes the strong reference to it.
This reference to the Timer instance also implies a reference to the timer Thread, which will also retain the Thread Context ClassLoader (which may or may not be the same ClassLoader that loaded AbstractInactivityMonitor).

Expectation

If the AbstractInactivityMonitor fires up a Timer, there should be a way to dispose of it when it is no longer needed. It looks like it should happen when calling AbstractInactivityMonitor#stop, but it is not the case for this particular scenario.
Ideally there should be no Timer created if the connection is not properly configured (following the same logic as for the "monitor threads").

About this PR

This PR includes a simple test that allows for reproducing the issue to help maintainers with troubleshooting it.
Additionally there is a very naive solution proposed by someone who is a complete stranger with this library, so it should be examined very carefully (if even at all).

[mattrpav]

@asanguinetti thank you for the contribution, we'll look into it. Have you had a chance to test this change with the other transport (non-TCP/NIO) connectors (like stomp, mqtt.. etc)?

[mattrpav]

JIRA created: AMQ-9392

[asanguinetti]

@asanguinetti thank you for the contribution, we'll look into it. Have you had a chance to test this change with the other transport (non-TCP/NIO) connectors (like stomp, mqtt.. etc)?

@mattrpav Nope, I haven't. Do you believe this wouldn't be covered by your existing tests? If you guide me a little bit I can try to create the tests that may be missing.

[jbonofre]

LGTM, thanks !
I'm doing manual tests with other transports but I don't expect issue.

[mattrpav]

@asanguinetti there is extensive overriding of methods by transports by other protocols.

I haven’t had time to dig into this one yet in detail, so I’m wondering if a parameterized test across all protocols would be a good idea to catch any bugs or regressions.

[jbonofre]

@mattrpav this change makes sense for TCP, and I don't expect any issue on other transports as the method is overrided.

[mattrpav]

Usage of the two methods changed in this PR

stopConnectCheckTask():
activemq-client/AbstractInactivityMonitor.java
activemq-client/InactivityMonitor.java
activemq-stomp/StompInactivityMonitor.java

stopMonitorThreads():
activemq-client/AbstractInactivityMonitor.java

Note:
The StompInactivityMonitor only invokes the stopConnectCheckTask() method, it does not override it.

[jbonofre]

@mattrpav so far I don't issue with the change. Thoughts ?

[mattrpav]

LGTM

[cshannon]

I've been looking over this PR and issue in preparation of the 6.2.0 release and I think we should target for 6.3.0 instead.

There is definitely an issue here with the timer being leaked, but I'm not convinced the fix is quite right. For example, the null checks don't make sense to me where they were added because directly before the checks the same variables are reference without a null check. Another issue is I think the fix could cause it to inadvertently miss shutting down the write task now but not sure. A third issue is it's normal to close the connect task before starting the monitor threads so this patch might destroy the thread pool just to immediately re-create it again if there are no other connections.

Also, I was playing around and testing concurrent connections and I found that while this patch improved things a bit, the read task can still be leak if a connection is closed immediately after creating (before it finishes initialization) because a socket read error can be thrown and that IOException was causing the inactivity monitor logic that does to the close to be skipped (pre-existing thing and unrelated to this patch).

So, i think we should just spend some more time on it as this code is high risk to change because it affects every connection

[cshannon]

Also, I was playing around and testing concurrent connections and I found that while this patch improved things a bit, the read task can still be leak if a connection is closed immediately after creating (before it finishes initialization) because a socket read error can be thrown and that IOException was causing the inactivity monitor logic that does to the close to be skipped (pre-existing thing and unrelated to this patch).

So this part may not be an issue after all, on exception I forgot that the default transport listener will async stop connections so I don't think I waited long enough for the cleanup but need to confirm. I think we need to write some improved tests with whatever the final version of this patch is that will concurrently create some connections with different states (some correct, some hung, some that immediately close and test the condition being fixed here etc). Note that we need a real integration test for this to use a real transport that sets the correct listener for cleanup as the InactivityMonitorTest just uses a custom test listener with the transport only and not a broker so those tests are good but not complete.

[cshannon]

I did some testing and confirmed that things do seem to clean up ok on normal exception with the transport listener.

However, I also confirmed that this patch will indeed destroy the thread pool here during connection initialization if there are no other connections yet, just to immediately re-create it again here because the stopConnectCheckTask() method is called as part of the normal initialization of the connection before the monitor threads are created so that would be nice to avoid if the connection hasn't failed.