Threadlocals and memory leaks in J2EE

classic Classic list List threaded Threaded
50 messages Options
123
Reply | Threaded
Open this post in threaded view
|

Re: Threadlocals and memory leaks in J2EE

Joe Bowbeer
On 10/8/07, David Holmes wrote:
>
> The problem - as per the eval of that bug report referenced - is that people
> use ThreadLocal as a mechanism to implement something that is
> "context"-local (where "context" can be different things).

Which leads to the thread-local guy and the thread-pool guy pointing
their finger at each other...

I like your analysis.

--Joe
_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: Threadlocals and memory leaks in J2EE

Péter Kovács
Very instructive indeed.

But strangely: is the conculsion that the use of ThreadLocal is
out-of-place except when used in Thread pools -- meaning that in the
majority of the cases where ThreadLocal currently is used some kind of
general context abstraction should be used instead? In other words:
ThreadLocal is a useful stand-in in many case, but is not the
conceptually correct solution. (Just like the hammer can be
efficiently used to drive a screw in, but...)

Thanks
P.

On 10/9/07, Joe Bowbeer <[hidden email]> wrote:

> On 10/8/07, David Holmes wrote:
> >
> > The problem - as per the eval of that bug report referenced - is that people
> > use ThreadLocal as a mechanism to implement something that is
> > "context"-local (where "context" can be different things).
>
> Which leads to the thread-local guy and the thread-pool guy pointing
> their finger at each other...
>
> I like your analysis.
>
> --Joe
> _______________________________________________
> Concurrency-interest mailing list
> [hidden email]
> http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest
>
_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: Threadlocals and memory leaks in J2EE

Endre Stølsvik-6
In reply to this post by Joe Bowbeer
Joe Bowbeer wrote:
> On 10/8/07, David Holmes wrote:
>> The problem - as per the eval of that bug report referenced - is that people
>> use ThreadLocal as a mechanism to implement something that is
>> "context"-local (where "context" can be different things).
>
> Which leads to the thread-local guy and the thread-pool guy pointing
> their finger at each other...

I don't really agree with this.

The 1.2 implementation was simple and slow, but didn't have this problem
of retaining wholly unused graphs of objects. The new implementation can
be extended to get this feature back. It would then work very "java
like" - as with the notion of Garbage Collection and all that!

People are using ThreadLocal for interesting and appropriate purposes.
This little flaw isn't very problematic in most cases, except
particularly in the situation where new instances of already loaded
classes are created using a new classloader, and then all references to
the old classes and their instances are ditched - except for the strong
reference that Thread keeps to the values of ThreadLocals in the old
class instances. This results in a leak that no one can fix, since you
cannot ask the Thread to ditch their references.

Why not just implement such a fix if possible? That way at least a whole
bunch of webapp developers would be somewhat happier!

Endre.

PS: Do note that the problem manifests itself not only in the classic
reload scenario: e.g. if the startup procedure of your application use
some stuff that is never used afterwards, and this involved some
ThreadLocal stuff (maybe in some third-party library), then those
objects and their classes can not be unloaded, and your application will
have "lost" that bit of memory. It won't leak in the classical sense, as
it won't build up over time, but it is a complete waste nevertheless. If
you now implement some reload sceheme, you're into the leak territory.
_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: Threadlocals and memory leaks in J2EE

Joe Bowbeer
In reply to this post by Péter Kovács
On 10/9/07, Peter Kovacs wrote:

> Very instructive indeed.
>
> But strangely: is the conculsion that the use of ThreadLocal is
> out-of-place except when used in Thread pools -- meaning that in the
> majority of the cases where ThreadLocal currently is used some kind of
> general context abstraction should be used instead? In other words:
> ThreadLocal is a useful stand-in in many case, but is not the
> conceptually correct solution. (Just like the hammer can be
> efficiently used to drive a screw in, but...)
>
> Thanks
> P.
>

In applications where the threading model is a given, or the
application creates and manages its own threads, then its safe to use
thread-local for several different purposes -- as long as they're
compatible with the threading model.  If there's only one thread per
context (or one parent thread) then a thread-local can even serve as a
per-context global variable.

In applications and shared libraries where the threading model is
somewhat flexible, the uses for thread-local are more limited and,
judging from this discussion, not entirely trouble free (yet?).

As for thread locals and pools, ThreadLocal can be used in combination
with a ThreadPool in order to reduce contention among the worker
threads.  In my view this is an optimization that is a bit fragile,
but often worth the maintentance risk.

--
Joe Bowbeer


> On 10/9/07, Joe Bowbeer wrote:
> > On 10/8/07, David Holmes wrote:
> > >
> > > The problem - as per the eval of that bug report referenced - is that people
> > > use ThreadLocal as a mechanism to implement something that is
> > > "context"-local (where "context" can be different things).
> >
> > Which leads to the thread-local guy and the thread-pool guy pointing
> > their finger at each other...
> >
> > I like your analysis.
> >
> > --Joe
>
_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: Threadlocals and memory leaks in J2EE

David Holmes-3
In reply to this post by Péter Kovács
Peter Kovacs writes:
> But strangely: is the conculsion that the use of ThreadLocal is
> out-of-place except when used in Thread pools

Not at all - almost the opposite: ThreadLocal in non-pool threads a common,
correct usage. Using ThreadLocal in threads that are part of Thread pools
takes great care.

When "thread" and "context" map nicely in terms of identity and lifetime,
then a ThreadLocal can be used for what is more generally a "context-local"
variable. It is when "thread" and "context" don't map nicely that problems
arise.

But taking a step back, Unmesh stated that the problem originally being
reported was that the change in the implementation of ThreadLocal causes the
retention problem in "reloads". I can't quite see how the two
implementations differ in that regard - I was hoping Josh would jump back in
and challenge this comment too, but guess I'll have to do it (in another
email) :)

Cheers,
David Holmes

_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: Threadlocals and memory leaks in J2EE

David Holmes-3
In reply to this post by Endre Stølsvik-6
Endre,

Endre Stolsvik writes:

>     However, when a "web application reload" happens, the CURRENT
> implementation of ThreadLocal will, given merely one non-nulled
> ThreadLocal, and given that Tomcat don't kill _all_ its "worker Threads"
> (the ones that have done service calls into the web app) upon any webapp
> reload, retain a strong chain to the ThreadLocal's values, hence to
> those object's classloader (which is the "now ditched" old webapp
> classloader), and hence all the loaded classes.
> ...
> The key point here, is that the old implementation, a map keyed on
> Thread, didn't have this problem. The problem was introduced with the
> reimplementation for performance improvement that the 1.3 version came
> up with, since now the Thread object itself holds a strong reference to
> the value

I am missing something here. I don't see how the old way (per-Variable:
Thread->value) is different, in terms of retention, to the new way
(per-Thread: variable->value). Can you clarify the problem - thanks.

David Holmes

_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: Threadlocals and memory leaks in J2EE

David Holmes-3
In reply to this post by David Holmes-3
Sorry I meant Endre, not Unmesh.

David

> -----Original Message-----
> From: David Holmes [mailto:[hidden email]]
> Sent: Wednesday, 10 October 2007 8:45 AM
> To: Peter Kovacs
> Cc: concurrency-interest
> Subject: RE: [concurrency-interest] Threadlocals and memory leaks in
> J2EE
>
>
> Peter Kovacs writes:
> > But strangely: is the conculsion that the use of ThreadLocal is
> > out-of-place except when used in Thread pools
>
> Not at all - almost the opposite: ThreadLocal in non-pool threads
> a common, correct usage. Using ThreadLocal in threads that are
> part of Thread pools takes great care.
>
> When "thread" and "context" map nicely in terms of identity and
> lifetime, then a ThreadLocal can be used for what is more
> generally a "context-local" variable. It is when "thread" and
> "context" don't map nicely that problems arise.
>
> But taking a step back, Unmesh stated that the problem originally
> being reported was that the change in the implementation of
> ThreadLocal causes the retention problem in "reloads". I can't
> quite see how the two implementations differ in that regard - I
> was hoping Josh would jump back in and challenge this comment
> too, but guess I'll have to do it (in another email) :)
>
> Cheers,
> David Holmes
>
_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: Threadlocals and memory leaks in J2EE

Bob Lee-8
In reply to this post by David Holmes-3
On 10/9/07, David Holmes <[hidden email]> wrote:
I am missing something here. I don't see how the old way (per-Variable:
Thread->value) is different, in terms of retention, to the new way
(per-Thread: variable->value). Can you clarify the problem - thanks.

David,

With the new model, as you know, Thread keeps a weak reference to the ThreadLocal (key) and a strong reference to the value. If the value inadvertently strongly references the ThreadLocal key, the entry will never get cleaned up. A similar situation could arise in the old model if a value strongly referenced the Thread.

It's very easy to accidentally create a strong reference between your value and ThreadLocal instance. For example, say we have classes Foo and Bar which are both loaded by MyClassLoader:

class Foo {
  static ThreadLocal<Bar> localBar = ...;
}

That code has a memory leak. If you get rid of all references to MyClassLoader, it and all its classes should get GCed, but in this case, we leak a strong reference:

  Thread -> Bar (localBar's value) -> MyClassLoader (Bar.getClass()) -> Foo -> localBar

I also ran into this once when I inadvertently created a circular reference by using an inner class: http://crazybob.org/2006/02/threadlocal-memory-leak.html

Thanks,
Bob

_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: Threadlocals and memory leaks in J2EE

Bob Lee-8
That said, the solution is simply to remove() your thread local values in a finally block.

Bob

On 10/9/07, Bob Lee <[hidden email]> wrote:
On 10/9/07, David Holmes <[hidden email]> wrote:
I am missing something here. I don't see how the old way (per-Variable:
Thread->value) is different, in terms of retention, to the new way
(per-Thread: variable->value). Can you clarify the problem - thanks.

David,

With the new model, as you know, Thread keeps a weak reference to the ThreadLocal (key) and a strong reference to the value. If the value inadvertently strongly references the ThreadLocal key, the entry will never get cleaned up. A similar situation could arise in the old model if a value strongly referenced the Thread.

It's very easy to accidentally create a strong reference between your value and ThreadLocal instance. For example, say we have classes Foo and Bar which are both loaded by MyClassLoader:

class Foo {
  static ThreadLocal<Bar> localBar = ...;
}

That code has a memory leak. If you get rid of all references to MyClassLoader, it and all its classes should get GCed, but in this case, we leak a strong reference:

  Thread -> Bar (localBar's value) -> MyClassLoader (Bar.getClass()) -> Foo -> localBar

I also ran into this once when I inadvertently created a circular reference by using an inner class: <a href="http://crazybob.org/2006/02/threadlocal-memory-leak.html" target="_blank" onclick="return top.js.OpenExtLink(window,event,this)"> http://crazybob.org/2006/02/threadlocal-memory-leak.html

Thanks,
Bob


_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: Threadlocals and memory leaks in J2EE

David Holmes-3
In reply to this post by Bob Lee-8
Thanks for the explanation Bob.
 
That's a nasty cycle: the WeakRef to the key isn't cleared because of the strong-ref to the key via the value; and the value isn't cleared because the WeakRef to the key isn't cleared. I don't suppose we can point the finger at the GC folk and tell them they should be able to spot this? :) More seriously if the relationship/dependency between the key and value could be captured explicitly (say class WeakReferenceWithValue) then the cycle could be broken.
 
Cheers,
David Holmes
-----Original Message-----
From: [hidden email] [mailto:[hidden email]]On Behalf Of Bob Lee
Sent: Wednesday, 10 October 2007 9:11 AM
To: [hidden email]
Cc: Endre Stolsvik; [hidden email]
Subject: Re: [concurrency-interest] Threadlocals and memory leaks in J2EE

On 10/9/07, David Holmes <[hidden email]> wrote:
I am missing something here. I don't see how the old way (per-Variable:
Thread->value) is different, in terms of retention, to the new way
(per-Thread: variable->value). Can you clarify the problem - thanks.

David,

With the new model, as you know, Thread keeps a weak reference to the ThreadLocal (key) and a strong reference to the value. If the value inadvertently strongly references the ThreadLocal key, the entry will never get cleaned up. A similar situation could arise in the old model if a value strongly referenced the Thread.

It's very easy to accidentally create a strong reference between your value and ThreadLocal instance. For example, say we have classes Foo and Bar which are both loaded by MyClassLoader:

class Foo {
  static ThreadLocal<Bar> localBar = ...;
}

That code has a memory leak. If you get rid of all references to MyClassLoader, it and all its classes should get GCed, but in this case, we leak a strong reference:

  Thread -> Bar (localBar's value) -> MyClassLoader (Bar.getClass()) -> Foo -> localBar

I also ran into this once when I inadvertently created a circular reference by using an inner class: http://crazybob.org/2006/02/threadlocal-memory-leak.html

Thanks,
Bob

_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: Threadlocals and memory leaks in J2EE

David Holmes-3
In reply to this post by Bob Lee-8
A finally block of what?
 
David
-----Original Message-----
From: [hidden email] [mailto:[hidden email]]On Behalf Of Bob Lee
Sent: Wednesday, 10 October 2007 9:13 AM
To: [hidden email]
Cc: Endre Stolsvik; [hidden email]
Subject: Re: [concurrency-interest] Threadlocals and memory leaks in J2EE

That said, the solution is simply to remove() your thread local values in a finally block.

Bob

On 10/9/07, Bob Lee <[hidden email]> wrote:
On 10/9/07, David Holmes <[hidden email]> wrote:
I am missing something here. I don't see how the old way (per-Variable:
Thread->value) is different, in terms of retention, to the new way
(per-Thread: variable->value). Can you clarify the problem - thanks.

David,

With the new model, as you know, Thread keeps a weak reference to the ThreadLocal (key) and a strong reference to the value. If the value inadvertently strongly references the ThreadLocal key, the entry will never get cleaned up. A similar situation could arise in the old model if a value strongly referenced the Thread.

It's very easy to accidentally create a strong reference between your value and ThreadLocal instance. For example, say we have classes Foo and Bar which are both loaded by MyClassLoader:

class Foo {
  static ThreadLocal<Bar> localBar = ...;
}

That code has a memory leak. If you get rid of all references to MyClassLoader, it and all its classes should get GCed, but in this case, we leak a strong reference:

  Thread -> Bar (localBar's value) -> MyClassLoader (Bar.getClass()) -> Foo -> localBar

I also ran into this once when I inadvertently created a circular reference by using an inner class: <A onclick="return top.js.OpenExtLink(window,event,this)" href="http://crazybob.org/2006/02/threadlocal-memory-leak.html" target=_blank>http://crazybob.org/2006/02/threadlocal-memory-leak.html

Thanks,
Bob


_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: Threadlocals and memory leaks in J2EE

Bob Lee-8
I just mean that in most use cases, you only need the thread local value set for a specific scope. This code will never leak:

try {
  threadLocal.set(...);
  doSomething();
} finally {
  threadLocal.remove ();
}

Josh's examples #1 and #3 fall into this category. I actually blogged about some related patterns awhile back: http://crazybob.org/2006/07/hard-core-java-threadlocal.html

Josh's example #2 poses a bit of a problem, but I've only ever used this pattern for DateFormat objects, and DateFormat is loaded by the system classloader, so memory leaks aren't an issue.

Bob

On 10/9/07, David Holmes <[hidden email]> wrote:
A finally block of what?
 
David
-----Original Message-----
From: [hidden email] [mailto:[hidden email]]On Behalf Of Bob Lee
Sent: Wednesday, 10 October 2007 9:13 AM
To: [hidden email]
Cc: Endre Stolsvik; [hidden email]
Subject: Re: [concurrency-interest] Threadlocals and memory leaks in J2EE

That said, the solution is simply to remove() your thread local values in a finally block.

Bob

On 10/9/07, Bob Lee <[hidden email]> wrote:
On 10/9/07, David Holmes <[hidden email]> wrote:
I am missing something here. I don't see how the old way (per-Variable:
Thread->value) is different, in terms of retention, to the new way
(per-Thread: variable->value). Can you clarify the problem - thanks.

David,

With the new model, as you know, Thread keeps a weak reference to the ThreadLocal (key) and a strong reference to the value. If the value inadvertently strongly references the ThreadLocal key, the entry will never get cleaned up. A similar situation could arise in the old model if a value strongly referenced the Thread.

It's very easy to accidentally create a strong reference between your value and ThreadLocal instance. For example, say we have classes Foo and Bar which are both loaded by MyClassLoader:

class Foo {
  static ThreadLocal<Bar> localBar = ...;
}

That code has a memory leak. If you get rid of all references to MyClassLoader, it and all its classes should get GCed, but in this case, we leak a strong reference:

  Thread -> Bar (localBar's value) -> MyClassLoader (Bar.getClass()) -> Foo -> localBar

I also ran into this once when I inadvertently created a circular reference by using an inner class: <a href="http://crazybob.org/2006/02/threadlocal-memory-leak.html" target="_blank" onclick="return top.js.OpenExtLink(window,event,this)">http://crazybob.org/2006/02/threadlocal-memory-leak.html

Thanks,
Bob



_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: Threadlocals and memory leaks in J2EE

Endre Stølsvik-6
In reply to this post by Joe Bowbeer
[I sent this answer Tue, 09 Oct 2007 22:46:26 +0200, but for some reason
it hasn't come through.. Trying again..]

Joe Bowbeer wrote:
> On 10/8/07, David Holmes wrote:
>> The problem - as per the eval of that bug report referenced - is that people
>> use ThreadLocal as a mechanism to implement something that is
>> "context"-local (where "context" can be different things).
>
> Which leads to the thread-local guy and the thread-pool guy pointing
> their finger at each other...
>

I don't really agree with this.

The 1.2 implementation was simple and slow, but didn't have this problem
of retaining wholly unused graphs of objects. The new implementation can
be extended to get this feature back. It would then work very "java
like" - as with the notion of Garbage Collection and all that!

People are using ThreadLocal for interesting and appropriate purposes.
This little flaw isn't very problematic in most cases, except
particularly in the situation where new instances of already loaded
classes are created using a new classloader, and then all references to
the old classes and their instances are ditched - except for the strong
reference that Thread keeps to the values of ThreadLocals in the old
class instances. This results in a leak that no one can fix, since you
cannot ask the Thread to ditch their references.

Why not just implement such a fix if possible? That way at least a whole
bunch of webapp developers would be somewhat happier!

Endre.

PS: Do note that the problem manifests itself not only in the classic
reload scenario: e.g. if the startup procedure of your application use
some stuff that is never used afterwards, and this involved some
ThreadLocal stuff (maybe in some third-party library), then those
objects and their classes can not be unloaded, and your application will
have "lost" that bit of memory. It won't leak in the classical sense, as
it won't build up over time, but it is a complete waste nevertheless. If
you now implement some reload sceheme, you're into the leak territory.

_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: Threadlocals and memory leaks in J2EE

Jeremy Manson-2
In reply to this post by Unmesh joshi
Unmesh joshi wrote:

> Hi,
>  >>People are using ThreadLocal for things that are not thread-local, in the
>  >>sense of how ThreadLocal was designed. Hence the problems. Changing
>  >>ThreadLocal might help alleviate some of the problems, but to me that's
>  >.somewhat missing the real problem. The applications that reuse threads in
>  >>different contexts should be the ones ensuring the threads are correctly
>  >>"configured", with respect to ThreadLocal, when they switch
> "contexts". For
>  >>the future we should look at how a ContextLocal variable might be
> defined.
>  >>Just my 2c.
>  >>
>  >>Cheers,
>  >>David Holmes
> I was reading a blog by Robert Martin at
> http://blog.objectmentor.com/articles/2007/09/04/thread-local-a-convenient-abomination.
> Here he states Jim Coplien saying /“An object is an abstraction of
> function. A thread is an abstraction of schedule.“An object is an
> abstraction of function. A thread is an abstraction of schedule." and
> how Unit of Work related variables are often wrongly assigned to
> ThreadLocal. In coming J2EE concurrency utilities, is there any work
> going on ContextLocal or UnitOfWorkLocal utilities?/

This is worth some work.  The Context was what we had in mind when we
added final-field-safe contexts to the JMM.  My next words were, "We
should let people have ThreadLocals that are local to the
final-field-safe contexts."  We never did anything about it, though.

                                        Jeremy
_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: Threadlocals and memory leaks in J2EE

Endre Stølsvik-6
In reply to this post by David Holmes-3
David Holmes wrote:
> Thanks for the explanation Bob.
>  
> That's a nasty cycle: the WeakRef to the key isn't cleared because of
> the strong-ref to the key via the value; and the value isn't cleared
> because the WeakRef to the key isn't cleared. I don't suppose we can
> point the finger at the GC folk and tell them they should be able to
> spot this? :) More seriously if the relationship/dependency between the
> key and value could be captured explicitly (say class
> WeakReferenceWithValue) then the cycle could be broken.

Lots of mails, but do you read the background info at all?

The links I sent in my _initial_ reply to this thread contains all the
information you need for this understanding.

Kind regards,
Endre.
_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: Threadlocals and memory leaks in J2EE

David Holmes-3
Endre Stolsvik writes:
> The links I sent in my _initial_ reply to this thread contains all the
> information you need for this understanding.

They do indeed - sorry.

David Holmes
_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: Threadlocals and memory leaks in J2EE

Joshua Bloch-2
In reply to this post by Endre Stølsvik-6
Endre,

On 10/9/07, Endre Stølsvik <[hidden email]> wrote:
The 1.2 implementation was simple and slow, but didn't have this problem
of retaining wholly unused graphs of objects.
 
This is wrong.  The 1.2 implementation (which I wrote) was simple, slow, massively oversynchronized, and did have the potential for retaining wholly unused graphs of objects.  In this implementation, each ThreadLocal had a WeakHashMap from Thread to value.  As Bob Lee said previously  "[unintentional object retention] could arise if a value strongly referenced the Thread."

It seems the problem you describe is primarily a case of thread pool abuse by Tomcat.  As I'm fond of saying, a thread pool is like a used car lot.  If you want a new car/thread, you have to pay for it. If you buy a used car/thread, it may not be pristine. Apparently Tomcat screwed up in this regard. (I'm no Tomcat expert; I'm basing this judgment on the information I was able to glean from reading this thread.)

I do think the notion of generalized context-local variables, with explicit support for context termination, is reasonable.  People have been talking about this for some time, and now may be the time to consider it seriously.  While I agree that it would be nice if the system could GC thread-local values in the case you describe (value strongly reference ThreadLocal), it is not worth substantially slowing down the facility to rectify this.  Initially ThreadLocal was written to support a small number of thread-local variables, consistent with common practice in the late '90s ( e.g., in Pthreads).  But Java programmers made very heavy (perhaps too heavy) use of ThreadLocal in the intervening years, so we sped it up repeatedly. We cannot afford to slow it down. Users do not take kindly to severe performance regressions.
 
            Josh

 

_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: Threadlocals and memory leaks in J2EE

Endre Stølsvik-6
Joshua Bloch wrote:

> Endre,
>
> On 10/9/07, *Endre Stølsvik* <[hidden email]
> <mailto:[hidden email]>> wrote:
>
>     The 1.2 implementation was simple and slow, but didn't have this problem
>     of retaining wholly unused graphs of objects.
>
>  
> This is wrong.  The 1.2 implementation (which I wrote) was simple, slow,
> massively oversynchronized, and /did/ have the potential for retaining
> wholly unused graphs of objects.  In this implementation, each
> ThreadLocal had a WeakHashMap from Thread to value.  As Bob Lee said
> previously  "[unintentional object retention] could arise if a value
> strongly referenced the Thread."

Okay, okay, but this obviously doesn't make nearly the same problems for
folks as the current issue does.

>
> It seems the problem you describe is primarily a case of thread pool
> abuse by Tomcat.  As I'm fond of saying, a thread pool is like a used
> car lot.  If you want a new car/thread, you have to pay for it. If you
> buy a used car/thread, it may not be pristine. Apparently Tomcat screwed
> up in this regard. (I'm no Tomcat expert; I'm basing this judgment on
> the information I was able to glean from reading this thread.)

This is not Tomcat's problem per se. It isn't Tomcat that isn't cleaning
up after itself. It is the webapp's code. However, as I've tried
repeatedly to point out (obviously without getting through at all), the
problem is that the user code (the webapp in question) might not be in a
position to fix this.

And also, what you and others actually state is that to use ThreadLocals
in your own code, you must handle them just as preciously as one does
e.g. SQL Connections, with try-finally:close and all that. This seems
extremely counter intuitive for "a simple ThreadLocal variable". And the
particular issue at hand _can_ be fixed.

Google "java memory leak", and count the number of times people have
ended up with variations of this extremely diffuse and very hard to
track down bug.. And these are only the people that have lived through
the experience to tell about it!
   Simply given that Servlets are a very common usage of Java, and that
Tomcat is a very common Servlet Container, one can deduce that actually
rather many developers are affected by this problem to some degree,
merely through Tomcat.

Why not look into it?!

(Of course, as mentioned, Tomcat _could_ easily fix the particular
problem by making a new Thread pool, ditching all the old threads. But
the bug affects all such scenarios, not just Tomcat)

>
> While I agree that it would be nice if the
> system could GC thread-local values in the case you describe (value
> strongly reference ThreadLocal), it is not worth substantially slowing
> down the facility to rectify this.  Initially ThreadLocal was written to
> support a small number of thread-local variables, consistent with common
> practice in the late '90s ( e.g., in Pthreads).  But Java programmers
> made very heavy (perhaps too heavy) use of ThreadLocal in the
> intervening years, so we sped it up repeatedly. We cannot afford to slow
> it down. Users do not take kindly to severe performance regressions.

Did you read the links?

Thomas Hawtin argues that his solution don't impose much of a penalty.

Here's a link to Sun's bug database:
   http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6254531

Please also read the "Evaluation" section that first brushed this off
with "you're insane", then changed tone completely.

I quote "tackline" in a comment: "The performance argument is extremely
weak, particularly with the patch supplied through the jdk-collaboration
java.net project. get should run at around the same speed as the
existing buggy code."

Why don't you at least sit down, check this through, and give a critique
of this code?

PS: Sorry for sounding so exasperated - I just feel "everyone" are
dodging the actual issue, and the potential for a solution. If there are
no way the ThreadLocal can be fixed without a rather significant
performance hit, then so be it. But at least let the experts look into
it.. Meaning you!

PPS: Given the problem we are discussion (the particular "reload"
issue), I would assume that also OSGi applications are affected by this?
(I haven't gotten into OSGi yet, but from my so far shallow
understanding, it uses classloaders to a large degree, and allows for
unloading and reloading and all that..)

PPPS: Don't laugh to hard, but this feature could potentially be made
system property configurable - one final static boolean could be
initialized based on some sys prop on ThreadLocal load, and then either
the larger, GC-able, somewhat slower structure, or the smaller,
non-GCable, somewhat faster structure was selected?! Given how many
other things, for example with Java 2D, are configurable this way, it
wouldn't be _completely_ insane.. IMHO, of course.

PPPPS: Is there any reason that I have to log in to see the archives of
this list?

Kind regards,
Endre.
_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: Threadlocals and memory leaks in J2EE

Doug Lea
In reply to this post by Endre Stølsvik-6

As the main author of current ThreadLocal implementation, I figure
I ought to say something. Here are a few somethings...

1. I agree with those who think that the problematic self-referential
usages are programming errors, or at least questionable practices.
But they are widespread, and worse,
their effects hurt people who have no control over their presence.
So something ought to be done to help them.

2. Last I left an exchange with Tom Hawtins about this last year,
it had occurred to me that some improvements to java.lang.ref
classes could lead to way to handle these cases that would have no
net performance loss, and maybe even an improvement.
But I haven't had a chance to pursue this.
I will try to somehow push up the priority of exploring this.

3. Stepping back a little, ThreadLocal is another one of those cases
that shows how hard it is to add a fundamental feature to an existing
language (they were added in 1.2) -- limitations seem to always
somehow show through. Not just in the case at hand, but also for
example that at a lower level, allocations should maintain
address locality among each thread's ThreadLocals. And just as
it is illegal to take the address of a local in Java and make
it accessible to other threads, it ought to be illegal to use
similar constructions to copy ref of a ThreadLocal into a static, etc.

-Doug


_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: Threadlocals and memory leaks in J2EE

Endre Stølsvik-6
Hi!

Doug Lea wrote:
> As the main author of current ThreadLocal implementation, I figure
> I ought to say something. Here are a few somethings...

hehe!

>
> 1. I agree with those who think that the problematic self-referential
> usages are programming errors, or at least questionable practices.

 > 3. .... And just as
 > it is illegal to take the address of a local in Java and make
 > it accessible to other threads, it ought to be illegal to use
 > similar constructions to copy ref of a ThreadLocal into a static, etc.

I'm a bit puzzled by these statements, as I read them to mean that you
think ThreadLocals should never be declared in a static field?

I can't get this to rhyme with the javadoc stating "ThreadLocal
instances are typically private static fields in classes that wish to
associate state with a thread (e.g., a user ID or Transaction ID)."

A static ThreadLocal is pretty much self-referential right there, aren't
they?

> But they are widespread, and worse,
> their effects hurt people who have no control over their presence.
> So something ought to be done to help them.
   ...
 > I will try to somehow push up the priority of exploring this.

This sounds really promising!

>
> 2. Last I left an exchange with Tom Hawtins about this last year,
> it had occurred to me that some improvements to java.lang.ref
> classes could lead to way to handle these cases that would have no
> net performance loss, and maybe even an improvement.

.. and even more promising!

Thanks,
Endre.
_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest
123