You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@activemq.apache.org by "asanguinetti (via GitHub)" <gi...@apache.org> on 2023/11/15 00:37:39 UTC
[PR] Prevent InactivityMonitor read check Timer leak when TCP connection fails [activemq]
asanguinetti opened a new pull request, #1119:
URL: https://github.com/apache/activemq/pull/1119
### Context
Whenever the `InactivityMonitor` `TransportFilter` is used in a scenario that would return `false` from `AbstractInactivityMonitor#configuredOk`, then the [read check Timer is created](https://github.com/apache/activemq/blob/dfd35d89f62558881bda1ed4cf7a165502c5c989/activemq-client/src/main/java/org/apache/activemq/transport/InactivityMonitor.java#L50) but the call to [startMonitorThreads](https://github.com/apache/activemq/blob/main/activemq-client/src/main/java/org/apache/activemq/transport/AbstractInactivityMonitor.java#L173) [exits prematurely](https://github.com/apache/activemq/blob/dfd35d89f62558881bda1ed4cf7a165502c5c989/activemq-client/src/main/java/org/apache/activemq/transport/AbstractInactivityMonitor.java#L450), and the `monitorStarted` flag is never set.
### Consequence
The class `AbstractInactivityMonitor` retains a strong static reference to the read check `Timer` created in [startConnectCheckTast](https://github.com/apache/activemq/blob/dfd35d89f62558881bda1ed4cf7a165502c5c989/activemq-client/src/main/java/org/apache/activemq/transport/AbstractInactivityMonitor.java#L422).
This reference cannot be removed even by calling `stop` on the `TransportFilter`, because `stopMonitorThreads` [checks the monitorStarted flag before doing anything](https://github.com/apache/activemq/blob/dfd35d89f62558881bda1ed4cf7a165502c5c989/activemq-client/src/main/java/org/apache/activemq/transport/AbstractInactivityMonitor.java#L487).
Also, [stopConnectCheckTask](https://github.com/apache/activemq/blob/dfd35d89f62558881bda1ed4cf7a165502c5c989/activemq-client/src/main/java/org/apache/activemq/transport/AbstractInactivityMonitor.java#L431) 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).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [AMQ-9392] Prevent InactivityMonitor read check Timer leak when TCP connection fails [activemq]
Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon commented on PR #1119:
URL: https://github.com/apache/activemq/pull/1119#issuecomment-3488459204
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] [AMQ-9392] Prevent InactivityMonitor read check Timer leak when TCP connection fails [activemq]
Posted by "jbonofre (via GitHub)" <gi...@apache.org>.
jbonofre commented on PR #1119:
URL: https://github.com/apache/activemq/pull/1119#issuecomment-3348522322
@mattrpav so far I don't issue with the change. Thoughts ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] [AMQ-9392] Prevent InactivityMonitor read check Timer leak when TCP connection fails [activemq]
Posted by "mattrpav (via GitHub)" <gi...@apache.org>.
mattrpav commented on PR #1119:
URL: https://github.com/apache/activemq/pull/1119#issuecomment-1812699852
JIRA created: [AMQ-9392](https://issues.apache.org/jira/browse/AMQ-9392)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [AMQ-9392] Prevent InactivityMonitor read check Timer leak when TCP connection fails [activemq]
Posted by "asanguinetti (via GitHub)" <gi...@apache.org>.
asanguinetti commented on PR #1119:
URL: https://github.com/apache/activemq/pull/1119#issuecomment-1813057950
> @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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [AMQ-9392] Prevent InactivityMonitor read check Timer leak when TCP connection fails [activemq]
Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon commented on PR #1119:
URL: https://github.com/apache/activemq/pull/1119#issuecomment-3487776325
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] [AMQ-9392] Prevent InactivityMonitor read check Timer leak when TCP connection fails [activemq]
Posted by "jbonofre (via GitHub)" <gi...@apache.org>.
jbonofre commented on PR #1119:
URL: https://github.com/apache/activemq/pull/1119#issuecomment-3345208712
@mattrpav this change makes sense for TCP, and I don't expect any issue on other transports as the method is overrided.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] [AMQ-9392] Prevent InactivityMonitor read check Timer leak when TCP connection fails [activemq]
Posted by "jbonofre (via GitHub)" <gi...@apache.org>.
jbonofre merged PR #1119:
URL: https://github.com/apache/activemq/pull/1119
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] [AMQ-9392] Prevent InactivityMonitor read check Timer leak when TCP connection fails [activemq]
Posted by "mattrpav (via GitHub)" <gi...@apache.org>.
mattrpav commented on PR #1119:
URL: https://github.com/apache/activemq/pull/1119#issuecomment-3343562416
@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 — I’m wondering if a parameterized test across all protocols would be a good idea to catch any bugs or regressions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact
Re: [PR] Prevent InactivityMonitor read check Timer leak when TCP connection fails [activemq]
Posted by "mattrpav (via GitHub)" <gi...@apache.org>.
mattrpav commented on PR #1119:
URL: https://github.com/apache/activemq/pull/1119#issuecomment-1812696819
@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)?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [AMQ-9392] Prevent InactivityMonitor read check Timer leak when TCP connection fails [activemq]
Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon commented on PR #1119:
URL: https://github.com/apache/activemq/pull/1119#issuecomment-3488571504
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](https://github.com/asanguinetti/activemq/blob/e8cf40e24f3c8678ad1655ef2757d6b3d6186cfb/activemq-client/src/main/java/org/apache/activemq/transport/AbstractInactivityMonitor.java#L446) during connection initialization if there are no other connections yet, just to immediately re-create it again [here](https://github.com/asanguinetti/activemq/blob/e8cf40e24f3c8678ad1655ef2757d6b3d6186cfb/activemq-client/src/main/java/org/apache/activemq/transport/AbstractInactivityMonitor.java#L476) because the stopConnectCheckTask() method is [called](https://github.com/asanguinetti/activemq/blob/e8cf40e24f3c8678ad1655ef2757d6b3d6186cfb/activemq-client/src/main/java/org/apache/activemq/transport/InactivityMonitor.java#L57) 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For additional commands, e-mail: gitbox-help@activemq.apache.org
For further information, visit: https://activemq.apache.org/contact