PicoContainer

Logging about memory leaks and ThreadLocals when redeploying a picocontainer-using app on Tomcat 6.0

Details

  • Type: Bug Bug
  • Status: Resolved Resolved
  • Priority: Minor Minor
  • Resolution: Fixed
  • Affects Version/s: 1.3
  • Fix Version/s: 3.0
  • Component/s: PicoContainer (Java)
  • Labels:
    None
  • Environment:
    Linux (Fedora Core 12)
  • Number of attachments :
    3

Description

Deploy the attached sample application onto Apache Tomcat v6 (included are a pre-built WAR exported from eclipse, a context file for tomcat, and the eclipse project containing the source code).

Now re-deploy by touching the war file.

Observe the following line in the catalina.out log file:

14-Jul-2010 10:52:02 org.apache.catalina.loader.WebappClassLoader clearThreadLocalMap
SEVERE: A web application created a ThreadLocal with key of type [org.picocontainer.defaults.ConstructorInjectionComponentAdapter$1] (value [org.picocontainer.defaults.ConstructorInjectionComponentAdapter$1@f91da9]) and a value of type [java.lang.Boolean] (value [false]) but failed to remove it when the web application was stopped. To prevent a memory leak, the ThreadLocal has been forcibly removed.

Is there some destroy/dispose/shutdown call on picocontainer that I am omitting, or is this a bug?

The following thread discusses the problem with Tomcat and ThreadLocals and makes a few suggestions for how to avoid it:
http://echo.nextapp.com/site/node/6254

I've marked this bug as minor since leaking Booleans isn't a serious memory leak, so it's really just the log spam that's upsetting.

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Mark J. Sinke added a comment -

This is really an extremely easy one to fix. What needs to happen is that the Boolean.FALSE which is set in the thread local for instantiation or verification guards should be set to null instead. There is no change to the logic of the guard at all.

The patch to AbstractInjector.java (of the 2.11 version) is attached as well as inline below:

{{{
245,249d244
< @Override
< protected Boolean initialValue()

{ < return Boolean.FALSE; < }

<
279c274
< set(Boolean.FALSE);
—
> set(null);
}}}

Show
Mark J. Sinke added a comment - This is really an extremely easy one to fix. What needs to happen is that the Boolean.FALSE which is set in the thread local for instantiation or verification guards should be set to null instead. There is no change to the logic of the guard at all. The patch to AbstractInjector.java (of the 2.11 version) is attached as well as inline below: {{{ 245,249d244 < @Override < protected Boolean initialValue() { < return Boolean.FALSE; < } < 279c274 < set(Boolean.FALSE); — > set(null); }}}
Hide
Permalink
Mark J. Sinke added a comment -

A trivial patch to reduce noise of Tomcat shutting down. Making it possible to find real leaks again

Show
Mark J. Sinke added a comment - A trivial patch to reduce noise of Tomcat shutting down. Making it possible to find real leaks again
Hide
Permalink
Mark J. Sinke added a comment -

A second source of log pollution is the use of the IntoThreadLocal thread-local. However, since that stores a Class object, it may be a real source of memory leaks. My suggestion is to change DefaultPicoContainer.java as follows

{{{
330c330
<
—
>
624,628d623
< synchronized (this) {
< if (intoThreadLocal == null)

{ < intoThreadLocal = new IntoThreadLocal(); < }

< }
630c625,629
< return getComponent(componentKeyOrType, (Class<? extends Annotation>) null);
—
> try

{ > return getComponent(componentKeyOrType, (Class<? extends Annotation>) null); > }

finally

{ > intoThreadLocal.set(null); > }

687,691d685
< synchronized (this) {
< if (intoThreadLocal == null)

{ < intoThreadLocal = new IntoThreadLocal(); < }

< }
}}}

Note that I also removed the lazy initialization of the IntoThreadLocal field, since it is private and initialized on construction (maybe it should be final as well to remove thread-visibility issues).

I added a patch file as well with the same content

Show
Mark J. Sinke added a comment - A second source of log pollution is the use of the IntoThreadLocal thread-local. However, since that stores a Class object, it may be a real source of memory leaks. My suggestion is to change DefaultPicoContainer.java as follows {{{ 330c330 < — > 624,628d623 < synchronized (this) { < if (intoThreadLocal == null) { < intoThreadLocal = new IntoThreadLocal(); < } < } 630c625,629 < return getComponent(componentKeyOrType, (Class<? extends Annotation>) null); — > try { > return getComponent(componentKeyOrType, (Class<? extends Annotation>) null); > } finally { > intoThreadLocal.set(null); > } 687,691d685 < synchronized (this) { < if (intoThreadLocal == null) { < intoThreadLocal = new IntoThreadLocal(); < } < } }}} Note that I also removed the lazy initialization of the IntoThreadLocal field, since it is private and initialized on construction (maybe it should be final as well to remove thread-visibility issues). I added a patch file as well with the same content
Hide
Permalink
Mark J. Sinke added a comment -

The DefaultPicoContainer patch which solves a potential real memory leak.

Show
Mark J. Sinke added a comment - The DefaultPicoContainer patch which solves a potential real memory leak.
Hide
Permalink
Paul Hammant added a comment -

Commit 5735 has applied Mark's patches ... though with some rework.

Show
Paul Hammant added a comment - Commit 5735 has applied Mark's patches ... though with some rework.

People

  • Assignee:
    Paul Hammant
    Reporter:
    David North
Vote (1)
Watch (1)

Dates

  • Created:
    Updated:
    Resolved: