Allow shutdown of executors, to clean up resources.

[lapo-luchini]

Proper cleanup of resource, avoid problems e.g. on Tomcat undeploy, which currently generates:

org.apache.catalina.loader.WebappClassLoaderBase.clearReferencesThreads The web application [w2s] appears to have started a thread named [ExpiringMap-Expirer] but has failed to stop it. This is very likely to create a memory leak.

In this way it has to be called manually, but it's more generic. Another approach could be to auto-register a context listener to automatically shutdown, like logback is doing or using a @WebListener annotation.

[coveralls]

Coverage increased (+0.04%) to 68.577% when pulling 6cb40a4752c7391c5a4d308d5737639e26dc1815 on andxor:master into 41cb913a95fc60c730d532c8e3734257e689a25d on jhalterman:master.

[jhalterman]

While this is a straightforward change, it's also potentially dangerous since calling shutdown would effectively kill all existing ExpiringMap instance expirations. I'm interested to get feedback from the community on what they think about this...

The only alternative to having something like shutdown would be to expose the internal thread pools as suggested in #38. Of course they'd still be shared/static.

In terms of the effect of a shutdown, I'd be interested to see if we could recover existing expiringmaps by re-initializing the EXPIRER and LISTENER_SERVICE based on the contents of an existing map.

Anyone care to weigh in on this issue?

[dagnelies]

I need that too, it's a meaningful pull request!

I have exactly the same use case too, killing the threads when undeploying a webapp from a tomcat. ...otherwise you have to bring the whole Tomcat down, which is a bit bothersome.

I guess it would be ideal to provide both: to kill all ExpiringMap instances, or to kill just "yours".

[efge]

Agreed this shutdown is useful given the current status of the code. Otherwise we have to use reflection to get to the EXPIRER and LISTENER_SERVICE to shut them down manually.

Of course this is dangerous because these are global, static instances. But this is necessary with the current code. A better way for application wanting isolation would be to allow the thread pool and executor to be contributable from the builder.

[jhalterman]

How about making the expirer and expiration listener thread pools configurable, then the user can shut them down if they want. I left a few additional comments on that here. This wouldn't necessarily require a shutdown API since users could shutdown the threadpools externally. That would of course require that users either maintain a reference to the threadpools, or ExpiringMap could have APIs to get them.

As for shutting down the shared, static threadpools, I'm not really in favor of that since it would have an affect on all ExpiringMap instances. For the same reason, I'm unsure of having a shutdown() API as it would need to throw an exception if someone called it without supplying their own threadpool - which might be surprising behavior.

Thoughts on this?

[dagnelies]

Any news on this? This shutdown hook is still very welcome!

[lapo-luchini]

Being able to define a "local threadpool" at build time (and thus being able to shutdown only my own and not everyone's) would be fine by me, but when a WAR is undeploying (and ExpiringMap is about to be removed from the classloaders) a global shutdown is still fine.

OTOH I know that logback is able to detect when it is deployed like that and automatically listen for context destruction to automatically cleanup all his resources… that'd be fine by me too, even better than my own "explicit shutdown" as it would be automatic. Also, in that case I guess your objects wouldn't apply since "we're about to be unloaded" is a situation when a global shutdown makes sense

[lapo-luchini]

AFAICT logback does it in ServletContextListener simply by including a META-INF/services/javax.servlet.ServletContainerInitializer file in the JAR.