Threadlocals and memory leaks in J2EE

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

Re: Threadlocals and memory leaks in J2EE

Doug Lea

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

No, I mean something like...
   ThreadLocal<Context> localContext = new ThreadLocal<Context>();
   static Context global;
   void f() {
      global = localContext.get(); // leaks per-thread into static
   }

Which hopefully no one ever does. However,
if this were strictly illegal, there is a chance
that underlying GC and other JVM support could be made
noticeably faster, knowing that thread-locals never escape
their threads.

-Doug

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

Re: Threadlocals and memory leaks in J2EE

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

On 10/10/07, Endre Stølsvik <[hidden email]> wrote

> This is wrong.  The 1.2 implementation (which I wrote) was simple, slow,
> massively oversynchronized, and did have the potential for retaining
> wholly unused graphs of objects.

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

 
This is not clear.  We have many years of experience with the present implementation.  If we had as many years experience with the previous implementation, we'd likely find the problem equally prevalent.  That said, the previous implementation wasn't even a contender.  It turns thread-local accesses into concurrency hot spots!  That defeats the whole purpose of the facility.  The 1.2 implementation really was a stopgap.  It's sole advantage was that I could write it without modifying the Thread implementation.  As soon as I had time to replace it with a plausible implementation, I did.  And Doug further improved on that implementation, resulting in a mechanism where a thread-local access requires only a handful of machine instructions.  That is as it should be.
 

 

This is not Tomcat's problem per se. It isn't Tomcat that isn't cleaning
up after itself. It is the webapp's code.
 
From the proposed solution "make a brand new thread pool when any webapp is reloaded" I assumed that it was the web app framework that was at fault, and I assumed that the framework was part of Tomcat.  I have no familiarity with this stack.  At any rate, the fault lies with whomever is creating the thread pool.  If you provide a thread pool, you'd better make clear your expectations regarding thread locals.  If you have any doubts as to your users' abilities to abide by those expectations, you'd better replace the thread pool at appropriate times.  As I said previously, you can't draw threads from a pool and expect them to be functionally equivalent to new threads.  It just doesn't work that way.  A thread pool is a compromise: you save the cost of thread creation, and wind up with dirty threads.
 
And also, what you and others actually state is that to use ThreadLocals
in your own code, you must handle them just as preciously as one does
e.g. SQL Connections, with try-finally:close and all that.
 
This really has little to do with thread locals.  If you have any resource that goes away when you exit a scope, you have to clean up when you exit that scope, and that's what try-finally blocks are for.  You have to use them to close files, too, or you'll leak file descriptors and your program may crash.  Thread locals are a red herring.

 
  Simply given that Servlets are a very common usage of Java, and that
Tomcat is a very common Servlet Container, one can deduce that actually
rather many developers are affected by this problem to some degree,
merely through Tomcat.  (Of course, as mentioned, Tomcat could easily fix the particular
problem by making a new Thread pool, ditching all the old threads.)
 
I think you're admitting that this is Tomcat's problem.
 

 

Did you read the links?

Thomas Hawtin argues that his solution don't impose much of a penalty
 
Yes, I skimmed them.  I think Tom is wrong.  Doug concurs, and I trust Doug in these matters.


Why don't you at least sit down, check this through, and give a critique
of this code?
 
Doug has already done that, and he's more qualified to do it than I am.  But do note that we are sympathetic to this issue, even though we see it as an abuse of thread locals and thread pools.  If it is possible to rectify the problem without harming the performance of well behaved applications, I strongly suspect that we'll do it (where "we" probably amounts to "Doug").
 
          Regards,
 
          Josh
 

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

Re: Threadlocals and memory leaks in J2EE

Peter Jones-9
In reply to this post by Doug Lea
On Wed, Oct 10, 2007 at 07:50:50AM -0400, Doug Lea wrote:
>
> 2. Last I left an exchange with Tom Hawtins about this last year,
> it had occurred to me that some improvements to java.lang.ref
> classes could lead to way to handle these cases that would have no
> net performance loss, and maybe even an improvement.
> But I haven't had a chance to pursue this.
> I will try to somehow push up the priority of exploring this.

If I understand correctly from skimming this thread, the "leak"
problem described for the current ThreadLocal implementation is a
variant of the general limitation underlying these RFEs:

    http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=4429536
    http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6389107

which I believe could be solved with a java.lang.ref enhancement more
or less along these lines:

    http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=4630118

I have longed for such a solution to this general limitation countless
times, not when using thread locals but for a variety of data
structures involving reference objects-- but the above RFEs have not
appeared to accumulate much interest, at least judging by SDN votes
and comments (lack thereof).  Adding any additional complexity to such
a fundamental VM/GC interface cannot be undertaken lightly of course,
but it would be delightful if the apparent interest in this
ThreadLocal case led to a more general solution.

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

Re: Threadlocals and memory leaks in J2EE

Unmesh joshi
In reply to this post by Unmesh joshi
Hi,
 
I just came across
http://chasethedevil.blogspot.com/2007/08/original-pattern-servletrequest-in.html
It looks like keeping request context in threadlocals is fairly common practice. GWT and Axis framework are using it.
In the "Context Object" pattern by Douglas Schmidt, passing context implicitly using thread local storage is mentioned as one of the arternatives.(http://www.cs.wustl.edu/~schmidt/PDF/Context-Object-Pattern.pdf)
For a person coming from this background or refering to these resources, it is very obvious to use Context Object pattern and keep it in thread local storage for propogating it across logical layers on the code.
 
Following is the code that I think very reasonable and helpful,
The example code might be like
RequestContextHolder {
 private static ThreadLocal threadLocal = new ThreadLocal();
 public static Map get() {
       return (Map)threadLocal.get();
 }

 public static void set(Map context) {
         threadLocal.set(context);
 }

 public static void clearContext() {
      threadLocal.remove();
 }

}


class FrontController {


   public void handleReqeuest(HttpRequst request.....) {

              Map requestContext = buildRequestContext(request);
              RequestContextHolder.set(requestContext);
               .....
              Response response = handleRequest(request);
              processResponse(response);

              RequestContextHolder.clearContext();

   }

}

Thanks,
Unmesh
 
> ----------------------------------------------------------------------

>
> Message: 1
> Date: Tue, 9 Oct 2007 23:43:41 -0700
> From: "Joshua Bloch" <[hidden email]>
> Subject: Re: [concurrency-interest] Threadlocals and memory leaks in
> J2EE
> To: " Endre St?lsvik " <[hidden email]>
> Cc: Joe Bowbeer <[hidden email]>, concurrency-interest
> <[hidden email]>
> Message-ID:
> <[hidden email]>
> Content-Type: text/plain; charset="iso-8859-1"
>
> Endre,
>
> On 10/9/07, Endre St?lsvik <[hidden email]> wrote:
>
> > The 1.2 implementation was simple and slow, but didn't have this problem
> > of retaining wholly unused graphs of objects.
>
>
> This is wrong. The 1.2 implementation (which I wrote) was simple, slow,
> massively oversynchronized, and *did* have the potential for retaining
> wholly unused graphs of objects. In this implementation, each ThreadLocal
> had a WeakHashMap from Thread to value. As Bob Lee said
> previously "[unintentional object retention] could arise if a value
> strongly referenced the Thread."
>
> It seems the problem you describe is primarily a case of thread pool abuse
> by Tomcat. As I'm fond of saying, a thread pool is like a used car lot. If
> you want a new car/thread, you have to pay for it. If you buy a used
> car/thread, it may not be pristine. Apparently Tomcat screwed up in this
> regard. (I'm no Tomcat expert; I'm basing this judgment on the information I
> was able to glean from reading this thread.)
>
> I do think the notion of generalized context-local variables, with explicit
> support for context termination, is reasonable. People have been talking
> about this for some time, and now may be the time to consider it seriously.
> While I agree that it would be nice if the system could GC thread-local
> values in the case you describe (value strongly reference ThreadLocal), it
> is not worth substantially slowing down the facility to rectify this.
> Initially ThreadLocal was written to support a small number of thread-local
> variables, consistent with common practice in the late '90s (e.g.,
> in Pthreads). But Java programmers made very heavy (perhaps too heavy) use
> of ThreadLocal in the intervening years, so we sped it up repeatedly. We
> cannot afford to slow it down. Users do not take kindly to severe
> performance regressions.
>
> Josh


Check out some new online services at Windows Live Ideas—so new they haven’t even been officially released yet. Try it!
_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Threadlocals and memory leaks in J2EE

Bob Lee-8
You should remove the thread local in a finally block. See http://crazybob.org/2006/07/hard-core-java-threadlocal.html for examples.

Bob

On 10/10/07, Unmesh joshi <[hidden email]> wrote:
class FrontController {


   public void handleReqeuest(HttpRequst request.....) {

              Map requestContext = buildRequestContext(request);
              RequestContextHolder.set (requestContext);
               .....
              Response response = handleRequest(request);
              processResponse(response);

              RequestContextHolder.clearContext();

   }

}


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

Re: Threadlocals and memory leaks in J2EE

Unmesh joshi
Yes I should be using try-finally. Also, if I am using JDK1.4.2, where there is no remove method on threadlocal, threadloca.set(null), should also be Ok right?
Also, When I am not keeping value as object of any class in my web module (Keeping java.util.Map and only objects of string or other java.lang type in there), there is also no danger of having any memory leak on reload.
 
Thanks,
Unmesh



Date: Wed, 10 Oct 2007 21:17:17 -0700
From: [hidden email]
To: [hidden email]
Subject: Re: [concurrency-interest] Threadlocals and memory leaks in J2EE
CC: [hidden email]

You should remove the thread local in a finally block. See http://crazybob.org/2006/07/hard-core-java-threadlocal.html for examples.

Bob

On 10/10/07, Unmesh joshi <[hidden email]> wrote:
class FrontController {


   public void handleReqeuest(HttpRequst request.....) {

              Map requestContext = buildRequestContext(request);
              RequestContextHolder.set (requestContext);
               .....
              Response response = handleRequest(request);
              processResponse(response);

              RequestContextHolder.clearContext();

   }

}



Check out some new online services at Windows Live Ideas—so new they haven’t even been officially released yet. Try it!
_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Threadlocals and memory leaks in J2EE

Carfield Yim
In reply to this post by Endre Stølsvik-6
Hi, I am just a developer and not really good at concurrency handling.

The application I currently working on stored all preparedstatement in
a hashmap in order to maximum performance according to
http://download.oracle.com/otn_hosted_doc/timesten/603/TimesTen-Documentation/goodpractices.pdf
.

In order to prevent concurrency issue, we use threadlocal to hold that
map. I am not sure it is a proper usecase for threadlocal. However the
application probably don't hurt by memory problem because it run at a
very powerful machine and will restart everyday and there is not class
reloading.

The question I like to ask is, recently a developer like to using
custom thread scope in spring according to this
http://opensource.atlassian.com/projects/spring/browse/SPR-2581 , I
think this is smart move, but I still like to gather more comment
about using spring thread scope as this link mentioned. Because we
have other place copying similar pattern but not yet move to use
spring, just wonder should we do so.
_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://altair.cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Threadlocals and memory leaks in J2EE

Bob Lee-8
On 10/11/07, Carfield Yim <[hidden email]> wrote:
The application I currently working on stored all preparedstatement in
a hashmap in order to maximum performance according to
http://download.oracle.com/otn_hosted_doc/timesten/603/TimesTen-Documentation/goodpractices.pdf

Do you use a connection pool? From my experience, most connection pools already cache prepared statements for you (as they should). I also thought prepared statements could be tied to a given connection, in which case you would want to make the cache per connection, not per thread.

Bob


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

Re: Threadlocals and memory leaks in J2EE

Larry Riedel
In reply to this post by Carfield Yim

> In order to prevent concurrency issue, we use threadlocal
> to hold that map. [...] recently a developer like to using
> custom thread scope in spring according to this
> http://opensource.atlassian.com/projects/spring/browse/SPR-2581,
> [...] I still like to gather more comment about using spring
> thread scope as this link mentioned.

If I am not going to have more than one Thread handling
a particular Request, or a particular Session, or a
particular Connection, etc, I usually just associate
the object with the Request, Session, Connection, etc,
rather than doing something with ThreadLocal, which to
me seems like a relatively arcane approach (or at least
couples me more to the implementation than I would like).


Larry

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

Re: Threadlocals and memory leaks in J2EE

Hanson Char
In reply to this post by Endre Stølsvik-6
>On 10/9/07, Endre Stølsvik <[hidden email]> wrote:
> ...This results in a leak that no one can fix, since you
> cannot ask the Thread to ditch their references.

Playing devil's advocate, I think you "can" ditch all thread locals
(and hence all the transitive references) of a thread - via
reflection.  Ugly but may work, if the security permits. :)

Hanson Char

    public static void ditchThreadLocals() throws Exception
    {
        Thread t = Thread.currentThread();
        String[] fnames = { "threadLocals", "inheritableThreadLocals" };

        for (String fname : fnames)
        {
            Field fThreadLocals = Thread.class.getDeclaredField(fname);
            fThreadLocals.setAccessible(true);
            fThreadLocals.set(t, null);
        }
    }

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