Hi Philippe,
Thanks for the patch,
On Oct 22, 2010, at 10:33 AM, Philippe Marschall wrote:
> Hi
>
> I have a ClassLoader leak that looks like it's caused by
> com.sun.jersey.spi.inject.Errors. My application is deployed in Tomcat
> and Jersey 1.4 is deployed with the application in WEB-INF/lib. [1]
> Shows the shortest path to a GC root of a class after undeployment
> (done
> with Eclipse MAT). According to it there's an instance of Errors still
> in a ThreadLocal preventing it, it's class and the WebappClassLoader
> from becoming eligible for garbage collection.
>
> The first thing I tried was replacing the anonymous TheadLocal in
> Errors
> with a normal TheadLocal
> (why is #initalValue() synchronized BTW?) and
It is a silly redundancy, because i dumbly copied the pattern of the
example in the ThreadLocal JavaDoc of SE 5.
http://download.oracle.com/javase/1.5.0/docs/api/java/lang/ThreadLocal.html
> remove()ing it even if there are no messages (see patch). That however
> did not solve the problem.
>
> In the end I wrote a ServletRequestListener that .remove()s
> Errors#errors after each request. That fixed the problem. This leads
> me
> to believe that somehow Errors#errors is not cleaned up after each
> request.
>
It should be cleaned up after servlet initialization. There should be
no need to do it for every request,
> Since my application is fairly simple I was wondering whether other
> people how deploy Jersey in Tomcat or Jetty have seen similar issues?
>
I have not, but from your analysis i suspect it is happening for the
case when no errors are detected. FWIW GlassFish, on undeployment,
will go through all classes loaded by the Web app classloader and set
static fields to null, to avoid this type of leak.
I am not sure why your patch to the Errors.postProcess method does not
work. Perhaps because there were other deployments? Have you tried
from a fresh re-start of Tomcat?
Any access to error state should only be allowed within the call/scope
of the Errors.processWithErrors method. I have strengthened this
constraint, see modified code at the end of the email.
Investigating...
Paul.
private static ThreadLocal<Errors> errors = new
ThreadLocal<Errors>();
public static interface Closure<T> {
public T f();
}
public static <T> T processWithErrors(Closure<T> c) {
Errors e = errors.get();
if (e == null) {
e = new Errors();
errors.set(e);
}
e.preProcess();
RuntimeException caught = null;
try {
return c.f();
} catch (RuntimeException re) {
// If a runtime exception is caught then report errors and
// rethrow
caught = re;
} finally {
e.postProcess(caught == null);
}
throw caught;
}
private static Errors getInstance() {
Errors e = errors.get();
// No error processing in scope
if (e == null) {
throw new IllegalStateException("There is no error
processing in scope");
}
// The following should not be necessary but given the
fragile nature of
// static thread local probably best to add it in case some
internals of
// this class change
if (e.stack == 0) {
errors.remove();
throw new IllegalStateException("There is no error
processing in scope");
}
return e;
}