Memory leak in SDKShutdownActivity - JVM shutdown hook

[zippitydooda]

I noticed AI uses a JVM shutdownhook to close various resources/threadpools created by AI. However this won't run until the application server is shutdown and will cause a classloader leak during redeploys. Please remove this shutdownhook when AI is running in an application server environment.

[littleaj]

Thank you for bringing this to our attention. We'll investigate the issue for the next release.

[dhaval24]

@zippitydooda thank you very much for reporting this issue. As a point of sharing knowledge on this particular issue, what could be a possibly better solution to free resources and threadpools created by AI instead of using JVM shutdownhook?

[zippitydooda]

This kind of cleanup could possibly be handled in ServletContextListener.contextDestroyed, or you could provide some shutdown method for the client to call manually.

I've also seen some applications that register a JVM shutdown hook try to detect whether or not the application is running standalone or in a servlet container through checking if certain library classes are present in the classpath. This doesn't seem to be 100% reliable though.

[dhaval24]

@zippitydooda well both ServletContextListener and providing a method would make the user to manage this thread pool which could lead to developers forgetting this and making errors? Am I correct here? Also reading from class path is also tricky thing.

[zippitydooda]

Maybe you could add a setup step so the user could add the listener in their web.xml like how users are adding WebRequestTrackingFilter? I think a publicly accessible shutdown method would allow more flexibility in case they want to do something different.

The classpath idea definitely needs more thought, it is only what I have observed how some applications try to handle the issue.

[littleaj]

@zippitydooda In your case, which Application Insights JARs are you using (core, web, agent)?

[littleaj]

I'm working on a solution for this and I'm considering each combination of JARs to come up with a solution which works for all cases. The cases are:

• web+core
• core only
• agent+core
• agent+web+core.

Also, my preference is that whatever spawns the threads, it also correctly registers them for cleanup; whatever is appropriate for the environment.

The problematic case is core only since it can be used in or out of an application server environment. I'm hoping there is a way to detect Java EE and register a listener programatically. I have found how to register a filter or listener programatically. I am currently exploring solutions for finding the runtime instance of the Java EE objects so I can register a listener for cleaning up the threads.

If the core only case is solved, then it will be solved for all the others; at least, for the threads it creates. If web or agent creates other threads, they'll need a similar change as well. I'll be auditing the code for this soon.

If my experiments comes up empty, we'll likely have to do something like your last suggestion and have to manually register a listener.

[littleaj]

Well, it looks like ServletContext.addListener is available since Servlet 3.0. We currently still support Servlet 2.5 and it doesn't look like we can programatically add a listener in that version. I'm still looking into it, but there may be some extra steps for Servlet 2.5.

[dhaval24]

@littleaj do we have an idea of which frameworks are currently on Servlet 2.5 ? I think the servlet API latest out there is 4.5 so I think we are 2 major versions behind :-)

[littleaj]

@dhaval24 I'm not sure. Tomcat is the only one I've found with a pretty table showing what they support (and tomcat 6 which supported Servlet 2.5 is EOL). I think it's a safe bet to support 3.0+, but we'll have to check on that.

It does say we currently support Servlet 2.5 somewhere.

[zippitydooda]

@littleaj I am using applicationinsights-web on servlet 3.0

[littleaj]

@zippitydooda I made a fix for this using an annotated ServletContextListener which should be picked up automatically by the app server. Let me know if you get a chance to try it out.