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: [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
