Proposed new Java feature

Discussion in 'Java' started by Mike Schilling, May 27, 2012.

  1. 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.
    Mike Schilling, May 27, 2012
    #1
    1. Advertising

  2. 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
    Robert Klemme, May 27, 2012
    #2
    1. Advertising

  3. Mike Schilling

    markspace Guest

    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.
    markspace, May 27, 2012
    #3
  4. Mike Schilling

    Tom Anderson Guest

    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
    Tom Anderson, May 27, 2012
    #4
  5. "markspace" <-@.> wrote in message news:jptkmp$vbg$...
    > 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.
    Mike Schilling, May 27, 2012
    #5
  6. "Robert Klemme" <> wrote in message
    news:...
    >> 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.
    Mike Schilling, May 27, 2012
    #6
  7. Mike Schilling

    Daniel Pitts Guest

    On 5/27/12 11:00 AM, Mike Schilling wrote:
    > "markspace"<-@.> wrote in message news:jptkmp$vbg$...
    >> 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.
    Daniel Pitts, May 27, 2012
    #7
  8. "Daniel Pitts" <> wrote in message
    news:8Euwr.47425$...
    > On 5/27/12 11:00 AM, Mike Schilling wrote:
    >> "markspace"<-@.> wrote in message news:jptkmp$vbg$...
    >>> 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.
    Mike Schilling, May 27, 2012
    #8
  9. Mike Schilling

    Eric Sosman Guest

    On 5/27/2012 3:04 PM, Mike Schilling wrote:
    > "Daniel Pitts"<> wrote in message
    > news:8Euwr.47425$...
    >> On 5/27/12 11:00 AM, Mike Schilling wrote:
    >>> "markspace"<-@.> wrote in message news:jptkmp$vbg$...
    >>>> 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
    d
    Eric Sosman, May 27, 2012
    #9
  10. "Eric Sosman" <> wrote in message
    news:jptvdf$1s5$...
    > On 5/27/2012 3:04 PM, Mike Schilling wrote:
    >> "Daniel Pitts"<> wrote in message
    >> news:8Euwr.47425$...
    >>> On 5/27/12 11:00 AM, Mike Schilling wrote:
    >>>> "markspace"<-@.> wrote in message news:jptkmp$vbg$...
    >>>>> 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.
    Mike Schilling, May 27, 2012
    #10
  11. Mike Schilling

    Eric Sosman Guest

    On 5/27/2012 3:59 PM, Mike Schilling wrote:
    > "Eric Sosman"<> wrote in message
    > news:jptvdf$1s5$...
    >> On 5/27/2012 3:04 PM, Mike Schilling wrote:
    >>> "Daniel Pitts"<> wrote in message
    >>> news:8Euwr.47425$...
    >>>> On 5/27/12 11:00 AM, Mike Schilling wrote:
    >>>>> "markspace"<-@.> wrote in message news:jptkmp$vbg$...
    >>>>>> 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
    d
    Eric Sosman, May 27, 2012
    #11
  12. Mike Schilling

    markspace Guest

    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.
    markspace, May 27, 2012
    #12
  13. Mike Schilling

    Tom Anderson Guest

    On Sun, 27 May 2012, Eric Sosman wrote:

    > On 5/27/2012 3:59 PM, Mike Schilling wrote:
    >> "Eric Sosman"<> wrote in message
    >> news:jptvdf$1s5$...
    >>> On 5/27/2012 3:04 PM, Mike Schilling wrote:
    >>>> "Daniel Pitts"<> wrote in message
    >>>> news:8Euwr.47425$...
    >>>>> On 5/27/12 11:00 AM, Mike Schilling wrote:
    >>>>>> "markspace"<-@.> wrote in message news:jptkmp$vbg$...
    >>>>>>> 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.
    >>>>>>>
    >>>>>>> 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.


    It seems to me that you did not.

    > If you think of anything new to add, pray do so.


    Mike added this pithy statement of the goal:

    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.

    At the moment, it is *not* possible to achieve that, because ThreadLocals
    will survive recycling.

    One way of looking at a thread pool is that it provides a supply of fresh
    short-lived virtual threads, implemented on top of long-lived real
    threads. Every time we take a thread out of the pool, we want it to appear
    brand-new. Deleting all ThreadLocals when a real thread is recycled is not
    "extremely dangerous", because at that moment, the *virtual* thread is
    dying, and a new one is being created; that makes it an entirely
    appropriate thing to do.

    It is certainly true that it is "only a crutch for not doing the right
    thing to begin with", but the sad fact is that programmers persist in not
    doing the right thing to begin with, and it is useful for framework
    writers to be able to limit the damage they can cause.

    tom

    --
    Mathematics is the door and the key to the sciences. -- Roger Bacon
    Tom Anderson, May 27, 2012
    #13
  14. Eric Sosman wrote:
    > On 5/27/2012 3:59 PM, Mike Schilling wrote:
    >> "Eric Sosman"<> wrote in message
    >> news:jptvdf$1s5$...
    >>> On 5/27/2012 3:04 PM, Mike Schilling wrote:
    >>>> "Daniel Pitts"<> wrote in
    >>>> message news:8Euwr.47425$...
    >>>>> On 5/27/12 11:00 AM, Mike Schilling wrote:
    >>>>>> "markspace"<-@.> wrote in message
    >>>>>> news:jptkmp$vbg$...
    >>>>>>> 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.


    Note that it makes your objections moot. There's no way to enforce that the
    called code be "correct", and there is no invoker to worry about.
    Mike Schilling, May 28, 2012
    #14
  15. On 05/27/2012 08:14 PM, Mike Schilling wrote:
    > "Robert Klemme"<> wrote in message
    > news:...
    >>> 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
    Robert Klemme, May 28, 2012
    #15
  16. 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
    Robert Klemme, May 28, 2012
    #16
  17. On 05/28/2012 12:27 AM, Tom Anderson wrote:
    > On Sun, 27 May 2012, Eric Sosman wrote:
    >
    >> If you think of anything new to add, pray do so.

    >
    > Mike added this pithy statement of the goal:
    >
    > 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.
    >
    > At the moment, it is *not* possible to achieve that, because
    > ThreadLocals will survive recycling.


    But that is actually a good thing: ThreadLocal's purpose is to attach
    state to a thread which can be accessed without synchronization (if not
    shared). Consider attaching a database connection to a thread. It
    would be disastrous for the connection itself to just clear the object
    reference through some global "remove all" and you would pay a high
    price for opening new connections over and over again. If the database
    connection is supposed to be attached to a thread for only a certain
    amount of time (e.g. a method call) then it must be coded to be properly
    cleaned up; in case of a JDCB connection that means invoking commit() or
    rollback() and then close(). Only after that you can safely clear the
    reference. But this entirely depends on the ThreadLocal at hand and
    needs to be done specifically for individual ThreadLocals - it cannot be
    done globally.

    > One way of looking at a thread pool is that it provides a supply of
    > fresh short-lived virtual threads, implemented on top of long-lived real
    > threads. Every time we take a thread out of the pool, we want it to
    > appear brand-new. Deleting all ThreadLocals when a real thread is
    > recycled is not "extremely dangerous", because at that moment, the
    > *virtual* thread is dying, and a new one is being created; that makes it
    > an entirely appropriate thing to do.


    It *is* dangerous as I have explained above. The observable state of
    each of these virtual threads should of course be identical but that
    does not mean that the technical state must be identical (that's
    basically what caching is all about). For example, in case of the DB
    connection you might open the connection in initialValue() or some other
    logic so the code using the ThreadLocal always sees a properly
    initialized connection.

    > It is certainly true that it is "only a crutch for not doing the right
    > thing to begin with", but the sad fact is that programmers persist in
    > not doing the right thing to begin with, and it is useful for framework
    > writers to be able to limit the damage they can cause.


    Actually the global remove would cause more damage than it prevents.
    You cannot know about all the other ThreadLocals used in code and a
    framework writer who attempts to just clear state of other modules
    without even knowing them should be tarred and feathered.

    Cheers

    robert
    Robert Klemme, May 28, 2012
    #17
  18. Mike Schilling

    markspace Guest

    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.
    markspace, May 28, 2012
    #18
  19. 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;
    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
    Robert Klemme, May 28, 2012
    #19
  20. Mike Schilling

    markspace Guest

    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.
    markspace, May 28, 2012
    #20
    1. Advertising

Want to reply to this thread or ask your own question?

It takes just 2 minutes to sign up (and it's free!). Just click the sign up button to choose a username and then you can ask your own questions on the forum.
Similar Threads
  1. Michael Attenborough
    Replies:
    22
    Views:
    2,255
    Mike Treseler
    Mar 13, 2006
  2. Mike Meyer

    Proposed new collection methods

    Mike Meyer, Aug 6, 2005, in forum: Python
    Replies:
    9
    Views:
    252
    Terry Reedy
    Aug 8, 2005
  3. James J. Besemer

    Proposed new PEP: print to expand generators

    James J. Besemer, Jun 4, 2006, in forum: Python
    Replies:
    3
    Views:
    287
    Bruno Desthuilliers
    Jun 5, 2006
  4. Scott M.

    Vote on proposed VS feature.

    Scott M., Jan 18, 2008, in forum: ASP .Net
    Replies:
    4
    Views:
    340
    Scott M.
    Jan 20, 2008
  5. Replies:
    0
    Views:
    376
Loading...

Share This Page