Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > comp.lang.java.programmer > #14824 > unrolled thread

Proposed new Java feature

Back to article view | Back to comp.lang.java.programmer

Contents

  Proposed new Java feature "Mike Schilling" <mscottschilling@hotmail.com> - 2012-05-26 16:11 -0700
    Re: Proposed new Java feature Robert Klemme <shortcutter@googlemail.com> - 2012-05-27 13:32 +0200
      Re: Proposed new Java feature "Mike Schilling" <mscottschilling@hotmail.com> - 2012-05-27 11:14 -0700
        Re: Proposed new Java feature Robert Klemme <shortcutter@googlemail.com> - 2012-05-28 12:13 +0200
        Re: Proposed new Java feature v_borchert@despammed.com (Volker Borchert) - 2012-06-04 20:16 +0000
    Re: Proposed new Java feature markspace <-@.> - 2012-05-27 09:28 -0700
      Re: Proposed new Java feature "Mike Schilling" <mscottschilling@hotmail.com> - 2012-05-27 11:00 -0700
        Re: Proposed new Java feature Daniel Pitts <newsgroup.nospam@virtualinfinity.net> - 2012-05-27 11:46 -0700
          Re: Proposed new Java feature "Mike Schilling" <mscottschilling@hotmail.com> - 2012-05-27 12:04 -0700
            Re: Proposed new Java feature Eric Sosman <esosman@ieee-dot-org.invalid> - 2012-05-27 15:30 -0400
              Re: Proposed new Java feature "Mike Schilling" <mscottschilling@hotmail.com> - 2012-05-27 12:59 -0700
                Re: Proposed new Java feature Eric Sosman <esosman@ieee-dot-org.invalid> - 2012-05-27 17:51 -0400
                  Re: Proposed new Java feature markspace <-@.> - 2012-05-27 15:20 -0700
                    Re: Proposed new Java feature Robert Klemme <shortcutter@googlemail.com> - 2012-05-28 12:13 +0200
                      Re: Proposed new Java feature markspace <-@.> - 2012-05-28 08:28 -0700
                        Re: Proposed new Java feature Robert Klemme <shortcutter@googlemail.com> - 2012-05-28 19:29 +0200
                          Re: Proposed new Java feature markspace <-@.> - 2012-05-28 12:16 -0700
                            Re: Proposed new Java feature Robert Klemme <shortcutter@googlemail.com> - 2012-05-28 22:58 +0200
                              Re: Proposed new Java feature "Mike Schilling" <mscottschilling@hotmail.com> - 2012-05-28 21:40 -0700
                                Re: Proposed new Java feature Robert Klemme <shortcutter@googlemail.com> - 2012-05-29 22:52 +0200
                                  Re: Proposed new Java feature "Mike Schilling" <mscottschilling@hotmail.com> - 2012-05-29 18:53 -0700
                  Re: Proposed new Java feature Tom Anderson <twic@urchin.earth.li> - 2012-05-27 23:27 +0100
                    Re: Proposed new Java feature Robert Klemme <shortcutter@googlemail.com> - 2012-05-28 12:22 +0200
                  Re: Proposed new Java feature "Mike Schilling" <mscottschilling@hotmail.com> - 2012-05-27 21:43 -0700
                    Re: Proposed new Java feature Andreas Leitgeb <avl@gamma.logic.tuwien.ac.at> - 2012-05-29 14:32 +0000
    Re: Proposed new Java feature Tom Anderson <twic@urchin.earth.li> - 2012-05-27 18:43 +0100
    Re: Proposed new Java feature Kevin McMurtrie <mcmurtrie@pixelmemory.us> - 2012-06-02 12:41 -0700
      Re: Proposed new Java feature markspace <-@.> - 2012-06-02 15:55 -0700

Page 1 of 2  [1] 2  Next page →

#14824 — Proposed new Java feature

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.

[toc] | [next] | [standalone]

#14829

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

[toc] | [prev] | [next] | [standalone]

#14834

"Robert Klemme" <shortcutter@googlemail.com> wrote in message 
news:a2ehm0Fee8U1@mid.individual.net...
>> 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. 

[toc] | [prev] | [next] | [standalone]

#14857

On 05/27/2012 08:14 PM, Mike Schilling wrote:
> "Robert Klemme"<shortcutter@googlemail.com>  wrote in message
> news:a2ehm0Fee8U1@mid.individual.net...
>>> 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

No, you don't.  If you employ proper cache size management schemes (e.g. 
via a LRU) then you do not get rid of the ThreadLocal after each 
request.  If for any reason this is not possible and you want to get rid 
of state after the request then it should be done per individual 
ThreadLocal and not globally for all ThreadLocals.

> A. The ThreadLocals for threads in the pool cause packratting.

Then you do not employ proper cache size management.  You always need to 
be aware of the life time difference between a method call and a 
ThreadLocal and code appropriately.  Not doing this is not following the 
contract of ThreadLocal.

> 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.

Then you are not using ThreadLocal properly, i.e. have omitted proper 
cleanup (e.g. because you either did not do it or did not do it in a 
safe way, i.e. finally block).

Kind regards

	robert

[toc] | [prev] | [next] | [standalone]

#15063

Mike Schilling wrote:
> 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

Your threads should know which ThreadLocal they have used, so you
should be able to get away with ThreadLocal.remove().

I would perhaps subclass Thread to PoolableThread and therein define
a method reset() or something, which will call that method for all
known used ThreadLocal, and clear other semi persistent state. This
method would be called when returning the PoolableThread to the pool.

-- 

"I'm a doctor, not a mechanic." Dr Leonard McCoy <mccoy@ncc1701.starfleet.fed>
"I'm a mechanic, not a doctor." Volker Borchert  <v_borchert@despammed.com>

[toc] | [prev] | [next] | [standalone]

#14831

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.

[toc] | [prev] | [next] | [standalone]

#14833

"markspace" <-@.> wrote in message news:jptkmp$vbg$1@dont-email.me...
> 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.

[toc] | [prev] | [next] | [standalone]

#14836

On 5/27/12 11:00 AM, Mike Schilling wrote:
> "markspace"<-@.>  wrote in message news:jptkmp$vbg$1@dont-email.me...
>> 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.

[toc] | [prev] | [next] | [standalone]

#14837

"Daniel Pitts" <newsgroup.nospam@virtualinfinity.net> wrote in message 
news:8Euwr.47425$On2.20024@newsfe16.iad...
> On 5/27/12 11:00 AM, Mike Schilling wrote:
>> "markspace"<-@.>  wrote in message news:jptkmp$vbg$1@dont-email.me...
>>> 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. 

[toc] | [prev] | [next] | [standalone]

#14838

On 5/27/2012 3:04 PM, Mike Schilling wrote:
> "Daniel Pitts"<newsgroup.nospam@virtualinfinity.net>  wrote in message
> news:8Euwr.47425$On2.20024@newsfe16.iad...
>> On 5/27/12 11:00 AM, Mike Schilling wrote:
>>> "markspace"<-@.>   wrote in message news:jptkmp$vbg$1@dont-email.me...
>>>> 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
esosman@ieee-dot-org.invalid

[toc] | [prev] | [next] | [standalone]

#14839

"Eric Sosman" <esosman@ieee-dot-org.invalid> wrote in message 
news:jptvdf$1s5$1@dont-email.me...
> On 5/27/2012 3:04 PM, Mike Schilling wrote:
>> "Daniel Pitts"<newsgroup.nospam@virtualinfinity.net>  wrote in message
>> news:8Euwr.47425$On2.20024@newsfe16.iad...
>>> On 5/27/12 11:00 AM, Mike Schilling wrote:
>>>> "markspace"<-@.>   wrote in message news:jptkmp$vbg$1@dont-email.me...
>>>>> 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. 

[toc] | [prev] | [next] | [standalone]

#14840

On 5/27/2012 3:59 PM, Mike Schilling wrote:
> "Eric Sosman"<esosman@ieee-dot-org.invalid>  wrote in message
> news:jptvdf$1s5$1@dont-email.me...
>> On 5/27/2012 3:04 PM, Mike Schilling wrote:
>>> "Daniel Pitts"<newsgroup.nospam@virtualinfinity.net>   wrote in message
>>> news:8Euwr.47425$On2.20024@newsfe16.iad...
>>>> On 5/27/12 11:00 AM, Mike Schilling wrote:
>>>>> "markspace"<-@.>    wrote in message news:jptkmp$vbg$1@dont-email.me...
>>>>>> 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 understood it fine the first time around, thanks.  If you
think of anything new to add, pray do so.

> [... reiteration of old material ...]

     I notice that you do not address any of the issues Daniel or I
raised; you just repeat what you "want."  I know three-year-olds
who can do that, but I don't rush to grant their every whim.

-- 
Eric Sosman
esosman@ieee-dot-org.invalid

[toc] | [prev] | [next] | [standalone]

#14841

On 5/27/2012 2:51 PM, Eric Sosman wrote:
> I understood it fine the first time around, thanks. If you
> think of anything new to add, pray do so.


This is uncharitable.  In particular because I think his point went 
whooshing over your head, and you didn't understand it at all.

He's saying the thread is done.  Kaput.  Any Sioux Unusual class using 
the thread will loose its thread locals, because at this point it 
expects the thread to die.

So instead of allowing the thread to die, we re-use it.  But what would 
Sioux Unusual do?  How would it expect this thread to be matched up with 
the same object ever again?  There's no guarantee of that.  Thread 
assignment from the executor is random.  Worse, another Sioux Unusual 
object could see the thread local from some different object, and assume 
it's been assigned from its own invocation, when it clearly hasn't.

Really, the problem is that Sioux Unusual is broken for this 
application, and shouldn't be used when threads are gong to be re-used 
and randomly reassigned.  But that's not what he's talking about.  He's 
talking about the case where he doesn't need the thread locals to 
persist, but he does need to use them, and to release them too.

The posts by myself and Tom should indicate that Mike is not alone in 
this need.  He's got a real problem.  That you insist that his 
requirements should be obviated because you or someone else might have 
some different requirement... well that just doesn't make sense.

[toc] | [prev] | [next] | [standalone]

#14858

On 05/28/2012 12:20 AM, markspace wrote:
> On 5/27/2012 2:51 PM, Eric Sosman wrote:
>> I understood it fine the first time around, thanks. If you
>> think of anything new to add, pray do so.
>
>
> This is uncharitable. In particular because I think his point went
> whooshing over your head, and you didn't understand it at all.
>
> He's saying the thread is done. Kaput. Any Sioux Unusual class using the
> thread will loose its thread locals, because at this point it expects
> the thread to die.

How can you know if you did not create the thread?  If the thread dies 
as in "terminates and then is GC'ed" there is no need for additional 
cleanup because GC will do that just fine.

> So instead of allowing the thread to die, we re-use it. But what would
> Sioux Unusual do? How would it expect this thread to be matched up with
> the same object ever again? There's no guarantee of that. Thread
> assignment from the executor is random. Worse, another Sioux Unusual
> object could see the thread local from some different object, and assume
> it's been assigned from its own invocation, when it clearly hasn't.
>
> Really, the problem is that Sioux Unusual is broken for this
> application, and shouldn't be used when threads are gong to be re-used
> and randomly reassigned. But that's not what he's talking about. He's
> talking about the case where he doesn't need the thread locals to
> persist, but he does need to use them, and to release them too.

But he cannot know how long ThreadLocals need to stay which are not 
created in his code.  If cleanup is needed then it must be done per 
ThreadLocal and never globally.

> The posts by myself and Tom should indicate that Mike is not alone in
> this need. He's got a real problem. That you insist that his
> requirements should be obviated because you or someone else might have
> some different requirement... well that just doesn't make sense.

If there is a real problem then it's not using ThreadLocal properly. 
That should be fixed.  And not a dangerous functionality added to 
standard library where changes are enormous to wreck havoc on all sorts 
of code.

Kind regards

	robert

[toc] | [prev] | [next] | [standalone]

#14862

On 5/28/2012 3:13 AM, Robert Klemme wrote:
> How can you know if you did not create the thread?


But we are creating the thread.  It's part of a thread pool for general 
use.  That's his use case; it's the fundamental crux of his request.


> If the thread dies as
> in "terminates and then is GC'ed" there is no need for additional
> cleanup because GC will do that just fine.


Threads in a thread pool don't die, they are re-used.  Hence the need 
for manual (even external) clean-up.


> But he cannot know how long ThreadLocals need to stay which are not
> created in his code.


When the thread is returned to the pool, all thread locals must be 
marked as invalid.  Period.


> If there is a real problem then it's not using ThreadLocal properly.
> That should be fixed. And not a dangerous functionality added to
> standard library where changes are enormous to wreck havoc on all sorts
> of code.


I can think of a few scenarios where it would be useful to use both 
thread locals and a thread pool, so while maybe "dangerous" it's still 
something we should investigate properly.  Hacks like the reflective 
code I linked to earlier that dump private fields seem a lot more 
"dangerous" to me, yet they are currently needed.

[toc] | [prev] | [next] | [standalone]

#14868

On 05/28/2012 05:28 PM, markspace wrote:
> On 5/28/2012 3:13 AM, Robert Klemme wrote:
>> How can you know if you did not create the thread?
>
> But we are creating the thread. It's part of a thread pool for general
> use. That's his use case; it's the fundamental crux of his request.

Maybe there is a misunderstanding: I read your statement to mean that 
"Sioux Unusual class" assumes the thread to be dead while it was created 
by someone else (presumably the thread pool).  So it cannot know 
anything about thread creation and dead because it does not have any 
control over the thread's lifetime.

> I can think of a few scenarios where it would be useful to use both
> thread locals and a thread pool, so while maybe "dangerous" it's still
> something we should investigate properly.

It's certainly a useful idiom: the thread pool saves the overhead of 
creating and destroying threads (and can also control the amount of 
concurrency in an application) and ThreadLocals cache state which can be 
accessed without synchronization overhead.  Killing that every time a 
thread returns to the pool will make caching much less efficient.

> Hacks like the reflective code
> I linked to earlier that dump private fields seem a lot more "dangerous"
> to me, yet they are currently needed.

IMHO they are OK as long as they are used to log debugging warnings 
during development but as I said, a general mechanism to clear all 
thread locals does have more drawbacks than advantages.  Now you are 
hunting memory leaks through badly coded thread locals, then you might 
have to hunt down weird application behavior because of state 
disappearing which is expected to be still there.  Remember that *any* 
method in the thread's call stack can invoke cleaner which means that 
all methods upwards the call stack will not find their ThreadLocals back 
once control returns to them.

If we think about extending ThreadLocal's functionality at all then I 
would think in the direction of registering a cleanup handle (callback 
interface) with a ThreadLocal (or defining an empty method in 
ThreadLocal which can be overridden).  This handle would be invoked by 
the Thread itself prior to termination but could also be invoked by a 
thread pool or other framework.  Even that functionality might introduce 
bad bugs since - again - all methods on the call stack would be able to 
invoke it.  At least the creator of the ThreadLocal would have a chance 
to detect the situation and report a proper error (illegalstate for 
example).

Illustration

public class ThreadLocal<T> {
     static class ThreadLocalMap {

         private void remove(ThreadLocal<X> key) {
             Entry[] tab = table;
             int len = tab.length;
             int i = key.threadLocalHashCode & (len-1);
             for (Entry e = tab[i];
                  e != null;
                  e = tab[i = nextIndex(i, len)]) {
                 if (e.get() == key) {
                     // invoke cleanup code:
                     key.cleanup((X) e.value);

                     e.clear();
                     expungeStaleEntry(i);
                     return;
                 }
             }

   }

   /**
    * This method does nothing. Sub classes must override as they see fit.
    * @param the value of a thread which must be cleared.
    * @see #remove()
    */
   protected void cleanup(T val) {
     // nop
   }

   // Now we can even allow a cleanup all
   public static removeAll() {
     // pseudo code
     for (final ThreadLocal<?> tl : allLocalsOfThisThread()) {
       tl.remove();
     }
   }
}

However I think about it: the idea of simply clearing ThreadLocal 
references still does not become a good idea.  The creator of a 
ThreadLocal needs to consider how it is used and how cleanup is done. 
Everything else introduces dangerous side effects which lead to bugs 
which are at least as hard to find as those memory leaks.

Kind regards

	robert

[toc] | [prev] | [next] | [standalone]

#14869

On 5/28/2012 10:29 AM, Robert Klemme wrote:

> However I think about it: the idea of simply clearing ThreadLocal
> references still does not become a good idea. The creator of a
> ThreadLocal needs to consider how it is used and how cleanup is done.
> Everything else introduces dangerous side effects which lead to bugs
> which are at least as hard to find as those memory leaks.


Naw, I just disagree.  Saying that the creator is responsible for 
cleanup is like saying that because some objects need an explicit 
"close," we should force those semantics onto all objects.

If you have a thread local that requires special semantics, go ahead and 
provide the special code for those objects.  The rest of the code can 
use a common cleanup up mechanism that MOST objects require: just GC them.

[toc] | [prev] | [next] | [standalone]

#14871

On 28.05.2012 21:16, markspace wrote:
> On 5/28/2012 10:29 AM, Robert Klemme wrote:
>
>> However I think about it: the idea of simply clearing ThreadLocal
>> references still does not become a good idea. The creator of a
>> ThreadLocal needs to consider how it is used and how cleanup is done.
>> Everything else introduces dangerous side effects which lead to bugs
>> which are at least as hard to find as those memory leaks.
>
> Naw, I just disagree. Saying that the creator is responsible for cleanup
> is like saying that because some objects need an explicit "close," we
> should force those semantics onto all objects.

The cleanup method does not need to be overridden - that's why I did not 
make it abstract.  There is no semantic forced on all objects.

Also, I don't see the implication: If clearing references is sufficient 
then that's perfectly OK.  But, as you noticed, there are objects which 
require some explicit cleanup.  Only the author of the code which 
creates a ThreadLocal can know how it must be cleaned up.  With the 
global reference nulling you are actually forcing semantics on all 
because then there is just this one way.  Since 
ThreadLocal.initialValue() is there for custom initialization, it's as 
reasonable to provide a hook for resource deallocation code. (Btw., 
that's the same pattern callback interfaces for object pools employ.)

If one does not want the cleanup hook for all ThreadLocals, one can 
place the cleanup functionality in a sub class of ThreadLocal thus not 
affecting existing code at all.

> If you have a thread local that requires special semantics, go ahead and
> provide the special code for those objects. The rest of the code can use
> a common cleanup up mechanism that MOST objects require: just GC them.

The point is that with the global reference cleanup the specific code 
does not have a chance to run because it does not know when.  The proper 
point in time is just before or during remove().

Kind regards

	robert

-- 
remember.guy do |as, often| as.you_can - without end
http://blog.rubybestpractices.com/

[toc] | [prev] | [next] | [standalone]

#14873

"Robert Klemme" <shortcutter@googlemail.com> wrote in message 
news:a2i77nFik2U1@mid.individual.net...
> The point is that with the global reference cleanup the specific code does 
> not have a chance to run because it does not know when.  The proper point 
> in time is just before or during remove().

How would the cleanup code knew when to run during Thread exit? 

[toc] | [prev] | [next] | [standalone]

#14908

On 05/29/2012 06:40 AM, Mike Schilling wrote:
> "Robert Klemme"<shortcutter@googlemail.com>  wrote in message
> news:a2i77nFik2U1@mid.individual.net...
>> The point is that with the global reference cleanup the specific code does
>> not have a chance to run because it does not know when.  The proper point
>> in time is just before or during remove().
>
> How would the cleanup code knew when to run during Thread exit?

Obviously that would have to be implemented in class Thread using a 
finally block.

	robert

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | comp.lang.java.programmer

csiph-web