Sandbox incorrectly caches list of interceptors / fix for PermGen leak

[sdudley]

When running multiple invocations of the same script class (hooked up to the same sandbox), I noticed that the sandbox seems to drop out from time to time, even when I have asked for it to be enabled.

It appears that this relates to GroovyInterceptor.threadInterceptorsView ThreadLocal not being maintained consistently with respect to threadInterceptors. The value of threadInterceptorsView is populated the first time it is accessed by the thread, but it is never updated again, even if threadInterceptors itself change. This patch below ensures that the threadInterceptorsView is removed from the ThreadLocal whenever the interceptor is registered or deregistered, which forces the values to be consistent.

Additionally, this also calls threadInterceptors.remove() on unregister() to ensure that all of the ThreadLocals are completely removed from the thread. If you use multiple interceptors then this logic is not quite right (since it will wipe out all interceptors), but at least it prevents PermGen/ThreadLocal leaks when redeploying the app. Perhaps this is worthy of being placed in a separate "cleanup()" function so as not to break the API contract?

--- a/src/main/java/org/kohsuke/groovy/sandbox/GroovyInterceptor.java
+++ b/src/main/java/org/kohsuke/groovy/sandbox/GroovyInterceptor.java
@@ -176,6 +176,7 @@ public abstract class GroovyInterceptor {
      */
     public void register() {
         threadInterceptors.get().add(this);
+        threadInterceptorsView.remove(); // make sure that the view value is reset
     }

     /**
@@ -183,6 +184,8 @@ public abstract class GroovyInterceptor {
      */
     public void unregister() {
         threadInterceptors.get().remove(this);
+        threadInterceptors.remove();
+        threadInterceptorsView.remove();
     }

     private static final ThreadLocal<List<GroovyInterceptor>> threadInterceptors = new ThreadLocal<List<GroovyInterceptor>>() {

[kohsuke]

I don't understand this fix. threadInterceptorsView is just a view, in the sense that it does not maintain a copy, but rather it decorates threadInterceptors by making it read-only. Mutation to threadInterceptors will always show through the view.

I don't see how the code incorrectly caches anything or results in PermGen leak. Closing as INVALID.

[sdudley]

threadInterceptorsView is only ever set once per thread (in its initialValue() method). If you register an interceptor, deregister that interceptor, and then re-register a new (distinct) interceptor, the threadInterceptorsView.get() call will still return a reference to the old interceptor.

Edited to add: I take it back about the threadInterceptorsView consistency issue. The problem with the incorrect caching of values was caused by an earlier (incomplete) fix for the PermGen issue in my local repo, in which I was calling .remove() on one ThreadLocal but not the other. That part was of my own doing and I apologize for wasting your time! I do believe that the PermGen issue below is still valid though (and to which the above patch also applies).

[sdudley]

@kohsuke, the PermGen leak comes from the ThreadLocal not being removed when we are done with the sandbox. This causes the thread (which can be run in a thread pool that never dies) to retain a reference to the classloader of the creating class, which itself contains references to all of its classes, and that stops them from being ever removed. This is only generally an issue in a dynamic loading environment (eg. OSGi or WAR) where you have application components being loaded and unloaded multiple times at runtime and with persistent threads. (One reference: http://stackoverflow.com/questions/17968803/threadlocal-memory-leak)