search

RusticiSoftware / TinCanJava

Tin Can Java Library
http://rusticisoftware.github.io/TinCanJava/
Apache License 2.0
44 stars 46 forks source link

Failed to stop HttpClient threads #38

Closed gomezgoiri closed 8 years ago

gomezgoiri commented 9 years ago

Since I started using this library I have experienced some memory leaks in Tomcat 7.

When I redeploy the webapp I see some messages like the following:

SEVERE: The web application [/analyse] appears to have started a thread named [HttpClient-82] but has failed to stop it. This is very likely to create a memory leak.

After checking TinCanJava, I see that you use Jetty to make HTTP requests. There is a suspicious static private attribute in RemoteLRS of the HttpClient class.

Is sharing a unique HttpClient object a common practice for Jetty? Can this be related to the memory leak?

I also see that httpClient.stop() is not called anywhere in RemoteLRS. Would not be a good idea to add a method to allow us (users of TinCanJava) to properly close it before our application shutdowns?

Any other reason that could make Tomcat not be able to stop these threads?

brianjmiller commented 9 years ago

You are probably correct. The PR for gzip compression (#32) started to address this issue by adjusting the thread pool handling, but based on a cursory look still doesn't seem to fully expose it via an API which is probably the best fix. I can't really speak to common practice for Jetty, but given how HttpClient is positioned, particularly with the thread setup, I suspect that yes sharing an instance is common, but as stated we probably need to expose the thread setup to the user. I can't give a timeframe on when I'll be able to get to this but if you want to take a stab and submit a pull request it will at least get the ball rolling.

brianjmiller commented 8 years ago

44 should help contribute a fix for this if I understand the issue correctly. I'd still like to get the thread pool stuff in place, and based on further discussion of gzip I'm expecting the other two PRs to be closed, though I would like to capture the thread stuff from them so I'm currently leaving them open a little longer in case I can get to it in this sprint.

gomezgoiri commented 8 years ago

Hi Brian, thanks for letting me know. The code sent for the issue 44 definitely looks like a solution for this problem. I will use it and test it ASAP.

P.S.: Sorry for not attending you first request, I had it marked as a TODO but I never found the moment.

gomezgoiri commented 8 years ago

The problem described in this issue can be fixed now thanks to the new RemoteLRS.destroy method. For more info see #50.

Many thanks @brianjmiller.