Cleanup Threads

[dimalinux]

The current background updater doesn't have a public interface for shutdown.

I created a stand-alone example that shows the issue:
https://github.com/dimalinux/uadetector-example

See the comments at the end of this file:
https://github.com/dimalinux/uadetector-example/blob/master/src/main/java/to/noc/uadetector/example/Main.java

[xyloman]

The need to expose a shutdown is still necessary when running on Tomcat and using parallel deployments. When an application using the UserAgentStringParser returned by UADetectorServiceFactory.getCachingAndUpdatingParser(); results in the following warnings in Tomcat on shutdown:

Jun 18, 2013 10:18:57 AM org.apache.catalina.loader.WebappClassLoader clearReferencesThreads
SEVERE: The web application [/gwx-tocs-rest] appears to have started a thread named [Thread-6] but has failed to stop it. This is very likely to create a memory leak.
Jun 18, 2013 10:18:57 AM org.apache.catalina.loader.WebappClassLoader clearReferencesThreads
SEVERE: The web application [/gwx-tocs-rest] appears to have started a thread named [Thread-7] but has failed to stop it. This is very likely to create a memory leak.
Jun 18, 2013 10:18:57 AM org.apache.catalina.loader.WebappClassLoader clearReferencesThreads

When using your kit you can see that Thread-6 and Thread-7 continue to run even though the application has been undeployed. This also causes the classloader that was used to Startup Thread-6 and Thread-7 to not get garbage collected.

[dimalinux]

The current implementation uses a daemon thread which self terminates for most of us. I assume it could additionally be terminated in a finalize() method of a dependent object. i.e. The thread could be killed during garbage collection. Would that be a better/simpler solution than a shutdown API?

I'm just throwing the idea out. I've never done it, so it could be a bad idea. :)

[xyloman]

When I was profiling this with your kit it appeared that the thread never was garbage collected because the JVM had not shutdown. From my understanding the demon thread will run until the JVM shutdown hook is raised. In the case of Tomcat + web application undeployment the JVM shutdown hook is not raised because the JVM is not shutting down just the JVM. For these cases most objects expose destroy methods that can be called when the Servlet context is destroyed to avoid memory leaks. In our applications this his how we manage cleaning up all of our thread pools through the web application lifecycle.

[xyloman]

If my understanding of finalize methods [1] "The general contract of finalize is that it is invoked if and when the JavaTM virtual machine has determined that there is no longer any means by which this object can be accessed by any thread that has not yet died" this is why the garbage collector would never execute the finalize method because the thread is still active.

[1] http://docs.oracle.com/javase/7/docs/api/java/lang/Object.html#finalize%28%29

[dimalinux]

xyloman, your original request was that a public API be introduced for shutting down the update thread. I proposed that, instead of a public API, UADetector could kill the thread itself during garbage collection. Based on your second-to-last post, it sounds like self-cleanup would be simpler for you. (Since you would just invoke any public API during your own garbage collection.) Am I understanding you correctly?

Andre maintains UADetector and will decide what to do.

[arouel]

@xyloman, @dimalinux At first, many thanks for your feedback. Huu, sounds like a good idea to provide a public API for this, but I need a little feedback from you.

Following suggestion:
We can introduce in our interface UserAgentStringParser a new method shutdown (or destroy, feel free to provide some good names). It would look as follows in a HttpServlet (for example):

public class HelloServlet extends HttpServlet {

    @Override
    protected void doGet(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException {
        // analyze me
        UserAgentStringParser parser = UADetectorServiceFactory.getOnlineUpdatingParser();
        ReadableUserAgent agent = parser.parse(request.getHeader("User-Agent"));
        // do more ...
    }

    @Override
    public void destroy() {
        UADetectorServiceFactory.getOnlineUpdatingParser().shutdown();
    }

}

Is this useful enough for such cases @xyloman?

[xyloman]

I think that is a great start. I assume that this shutdown would call the shutdown on the ExecutorService instance that is associated to the OnlineUpdatingParser?

I also did more digging and wanted to ensure that the shutdown exposed on the parser would also perform a shutdown on the ExecutorService instance associated with implementations of the net.sf.uadetector.datastore.AbstractUpdateOperation class to ensure that that this background thread is also cleaned up. This might require a shutdown method added to the UpdateOperation interface.

[arouel]

@xyloman Exactly, what you've described is how I want to implement it. I'll post changes during the next days and link it with this issue. Maybe you can do a quick review when it's done.

One more question: What version of uadetector-core is running in when your above mentioned log messages occur? In a recent version there should be clear-named thread names (please see 2f0dc0b).

[xyloman]

That sounds good to me. I would have forked and provided my own fixes but time got away from me. I will be able to review these changes when you get them out for review.

I am currently using 0.9.2 of the uadetector-core. I can update to 0.9.4 to determine if the threads have names.

[arouel]

I've implemented two ways to shutdown ExecutorServices created by UADetector. The call on a UserAgentStringParser.shutdown() stops only corresponding executors and schedulers or if the developer wants it very simple the handy call ExecutorServices.shutdownAll() stops everything at once.

[arouel]

Since uadetector-core-0.9.6 and uadetector-resources-2013.06 you can use the provided shutdown operations.