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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected] For further information, visit: https://activemq.apache.org/contact
