WICKET-2846 - Store Application in InheritableThreadLocal instead of ThreadLocal

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

WICKET-2846 - Store Application in InheritableThreadLocal instead of ThreadLocal

asfernandes
Hi!

I see that on the to be released 1.4.9 and worried. Was its consequences
really thought about?

I do think this can introduce memory leaks, when some Java code creates
threads (like Java 2D code when dealing with images) that never dies.

Or what am I missing?


Adriano

Reply | Threaded
Open this post in threaded view
|

Re: WICKET-2846 - Store Application in InheritableThreadLocal instead of ThreadLocal

Alex Objelean
I still don't see why using InheritableThreadLocal would introduce memory leaks when dealing with threads. Do you have a good example to prove it? I don't see any difference...
Reply | Threaded
Open this post in threaded view
|

Re: WICKET-2846 - Store Application in InheritableThreadLocal instead of ThreadLocal

asfernandes
On 19/05/2010 09:50, Alex Objelean wrote:
> I still don't see why using InheritableThreadLocal would introduce memory
> leaks when dealing with threads. Do you have a good example to prove it? I
> don't see any difference...
>    
The application instance would go to arbitrary (like ones possible
created by Java core library) and would never be garbaged collected.


Adriano

Reply | Threaded
Open this post in threaded view
|

Re: WICKET-2846 - Store Application in InheritableThreadLocal instead of ThreadLocal

James Carman
Why would the application object itself need to be garbage collected?

On Wed, May 19, 2010 at 8:53 AM, Adriano dos Santos Fernandes
<[hidden email]> wrote:

> On 19/05/2010 09:50, Alex Objelean wrote:
>>
>> I still don't see why using InheritableThreadLocal would introduce memory
>> leaks when dealing with threads. Do you have a good example to prove it? I
>> don't see any difference...
>>
>
> The application instance would go to arbitrary (like ones possible created
> by Java core library) and would never be garbaged collected.
>
>
> Adriano
>
>
Reply | Threaded
Open this post in threaded view
|

Re: WICKET-2846 - Store Application in InheritableThreadLocal instead of ThreadLocal

asfernandes
On 19/05/2010 10:57, James Carman wrote:
> Why would the application object itself need to be garbage collected?
>    
To hot-deployment not eat your memory.


Adriano

PS: I'm much more worried in production environments. Restarting the
container because you need to update the application is something I
consider an awful practice.

Reply | Threaded
Open this post in threaded view
|

Re: WICKET-2846 - Store Application in InheritableThreadLocal instead of ThreadLocal

Alex Objelean
In reply to this post by asfernandes
Since you've never needed the Application instance in threads you had created, why would you need it from now on? The only problem related to memory leaks may be caused by your custom implementation, not with the fact of using InheritableThreadLocal.
Reply | Threaded
Open this post in threaded view
|

Re: WICKET-2846 - Store Application in InheritableThreadLocal instead of ThreadLocal

James Carman
In reply to this post by asfernandes
Use JRebel!  Problem solved. :)

On Wed, May 19, 2010 at 10:03 AM, Adriano dos Santos Fernandes
<[hidden email]> wrote:

> On 19/05/2010 10:57, James Carman wrote:
>>
>> Why would the application object itself need to be garbage collected?
>>
>
> To hot-deployment not eat your memory.
>
>
> Adriano
>
> PS: I'm much more worried in production environments. Restarting the
> container because you need to update the application is something I consider
> an awful practice.
>
>
Reply | Threaded
Open this post in threaded view
|

Re: WICKET-2846 - Store Application in InheritableThreadLocal instead of ThreadLocal

asfernandes
In reply to this post by Alex Objelean
On 19/05/2010 12:48, Alex Objelean wrote:
> Since you've never needed the Application instance in threads you had
> created, why would you need it from now on? The only problem related to
> memory leaks may be caused by your custom implementation, not with the fact
> of using InheritableThreadLocal.
>    
* This class extends <tt>ThreadLocal</tt> to provide inheritance of values
  * from parent thread to child thread: when a child thread is created, the
  * child receives initial values for all inheritable thread-local variables
  * for which the parent has values.  Normally the child's values will be
  * identical to the parent's; however, the child's value can be made an
  * arbitrary function of the parent's by overriding the <tt>childValue</tt>
  * method in this class.

As soon a child thread is created, the application will become a "root"
of it and will never be collected, even if you not use it.


Adriano

Reply | Threaded
Open this post in threaded view
|

Re: WICKET-2846 - Store Application in InheritableThreadLocal instead of ThreadLocal

asfernandes
In reply to this post by James Carman
On 19/05/2010 12:50, James Carman wrote:
> Use JRebel!  Problem solved. :)
>    
I suggest to not change something in a minor release that breaks things
that is working. I also suggest this shouldn't be done by default in
major releases as well.

I see no way JRebel or any tool going to remove thread locals knowing
what its being done.


Adriano

Reply | Threaded
Open this post in threaded view
|

Re: WICKET-2846 - Store Application in InheritableThreadLocal instead of ThreadLocal

Alex Objelean
In reply to this post by asfernandes
Please, correct me if I'm wrong, but the Application won't become the root of child threads. Using InheritableThreadLocal will only make Application available from the threads created during a request cycle. There is absolutely no memory related problem with it.

Alex
Reply | Threaded
Open this post in threaded view
|

Re: WICKET-2846 - Store Application in InheritableThreadLocal instead of ThreadLocal

asfernandes
On 19/05/2010 13:06, Alex Objelean wrote:
> Please, correct me if I'm wrong, but the Application won't become the root of
> child threads. Using InheritableThreadLocal will only make Application
> available from the threads created during a request cycle. There is
> absolutely no memory related problem with it.
>    
As I said, some operations may spawn threads, like manipulation images,
for example.

If your application call it (during a request cycle), the application
object will be stored in a system thread.

Take a look at this:
http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6489540

This currently make web-classloader leaks. If you start using
InheritableThreadLocal's with arbitrary objects, you're going to make
even more leaks.

Also note, there is something not good here. AFAIK, Wicket sets the
thread locals only during the request. But if child threads are spawned,
they can't be cleaned automatically. IMO, it should be done something
that the user needs to call to set/clear this, like a specialized
WicketThread class.


Adriano

Reply | Threaded
Open this post in threaded view
|

Re: WICKET-2846 - Store Application in InheritableThreadLocal instead of ThreadLocal

James Carman
In reply to this post by asfernandes
On Wed, May 19, 2010 at 11:56 AM, Adriano dos Santos Fernandes
<[hidden email]> wrote:
>
> I suggest to not change something in a minor release that breaks things that
> is working. I also suggest this shouldn't be done by default in major
> releases as well.
>
> I see no way JRebel or any tool going to remove thread locals knowing what
> its being done.

JRebel won't require totally blowing away your application object.
It'll just swap out the underlying logic behind it when you restart
your application.
Reply | Threaded
Open this post in threaded view
|

Re: WICKET-2846 - Store Application in InheritableThreadLocal instead of ThreadLocal

asfernandes
On 19/05/2010 14:35, James Carman wrote:

> On Wed, May 19, 2010 at 11:56 AM, Adriano dos Santos Fernandes
> <[hidden email]>  wrote:
>    
>> I suggest to not change something in a minor release that breaks things that
>> is working. I also suggest this shouldn't be done by default in major
>> releases as well.
>>
>> I see no way JRebel or any tool going to remove thread locals knowing what
>> its being done.
>>      
> JRebel won't require totally blowing away your application object.
> It'll just swap out the underlying logic behind it when you restart
> your application.
>    
AFAIU, JRebel has nothing to do with the problem I referred, sorry. I've
no problem with redeploy, at least, not caused by Wicket so far, and it
would be very desirable that Wicket doesn't go this way.


Adriano

Reply | Threaded
Open this post in threaded view
|

Re: WICKET-2846 - Store Application in InheritableThreadLocal instead of ThreadLocal

tetsuo
In reply to this post by James Carman
JRebel is intended to be used in development, not in production. In
production, you want to undeploy and redeploy your application and,
hopefully, leave no old ClassLoader reference behind.

I'm not sure if InheritableThreadLocal will create memory leaks, but I know
it is something to be very carefully considered.



On Wed, May 19, 2010 at 2:35 PM, James Carman <[hidden email]>wrote:

> On Wed, May 19, 2010 at 11:56 AM, Adriano dos Santos Fernandes
> <[hidden email]> wrote:
> >
> > I suggest to not change something in a minor release that breaks things
> that
> > is working. I also suggest this shouldn't be done by default in major
> > releases as well.
> >
> > I see no way JRebel or any tool going to remove thread locals knowing
> what
> > its being done.
>
> JRebel won't require totally blowing away your application object.
> It'll just swap out the underlying logic behind it when you restart
> your application.
>
Reply | Threaded
Open this post in threaded view
|

Re: WICKET-2846 - Store Application in InheritableThreadLocal instead of ThreadLocal

tetsuo
In reply to this post by asfernandes
If I understand it correctly, the InheritedThreadLocal (ITL) wil hold the
Application reference only if its corresponding thread stays alive. If the
thread dies, the ITL value is eligible for gc, right?

Well, if your application creates a new thread, that is kept alive even
after the webapp is undeployed, the leak is the living thread, not the ITL.
This thread will probably also make some reference to application classes
(if not, why was it created in the first place?), which will cause the same
memory leak.

And, if the thread is taken from a pool managed by some lightweight
container, e.g. Spring, the ITL will not inherit the Application value,
since the 'working' thread is not a child of the request thread.

Well, maybe there is some corner case, for example, if the thread pool
implementation creates new threads on demand, and make them children of the
requesting threads and thus, inheriting the Application instance (this
should probably be considered a bug of the pool implementation). Even then,
it will only be a problem if the pool is managed by something outside the
application, because its threads must survive the web app undeployment to
cause a leak.

Well, I tried to think of some problematic case (since I was worried about
this, too), but couldn't. Do you have any case with potential to trigger the
leak?

Tetsuo


On Wed, May 19, 2010 at 1:15 PM, Adriano dos Santos Fernandes <
[hidden email]> wrote:

> On 19/05/2010 13:06, Alex Objelean wrote:
>
>> Please, correct me if I'm wrong, but the Application won't become the root
>> of
>> child threads. Using InheritableThreadLocal will only make Application
>> available from the threads created during a request cycle. There is
>> absolutely no memory related problem with it.
>>
>>
> As I said, some operations may spawn threads, like manipulation images, for
> example.
>
> If your application call it (during a request cycle), the application
> object will be stored in a system thread.
>
> Take a look at this:
> http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6489540
>
> This currently make web-classloader leaks. If you start using
> InheritableThreadLocal's with arbitrary objects, you're going to make even
> more leaks.
>
> Also note, there is something not good here. AFAIK, Wicket sets the thread
> locals only during the request. But if child threads are spawned, they can't
> be cleaned automatically. IMO, it should be done something that the user
> needs to call to set/clear this, like a specialized WicketThread class.
>
>
> Adriano
>
>
Reply | Threaded
Open this post in threaded view
|

Re: WICKET-2846 - Store Application in InheritableThreadLocal instead of ThreadLocal

Johan Compagner
In reply to this post by asfernandes

----- Original message -----

> On 19/05/2010 13:06, Alex Objelean wrote:
> This currently make web-classloader leaks. If you start using
> InheritableThreadLocal's with arbitrary objects, you're going to make
> even more leaks.
>
> Also note, there is something not good here. AFAIK, Wicket sets the
> thread locals only during the request. But if child threads are spawned,
> they can't be cleaned automatically. IMO, it should be done something
> that the user needs to call to set/clear this, like a specialized
> WicketThread class.

And when should that one clean up the threadlocal??
What would be a good time to clean it up?

There would only be 1 place thats when then run method is finished. But if thats the case the thread and the threadlocals are cleared any way.

if you would use a thread pool then you have to use otherways any way to se the Application threadlocal. Thread pools have call backs when a runnable is being executed and finished.

But threads that are created in a request should finish and terminate at one point and never be reused.
Reply | Threaded
Open this post in threaded view
|

Re: WICKET-2846 - Store Application in InheritableThreadLocal instead of ThreadLocal

asfernandes
On 19/05/2010 16:16, Johan Compagner wrote:

>
>
> ----- Original message -----
> > On 19/05/2010 13:06, Alex Objelean wrote:
> > This currently make web-classloader leaks. If you start using
> > InheritableThreadLocal's with arbitrary objects, you're going to make
> > even more leaks.
> >
> > Also note, there is something not good here. AFAIK, Wicket sets the
> > thread locals only during the request. But if child threads are
> spawned,
> > they can't be cleaned automatically. IMO, it should be done something
> > that the user needs to call to set/clear this, like a specialized
> > WicketThread class.
>
> And when should that one clean up the threadlocal??
> What would be a good time to clean it up?
>
That the user decides. Wicket could allow it to set/clear the TLS
application.

I do not think the application object is general enough to be passed
implicitly to threads.

BTW, can't a web application have more than one Wicket filters and then
more than one application object? In this case, threads shared by the
web application would have "arbitrary" application objects.

> But threads that are created in a request should finish and terminate
> at one point and never be reused.
>
I already shown Java 1.4 bug. They seems not interested to change it, so
you deal with it, you say "Java is bad", or you're part of the "restart
is ok" people.


Adriano

Reply | Threaded
Open this post in threaded view
|

Re: WICKET-2846 - Store Application in InheritableThreadLocal instead of ThreadLocal

James Carman
What itch are we trying to scratch, here, anyway?  When do folks need
access to the Application object outside of a request thread?  What is
the usecase?

On Wed, May 19, 2010 at 3:30 PM, Adriano dos Santos Fernandes
<[hidden email]> wrote:

> On 19/05/2010 16:16, Johan Compagner wrote:
>>
>>
>> ----- Original message -----
>> > On 19/05/2010 13:06, Alex Objelean wrote:
>> > This currently make web-classloader leaks. If you start using
>> > InheritableThreadLocal's with arbitrary objects, you're going to make
>> > even more leaks.
>> >
>> > Also note, there is something not good here. AFAIK, Wicket sets the
>> > thread locals only during the request. But if child threads are spawned,
>> > they can't be cleaned automatically. IMO, it should be done something
>> > that the user needs to call to set/clear this, like a specialized
>> > WicketThread class.
>>
>> And when should that one clean up the threadlocal??
>> What would be a good time to clean it up?
>>
> That the user decides. Wicket could allow it to set/clear the TLS
> application.
>
> I do not think the application object is general enough to be passed
> implicitly to threads.
>
> BTW, can't a web application have more than one Wicket filters and then more
> than one application object? In this case, threads shared by the web
> application would have "arbitrary" application objects.
>
>> But threads that are created in a request should finish and terminate at
>> one point and never be reused.
>>
> I already shown Java 1.4 bug. They seems not interested to change it, so you
> deal with it, you say "Java is bad", or you're part of the "restart is ok"
> people.
>
>
> Adriano
>
>
Reply | Threaded
Open this post in threaded view
|

Re: WICKET-2846 - Store Application in InheritableThreadLocal instead of ThreadLocal

asfernandes
On 19/05/2010 16:36, James Carman wrote:
> What itch are we trying to scratch, here, anyway?  When do folks need
> access to the Application object outside of a request thread?  What is
> the usecase?
>    
I have a piece of code that runs in a timer, and renders a page to a
string to be sent via email. So I do need the app object to make a
session. I do catch it from a static field.

The code (that is probably inspired in something in the wiki) is not
very clear, needing mock objects. I do think Wicket could have a way to
simplify it, but not introducing leaks that's know to happen with Java
system classes way of work, as I shown.


Adriano

Reply | Threaded
Open this post in threaded view
|

Re: WICKET-2846 - Store Application in InheritableThreadLocal instead of ThreadLocal

tetsuo
In reply to this post by asfernandes
On Wed, May 19, 2010 at 4:30 PM, Adriano dos Santos Fernandes <
[hidden email]> wrote:

> I do not think the application object is general enough to be passed
> implicitly to threads.
>

Now, this is a valid argument!

I agree wholeheartedly that Application.get() shouldn't be accessible to
child threads by default. But, it's just my opinion about how to design
applications and deal with thread issues, not some fundamental flaw.

Anyway, the InheritableThreadLocal doesn't seem to cause any memory leak
issue that wouldn't already occur due threads running amok. If the
maintainers don't see a problem with it, I think this is an non-issue.
123