Velocity Reviews - Computer Hardware Reviews

Velocity Reviews > Newsgroups > Programming > Java > Proposed new Java feature

Reply
Thread Tools

Proposed new Java feature

 
 
Mike Schilling
Guest
Posts: n/a
 
      05-26-2012
First a few observations:

1. ThreadLocals may be, in general, an abomination, but there are situation
where they're the least of evils. And if you're building a framework in
which third-party code runs (e.g. a webserver), there's no way to disallow
their use.

2. ThreadLocals interact badly with ThreadPools, because the ThreadLocals
keep their value when the tyhread is put back into the pool. This can lead
to leaks and even potential security issues.

3. The current implementation of ThreadLocal is this: each thread contains a
map from the ThreadLocal instance to its value for that thread. (This is
slightly less intiutive than giving a ThreadLocal instance a map from Thread
to value, but has the advantage that the map, which is only used within one
thread, needn't be synchronized.)

Proposed feature: a static method on Thread that clears all ThreadLocals for
the current thread. By 3 it's trivial to implement. By 1 and 2 it's
needed. And ThreadPool implementations can simply call it directly before
putting the Thread back into the pool.


 
Reply With Quote
 
 
 
 
Robert Klemme
Guest
Posts: n/a
 
      05-27-2012
On 05/27/2012 01:11 AM, Mike Schilling wrote:
> First a few observations:
>
> 1. ThreadLocals may be, in general, an abomination, but there are situation
> where they're the least of evils. And if you're building a framework in
> which third-party code runs (e.g. a webserver), there's no way to disallow
> their use.


"abomination" is a too strong word: they are a tool with particular
usefulness and particular issues. They should definitively be used
sparingly because they carry state in a kind of hidden way. But there
are good use cases (e.g. attaching transaction context to a thread).

> 2. ThreadLocals interact badly with ThreadPools, because the ThreadLocals
> keep their value when the tyhread is put back into the pool. This can lead
> to leaks and even potential security issues.


I would actually consider this good interaction with thread pools: the
local stays around for as long as the thread lives. If you introduce
security issues this way than you are probably not using thread locals
properly. There are two things that you generally need to consider with
thread locals which both result from the fact that the life time of a
thread local value extends across a current method call (i.e. earlier
and later):

1. You need to be ready to calculate the value any time because it might
be the first time that you access it in the current thread.

2. You need to be aware that the ThreadLocal value will stay around
longer than the current method call. So if you want things removed from
it after the current call terminates you better ensure it's done
(usually in a finally block).

> 3. The current implementation of ThreadLocal is this: each thread contains a
> map from the ThreadLocal instance to its value for that thread. (This is
> slightly less intiutive than giving a ThreadLocal instance a map from Thread
> to value, but has the advantage that the map, which is only used within one
> thread, needn't be synchronized.)


Even more important: this construction will allow for GC of all thread
local objects when the thread dies. This is important since a
ThreadLocal instance often lives much longer than threads which might
use it (especially if it is a static member which is a typical use case).

> Proposed feature: a static method on Thread that clears all ThreadLocals for
> the current thread. By 3 it's trivial to implement. By 1 and 2 it's
> needed. And ThreadPool implementations can simply call it directly before
> putting the Thread back into the pool.


I am not convinced this is a good idea: the current design ensures that
all ThreadLocals are completely independent from each other. By
introducing this clear all method you can generate side effects on other
thread locals that might not be wanted - this could at least make things
significantly more inefficient because values have to be recalculated
much more often than intended. It may in fact introduce functional
bugs: Consider a thread context which is stored in a ThreadLocal before
your current method was invoked in order to carry it forward to methods
much deeper on the call stack (e.g. a method on a JCA connection). You
decide to do Thread.clearAllLocals() in this thread. The JCA method
cannot properly deal with the TX because the thread local value is gone
and the caller relies on the ThreadLocal to be still there and when it's
gone the TX cannot be properly finished.


Side note: it happened to me more than once that I found Java's standard
library design or implementation weird. And there are in fact bad
quirks (Vector, Hashtable) but often when I think longer about how they
did it I have to say it is done properly the way they did it. So the
standard lib is definitively better than one often thinks.

Kind regards

robert
 
Reply With Quote
 
 
 
 
markspace
Guest
Posts: n/a
 
      05-27-2012
On 5/26/2012 4:11 PM, Mike Schilling wrote:

> Proposed feature: a static method on Thread that clears all ThreadLocals for
> the current thread.
>



I can see your points. However, I don't have any real experience with
ThreadLocal, and when a neophyte agrees with your argument, that's a red
flag.

Here's a blog where someone seems to have the same issue as you.

<http://weblogs.java.net/blog/jjviana/archive/2010/06/10/threadlocal-thread-pool-bad-idea-or-dealing-apparent-glassfish-memor>

At the end of the comments, there's a suggestion to use
ThreadLocal::remove(), with the implication that it allows the thread
local variable to be garbage collection. Is there a reason that doesn't
work for you?

My other thought is that "for the current thread" could be improved with
"for a given thread." So, inside an Executor, I can just call

Thread t = ...
// .. use the thread ..
Thread.removeLocals( t );
// now add the thread back into the pool...

And this seems better because I don't have to rely on the users of a
thread remembering to do it themselves. External control seems better here.

 
Reply With Quote
 
Tom Anderson
Guest
Posts: n/a
 
      05-27-2012
On Sat, 26 May 2012, Mike Schilling wrote:

> Proposed feature: a static method on Thread that clears all ThreadLocals
> for the current thread. By 3 it's trivial to implement. By 1 and 2
> it's needed. And ThreadPool implementations can simply call it directly
> before putting the Thread back into the pool.


It's a good idea. Tomcat already has some sort of eldritch hack to do
exactly this to request threads; it logs a warning if it finds undeleted
entries. There's a bit about it here:

http://wiki.apache.org/tomcat/MemoryLeakProtection

They, and other container manufacturers, might be glad of an official way
of doing this.

tom

--
Kein Mehrheit Fur Die Mitleid
 
Reply With Quote
 
Mike Schilling
Guest
Posts: n/a
 
      05-27-2012

"markspace" <-@.> wrote in message news:jptkmp$vbg$(E-Mail Removed)...
> On 5/26/2012 4:11 PM, Mike Schilling wrote:
>
>> Proposed feature: a static method on Thread that clears all ThreadLocals
>> for
>> the current thread.
>>

>
>
> I can see your points. However, I don't have any real experience with
> ThreadLocal, and when a neophyte agrees with your argument, that's a red
> flag.
>
> Here's a blog where someone seems to have the same issue as you.
>
> <http://weblogs.java.net/blog/jjviana/archive/2010/06/10/threadlocal-thread-pool-bad-idea-or-dealing-apparent-glassfish-memor>
>
> At the end of the comments, there's a suggestion to use
> ThreadLocal::remove(), with the implication that it allows the thread
> local variable to be garbage collection. Is there a reason that doesn't
> work for you?


That acts on an individual ThreadLocal (and works quite well), but it
doens't allow removing all ThreadLocals that might have been accumlated.

>
> My other thought is that "for the current thread" could be improved with
> "for a given thread." So, inside an Executor, I can just call
>
> Thread t = ...
> // .. use the thread ..
> Thread.removeLocals( t );
> // now add the thread back into the pool...
>
> And this seems better because I don't have to rely on the users of a
> thread remembering to do it themselves. External control seems better
> here.
>


Same comment. What I'm asking for is Thread.removeLocals(), which doesn't
currently exist.


 
Reply With Quote
 
Mike Schilling
Guest
Posts: n/a
 
      05-27-2012
"Robert Klemme" <(E-Mail Removed)> wrote in message
news:(E-Mail Removed)...
>> 2. ThreadLocals interact badly with ThreadPools, because the ThreadLocals
>> keep their value when the tyhread is put back into the pool. This can
>> lead
>> to leaks and even potential security issues.

>
> I would actually consider this good interaction with thread pools: the
> local stays around for as long as the thread lives. If you introduce
> security issues this way than you are probably not using thread locals
> properly. There are two things that you generally need to consider with
> thread locals which both result from the fact that the life time of a
> thread local value extends across a current method call (i.e. earlier and
> later):



OK. Now, coinsider these two cases (for, say, a webserver):

1. I create a new thread to handle each new request.
2. I optimize (1) by using a thread pool to minimize thread creation.

I want those two to behave identically (other than performance). To acheive
that, I need to be able to kill all the ThreadLocals when putting the
Threads back into the pool for later reuse. Otherwise

A. The ThreadLocals for threads in the pool cause packratting.
B. A reused thread contains context created during its previous use. This
may be context that the user correspnding to the request currently being
handled by the thread should not be able to see.


 
Reply With Quote
 
Daniel Pitts
Guest
Posts: n/a
 
      05-27-2012
On 5/27/12 11:00 AM, Mike Schilling wrote:
> "markspace"<-@.> wrote in message news:jptkmp$vbg$(E-Mail Removed)...
>> On 5/26/2012 4:11 PM, Mike Schilling wrote:
>>
>>> Proposed feature: a static method on Thread that clears all ThreadLocals
>>> for
>>> the current thread.
>>>

>>
>>
>> I can see your points. However, I don't have any real experience with
>> ThreadLocal, and when a neophyte agrees with your argument, that's a red
>> flag.
>>
>> Here's a blog where someone seems to have the same issue as you.
>>
>> <http://weblogs.java.net/blog/jjviana/archive/2010/06/10/threadlocal-thread-pool-bad-idea-or-dealing-apparent-glassfish-memor>
>>
>> At the end of the comments, there's a suggestion to use
>> ThreadLocal::remove(), with the implication that it allows the thread
>> local variable to be garbage collection. Is there a reason that doesn't
>> work for you?

>
> That acts on an individual ThreadLocal (and works quite well), but it
> doens't allow removing all ThreadLocals that might have been accumlated.

You're basically saying "This type of resource can leak if not cleared
appropriately, so there should be a 'Release all resources' method."

When paraphrased that way, does that make it clearer why it isn't a good
idea? It would be about the same as a "File.closeAll()" or a
"Socket.closeAll()" call. Extremely dangerous and only a crutch for not
doing the right thing to begin with.
 
Reply With Quote
 
Mike Schilling
Guest
Posts: n/a
 
      05-27-2012

"Daniel Pitts" <(E-Mail Removed)> wrote in message
news:8Euwr.47425$(E-Mail Removed)...
> On 5/27/12 11:00 AM, Mike Schilling wrote:
>> "markspace"<-@.> wrote in message news:jptkmp$vbg$(E-Mail Removed)...
>>> On 5/26/2012 4:11 PM, Mike Schilling wrote:
>>>
>>>> Proposed feature: a static method on Thread that clears all
>>>> ThreadLocals
>>>> for
>>>> the current thread.
>>>>
>>>
>>>
>>> I can see your points. However, I don't have any real experience with
>>> ThreadLocal, and when a neophyte agrees with your argument, that's a red
>>> flag.
>>>
>>> Here's a blog where someone seems to have the same issue as you.
>>>
>>> <http://weblogs.java.net/blog/jjviana/archive/2010/06/10/threadlocal-thread-pool-bad-idea-or-dealing-apparent-glassfish-memor>
>>>
>>> At the end of the comments, there's a suggestion to use
>>> ThreadLocal::remove(), with the implication that it allows the thread
>>> local variable to be garbage collection. Is there a reason that doesn't
>>> work for you?

>>
>> That acts on an individual ThreadLocal (and works quite well), but it
>> doens't allow removing all ThreadLocals that might have been accumlated.

> You're basically saying "This type of resource can leak if not cleared
> appropriately, so there should be a 'Release all resources' method."
>
> When paraphrased that way, does that make it clearer why it isn't a good
> idea? It would be about the same as a "File.closeAll()" or a
> "Socket.closeAll()" call. Extremely dangerous and only a crutch for not
> doing the right thing to begin with.


Or a "collect all unused memory" call . Clearly, that's a crutch for not
keeping track of memory allocation properly in the first place. And the
fact that files and sockets are closed when a process exits is yet another
crutch.


 
Reply With Quote
 
Eric Sosman
Guest
Posts: n/a
 
      05-27-2012
On 5/27/2012 3:04 PM, Mike Schilling wrote:
> "Daniel Pitts"<(E-Mail Removed)> wrote in message
> news:8Euwr.47425$(E-Mail Removed)...
>> On 5/27/12 11:00 AM, Mike Schilling wrote:
>>> "markspace"<-@.> wrote in message news:jptkmp$vbg$(E-Mail Removed)...
>>>> On 5/26/2012 4:11 PM, Mike Schilling wrote:
>>>>
>>>>> Proposed feature: a static method on Thread that clears all
>>>>> ThreadLocals
>>>>> for
>>>>> the current thread.
>>>>>
>>>>
>>>>
>>>> I can see your points. However, I don't have any real experience with
>>>> ThreadLocal, and when a neophyte agrees with your argument, that's a red
>>>> flag.
>>>>
>>>> Here's a blog where someone seems to have the same issue as you.
>>>>
>>>> <http://weblogs.java.net/blog/jjviana/archive/2010/06/10/threadlocal-thread-pool-bad-idea-or-dealing-apparent-glassfish-memor>
>>>>
>>>> At the end of the comments, there's a suggestion to use
>>>> ThreadLocal::remove(), with the implication that it allows the thread
>>>> local variable to be garbage collection. Is there a reason that doesn't
>>>> work for you?
>>>
>>> That acts on an individual ThreadLocal (and works quite well), but it
>>> doens't allow removing all ThreadLocals that might have been accumlated.

>> You're basically saying "This type of resource can leak if not cleared
>> appropriately, so there should be a 'Release all resources' method."
>>
>> When paraphrased that way, does that make it clearer why it isn't a good
>> idea? It would be about the same as a "File.closeAll()" or a
>> "Socket.closeAll()" call. Extremely dangerous and only a crutch for not
>> doing the right thing to begin with.

>
> Or a "collect all unused memory" call . Clearly, that's a crutch for not
> keeping track of memory allocation properly in the first place. And the
> fact that files and sockets are closed when a process exits is yet another
> crutch.


I'm with Daniel. Your code uses classes that you wrote, and
classes you got from somewhere else -- Sioux Unusuals, perhaps.
And if those classes use ThreadLocals for their own purposes, and
you blithely destroy them all, what happens then? Or what about
the class that invoked your code, passing an InheritableThreadLocal?
Is it a good idea to change parts of your invoker's state that you
don't understand, that you aren't even aware of?

--
Eric Sosman
http://www.velocityreviews.com/forums/(E-Mail Removed)d
 
Reply With Quote
 
Mike Schilling
Guest
Posts: n/a
 
      05-27-2012

"Eric Sosman" <(E-Mail Removed)> wrote in message
news:jptvdf$1s5$(E-Mail Removed)...
> On 5/27/2012 3:04 PM, Mike Schilling wrote:
>> "Daniel Pitts"<(E-Mail Removed)> wrote in message
>> news:8Euwr.47425$(E-Mail Removed)...
>>> On 5/27/12 11:00 AM, Mike Schilling wrote:
>>>> "markspace"<-@.> wrote in message news:jptkmp$vbg$(E-Mail Removed)...
>>>>> On 5/26/2012 4:11 PM, Mike Schilling wrote:
>>>>>
>>>>>> Proposed feature: a static method on Thread that clears all
>>>>>> ThreadLocals
>>>>>> for
>>>>>> the current thread.
>>>>>>
>>>>>
>>>>>
>>>>> I can see your points. However, I don't have any real experience with
>>>>> ThreadLocal, and when a neophyte agrees with your argument, that's a
>>>>> red
>>>>> flag.
>>>>>
>>>>> Here's a blog where someone seems to have the same issue as you.
>>>>>
>>>>> <http://weblogs.java.net/blog/jjviana/archive/2010/06/10/threadlocal-thread-pool-bad-idea-or-dealing-apparent-glassfish-memor>
>>>>>
>>>>> At the end of the comments, there's a suggestion to use
>>>>> ThreadLocal::remove(), with the implication that it allows the thread
>>>>> local variable to be garbage collection. Is there a reason that
>>>>> doesn't
>>>>> work for you?
>>>>
>>>> That acts on an individual ThreadLocal (and works quite well), but it
>>>> doens't allow removing all ThreadLocals that might have been
>>>> accumlated.
>>> You're basically saying "This type of resource can leak if not cleared
>>> appropriately, so there should be a 'Release all resources' method."
>>>
>>> When paraphrased that way, does that make it clearer why it isn't a good
>>> idea? It would be about the same as a "File.closeAll()" or a
>>> "Socket.closeAll()" call. Extremely dangerous and only a crutch for not
>>> doing the right thing to begin with.

>>
>> Or a "collect all unused memory" call . Clearly, that's a crutch for not
>> keeping track of memory allocation properly in the first place. And the
>> fact that files and sockets are closed when a process exits is yet
>> another
>> crutch.

>
> I'm with Daniel. Your code uses classes that you wrote, and
> classes you got from somewhere else -- Sioux Unusuals, perhaps.
> And if those classes use ThreadLocals for their own purposes, and
> you blithely destroy them all, what happens then? Or what about
> the class that invoked your code, passing an InheritableThreadLocal?
> Is it a good idea to change parts of your invoker's state that you
> don't understand, that you aren't even aware of?


Consider the use case again. I've written a container. It procures a
thread and lets third-party code run in it. Currently I have two choices:

1. Create a fresh thread each time. This kills all the ThreadLocals that
might be lying around, but doesn't perform very well.
2. Use a ThreadPool. This improves performance at the cost of letting
ThreadLocals packrat (which, in the worst case, holds entire ClassLoaders in
memory).

I'm greedy; I want the performance without the memory issues. Or, to say it
another way, when getting a Thread from a ThreadPool, it should be
completely indistinguishable whether it's a recycled or brand-new thread.


 
Reply With Quote
 
 
 
Reply

Thread Tools

Posting Rules
You may not post new threads
You may not post replies
You may not post attachments
You may not edit your posts

BB code is On
Smilies are On
[IMG] code is On
HTML code is Off
Trackbacks are On
Pingbacks are On
Refbacks are Off


Similar Threads
Thread Thread Starter Forum Replies Last Post
Vote on proposed VS feature. Scott M. ASP .Net 4 01-20-2008 08:40 PM
Proposed new PEP: print to expand generators James J. Besemer Python 3 06-05-2006 02:33 AM
Request for feedback: proposed new Perl modules to aid VHDL projects Michael Attenborough VHDL 22 03-13-2006 04:36 PM
Proposed new collection methods Mike Meyer Python 9 08-08-2005 01:32 AM
[Meta] *Poll Results* Proposed new, moderated digital photography group (rec.photo.digital.moderated) Lionel Digital Photography 27 07-15-2004 11:24 AM



Advertisments