You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@wicket.apache.org by Adriano dos Santos Fernandes <ad...@gmail.com> on 2010/05/19 12:53:06 UTC
WICKET-2846 - Store Application in InheritableThreadLocal instead
of ThreadLocal
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
Re: WICKET-2846 - Store Application in InheritableThreadLocal instead
of ThreadLocal
Posted by Adriano dos Santos Fernandes <ad...@gmail.com>.
On 19/05/2010 14:35, James Carman wrote:
> On Wed, May 19, 2010 at 11:56 AM, Adriano dos Santos Fernandes
> <ad...@gmail.com> 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
Re: WICKET-2846 - Store Application in InheritableThreadLocal instead
of ThreadLocal
Posted by tetsuo <ro...@gmail.com>.
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 <ja...@carmanconsulting.com>wrote:
> On Wed, May 19, 2010 at 11:56 AM, Adriano dos Santos Fernandes
> <ad...@gmail.com> 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.
>
Re: WICKET-2846 - Store Application in InheritableThreadLocal instead
of ThreadLocal
Posted by James Carman <ja...@carmanconsulting.com>.
On Wed, May 19, 2010 at 11:56 AM, Adriano dos Santos Fernandes
<ad...@gmail.com> 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.
Re: WICKET-2846 - Store Application in InheritableThreadLocal instead
of ThreadLocal
Posted by Adriano dos Santos Fernandes <ad...@gmail.com>.
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
Re: WICKET-2846 - Store Application in InheritableThreadLocal instead
of ThreadLocal
Posted by James Carman <ja...@carmanconsulting.com>.
Use JRebel! Problem solved. :)
On Wed, May 19, 2010 at 10:03 AM, Adriano dos Santos Fernandes
<ad...@gmail.com> 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.
>
>
Re: WICKET-2846 - Store Application in InheritableThreadLocal instead
of ThreadLocal
Posted by Adriano dos Santos Fernandes <ad...@gmail.com>.
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.
Re: WICKET-2846 - Store Application in InheritableThreadLocal instead
of ThreadLocal
Posted by James Carman <ja...@carmanconsulting.com>.
Why would the application object itself need to be garbage collected?
On Wed, May 19, 2010 at 8:53 AM, Adriano dos Santos Fernandes
<ad...@gmail.com> 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
>
>
Re: WICKET-2846 - Store Application in InheritableThreadLocal instead
of ThreadLocal
Posted by tetsuo <ro...@gmail.com>.
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 <
adrianosf@gmail.com> 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
>
>
Re: WICKET-2846 - Store Application in InheritableThreadLocal instead
of ThreadLocal
Posted by tetsuo <ro...@gmail.com>.
onClickOrSomethingSimilar() {
final Application app = Application.get();
new Thread(new Runnable() {
void run() {
doSomethingWith(app);
}
}).start();
}
On Wed, May 19, 2010 at 10:29 PM, Jeremy Thomerson <
jeremy@wickettraining.com> wrote:
> It solves this problem, which is specifically why it was requested:
>
> onClickOrSomethingSimilar() {
> new Thread(new Runnable() {
> void run() {
> doSomethingWith(Application.get());
> }
> }).start();
> }
>
> --
> Jeremy Thomerson
> http://www.wickettraining.com
>
>
>
> On Wed, May 19, 2010 at 6:28 PM, James Carman <james@carmanconsulting.com
> >wrote:
>
> > Sure this might work, but then again you wouldn't need the
> > InheritableThreadLocal for this. The question is, does the
> > InheritableThreadLocal really solve anything? Is it really addressing
> > the problem? Or, would you have to do code like this to manage it
> > properly anyway? And, if so, then why implement the
> > InheritableThreadLocal, especially since we've shown that it will fail
> > in more cases than it will work?
> >
> > On Wed, May 19, 2010 at 6:28 PM, Johan Compagner <jc...@gmail.com>
> > wrote:
> > > If you where using threads in your application
> > > Then i would do it this way:
> > >
> > > Your WebApplication class has a method:
> > > getExecutor() that returns a ThreadPoolExecutor
> > >
> > > That threadpoolexecutor (that you extend) overrides 2 methods
> > >
> > > protected void beforeExecute(Thread t, Runnable r) { }
> > >
> > > that sets the thread locals (so the application instance that has the
> > > executor) and
> > >
> > > protected void afterExecute(Runnable r, Throwable t) { }
> > >
> > > to release all thread locals.
> > >
> > > this way you use a pool (way better to control your web application)
> and
> > all
> > > the resources you need are set just before and release right after.
> > >
> > > johanm
> > >
> > >
> > >
> > > On Wed, May 19, 2010 at 23:41, James Carman <
> james@carmanconsulting.com
> > >wrote:
> > >
> > >> Will the inheritance of the application really work correctly? For
> > pooled
> > >> threads that are created at application startup, the threadlocal will
> be
> > >> null, because the parent thread is the thread that starts the
> container.
> > >> For threads that are created within the context of the request thread,
> > they
> > >> will get the current application object, which would be fine if that
> > thread
> > >> executes and finishes. But, for threads that are going to be reused
> > >> (executor threads in a pool), they will see the original application
> > object
> > >> because the value is set at thread creation time. If you have
> multiple
> > >> wicket filters in the same context, that could be incorrect, meaning a
> > >> request thread for a different application submitted a task to be
> > executed.
> > >>
> > >> On May 19, 2010 4:13 PM, "Adriano dos Santos Fernandes" <
> > >> adrianosf@gmail.com>
> > >> wrote:
> > >>
> > >> On 19/05/2010 17:03, Jeremy Thomerson wrote:
> > >> >>
> > >> >>
> > >> >> To clarify this, I use Application.set and App...
> > >> Well, forgetting to unset it would not leak any more than have it
> > >> implicitly
> > >> set like it's going to be. And I do think forgetting this is developer
> > >> fault.
> > >>
> > >> What you all do not want to understand is what I said about Java
> library
> > >> spawning its own threads, and that is not documented, as its for
> cleanup
> > in
> > >> the case I shown.
> > >>
> > >>
> > >> Adriano
> > >>
> > >
> >
>
Re: WICKET-2846 - Store Application in InheritableThreadLocal instead
of ThreadLocal
Posted by James Carman <ja...@carmanconsulting.com>.
It solves that one particular usecase, but I doubt folks would be
starting threads like that. Most folks, if they're smart, would use a
thread pool for something like that. For the pooled thread case, it
doesn't work.
On Wed, May 19, 2010 at 9:29 PM, Jeremy Thomerson
<je...@wickettraining.com> wrote:
> It solves this problem, which is specifically why it was requested:
>
> onClickOrSomethingSimilar() {
> new Thread(new Runnable() {
> void run() {
> doSomethingWith(Application.get());
> }
> }).start();
> }
>
> --
> Jeremy Thomerson
> http://www.wickettraining.com
>
>
>
> On Wed, May 19, 2010 at 6:28 PM, James Carman <ja...@carmanconsulting.com>wrote:
>
>> Sure this might work, but then again you wouldn't need the
>> InheritableThreadLocal for this. The question is, does the
>> InheritableThreadLocal really solve anything? Is it really addressing
>> the problem? Or, would you have to do code like this to manage it
>> properly anyway? And, if so, then why implement the
>> InheritableThreadLocal, especially since we've shown that it will fail
>> in more cases than it will work?
>>
>> On Wed, May 19, 2010 at 6:28 PM, Johan Compagner <jc...@gmail.com>
>> wrote:
>> > If you where using threads in your application
>> > Then i would do it this way:
>> >
>> > Your WebApplication class has a method:
>> > getExecutor() that returns a ThreadPoolExecutor
>> >
>> > That threadpoolexecutor (that you extend) overrides 2 methods
>> >
>> > protected void beforeExecute(Thread t, Runnable r) { }
>> >
>> > that sets the thread locals (so the application instance that has the
>> > executor) and
>> >
>> > protected void afterExecute(Runnable r, Throwable t) { }
>> >
>> > to release all thread locals.
>> >
>> > this way you use a pool (way better to control your web application) and
>> all
>> > the resources you need are set just before and release right after.
>> >
>> > johanm
>> >
>> >
>> >
>> > On Wed, May 19, 2010 at 23:41, James Carman <james@carmanconsulting.com
>> >wrote:
>> >
>> >> Will the inheritance of the application really work correctly? For
>> pooled
>> >> threads that are created at application startup, the threadlocal will be
>> >> null, because the parent thread is the thread that starts the container.
>> >> For threads that are created within the context of the request thread,
>> they
>> >> will get the current application object, which would be fine if that
>> thread
>> >> executes and finishes. But, for threads that are going to be reused
>> >> (executor threads in a pool), they will see the original application
>> object
>> >> because the value is set at thread creation time. If you have multiple
>> >> wicket filters in the same context, that could be incorrect, meaning a
>> >> request thread for a different application submitted a task to be
>> executed.
>> >>
>> >> On May 19, 2010 4:13 PM, "Adriano dos Santos Fernandes" <
>> >> adrianosf@gmail.com>
>> >> wrote:
>> >>
>> >> On 19/05/2010 17:03, Jeremy Thomerson wrote:
>> >> >>
>> >> >>
>> >> >> To clarify this, I use Application.set and App...
>> >> Well, forgetting to unset it would not leak any more than have it
>> >> implicitly
>> >> set like it's going to be. And I do think forgetting this is developer
>> >> fault.
>> >>
>> >> What you all do not want to understand is what I said about Java library
>> >> spawning its own threads, and that is not documented, as its for cleanup
>> in
>> >> the case I shown.
>> >>
>> >>
>> >> Adriano
>> >>
>> >
>>
>
Re: WICKET-2846 - Store Application in InheritableThreadLocal instead
of ThreadLocal
Posted by Juliano Viana <ju...@logicstyle.com>.
Hi everyone,
I know this issue has already been debated and that a decision was made to
revert this change in a future version of Wicket.
However, the discussions about this issue were centered on the fact starting
threads in web applications is not a good idea anyway, and hence this would
not break applications that are not already broken.
I have found a real case where this breaks an innocent application:
redeploying an application based on Wicket 1.4.9 on Glassfish 3.0.1 causes
a memory leak due to the use of InheritableThreadLocal.
The problem is that when the application accesses a JDBC resource for the
first time, Glassfish lazily starts a timer (connector-timer-proxy) that has
an associated thread. This timer is started from the web request processing
thread. This thread never dies, and inherits a reference to the Wicket
Application object.
This only happens on redeployments, but it really hurts development as you
keep having to restart Glassfish due to OOM exceptions.
Removing the InheritableThreadLocal resolves the issue completely and makes
development really smooth again.
So if you are using Wicket 1.4.9 with Glassfish v3 you should consider
patching it until a new Wicket release is out.
Regards,
- Juliano
On Thu, May 20, 2010 at 2:46 PM, tetsuo <ro...@gmail.com> wrote:
> On Thu, May 20, 2010 at 10:29 AM, Johan Compagner <jcompagner@gmail.com
> >wrote:
>
> > But using
> >
> > final Application app = Application.get()
> > // start thread
> >
> > is exactly the same kind of leakage as using InheritableThreadLocal
> >
> >
> Exactly, the bug is not in Wicket, it's in the application, which doesn't
> manage threads correctly.
>
Re: WICKET-2846 - Store Application in InheritableThreadLocal instead
of ThreadLocal
Posted by tetsuo <ro...@gmail.com>.
On Thu, May 20, 2010 at 10:29 AM, Johan Compagner <jc...@gmail.com>wrote:
> But using
>
> final Application app = Application.get()
> // start thread
>
> is exactly the same kind of leakage as using InheritableThreadLocal
>
>
Exactly, the bug is not in Wicket, it's in the application, which doesn't
manage threads correctly.
Re: WICKET-2846 - Store Application in InheritableThreadLocal instead
of ThreadLocal
Posted by Johan Compagner <jc...@gmail.com>.
i still dont see your solution about the wicket thread class.
What should that one do??? give an example.
The best solution is to use a threadpool like a described above.
And yes that InheritableThreadLocal isnt needed then.
But using
final Application app = Application.get()
// start thread
is exactly the same kind of leakage as using InheritableThreadLocal
On Thu, May 20, 2010 at 12:38, Adriano dos Santos Fernandes <
adrianosf@gmail.com> wrote:
> On 19/05/2010 22:29, Jeremy Thomerson wrote:
>
>> It solves this problem, which is specifically why it was requested:
>>
>> onClickOrSomethingSimilar() {
>> new Thread(new Runnable() {
>> void run() {
>> doSomethingWith(Application.get());
>> }
>> }).start();
>> }
>>
>>
> That's what I said about have a WicketThread class, or "publish"
> Application.set/clear for users do this with more safety.
>
>
> Adriano
>
>
Re: WICKET-2846 - Store Application in InheritableThreadLocal instead
of ThreadLocal
Posted by Adriano dos Santos Fernandes <ad...@gmail.com>.
On 19/05/2010 22:29, Jeremy Thomerson wrote:
> It solves this problem, which is specifically why it was requested:
>
> onClickOrSomethingSimilar() {
> new Thread(new Runnable() {
> void run() {
> doSomethingWith(Application.get());
> }
> }).start();
> }
>
That's what I said about have a WicketThread class, or "publish"
Application.set/clear for users do this with more safety.
Adriano
Re: WICKET-2846 - Store Application in InheritableThreadLocal instead
of ThreadLocal
Posted by Jeremy Thomerson <je...@wickettraining.com>.
It solves this problem, which is specifically why it was requested:
onClickOrSomethingSimilar() {
new Thread(new Runnable() {
void run() {
doSomethingWith(Application.get());
}
}).start();
}
--
Jeremy Thomerson
http://www.wickettraining.com
On Wed, May 19, 2010 at 6:28 PM, James Carman <ja...@carmanconsulting.com>wrote:
> Sure this might work, but then again you wouldn't need the
> InheritableThreadLocal for this. The question is, does the
> InheritableThreadLocal really solve anything? Is it really addressing
> the problem? Or, would you have to do code like this to manage it
> properly anyway? And, if so, then why implement the
> InheritableThreadLocal, especially since we've shown that it will fail
> in more cases than it will work?
>
> On Wed, May 19, 2010 at 6:28 PM, Johan Compagner <jc...@gmail.com>
> wrote:
> > If you where using threads in your application
> > Then i would do it this way:
> >
> > Your WebApplication class has a method:
> > getExecutor() that returns a ThreadPoolExecutor
> >
> > That threadpoolexecutor (that you extend) overrides 2 methods
> >
> > protected void beforeExecute(Thread t, Runnable r) { }
> >
> > that sets the thread locals (so the application instance that has the
> > executor) and
> >
> > protected void afterExecute(Runnable r, Throwable t) { }
> >
> > to release all thread locals.
> >
> > this way you use a pool (way better to control your web application) and
> all
> > the resources you need are set just before and release right after.
> >
> > johanm
> >
> >
> >
> > On Wed, May 19, 2010 at 23:41, James Carman <james@carmanconsulting.com
> >wrote:
> >
> >> Will the inheritance of the application really work correctly? For
> pooled
> >> threads that are created at application startup, the threadlocal will be
> >> null, because the parent thread is the thread that starts the container.
> >> For threads that are created within the context of the request thread,
> they
> >> will get the current application object, which would be fine if that
> thread
> >> executes and finishes. But, for threads that are going to be reused
> >> (executor threads in a pool), they will see the original application
> object
> >> because the value is set at thread creation time. If you have multiple
> >> wicket filters in the same context, that could be incorrect, meaning a
> >> request thread for a different application submitted a task to be
> executed.
> >>
> >> On May 19, 2010 4:13 PM, "Adriano dos Santos Fernandes" <
> >> adrianosf@gmail.com>
> >> wrote:
> >>
> >> On 19/05/2010 17:03, Jeremy Thomerson wrote:
> >> >>
> >> >>
> >> >> To clarify this, I use Application.set and App...
> >> Well, forgetting to unset it would not leak any more than have it
> >> implicitly
> >> set like it's going to be. And I do think forgetting this is developer
> >> fault.
> >>
> >> What you all do not want to understand is what I said about Java library
> >> spawning its own threads, and that is not documented, as its for cleanup
> in
> >> the case I shown.
> >>
> >>
> >> Adriano
> >>
> >
>
Re: WICKET-2846 - Store Application in InheritableThreadLocal instead
of ThreadLocal
Posted by James Carman <ja...@carmanconsulting.com>.
Sure this might work, but then again you wouldn't need the
InheritableThreadLocal for this. The question is, does the
InheritableThreadLocal really solve anything? Is it really addressing
the problem? Or, would you have to do code like this to manage it
properly anyway? And, if so, then why implement the
InheritableThreadLocal, especially since we've shown that it will fail
in more cases than it will work?
On Wed, May 19, 2010 at 6:28 PM, Johan Compagner <jc...@gmail.com> wrote:
> If you where using threads in your application
> Then i would do it this way:
>
> Your WebApplication class has a method:
> getExecutor() that returns a ThreadPoolExecutor
>
> That threadpoolexecutor (that you extend) overrides 2 methods
>
> protected void beforeExecute(Thread t, Runnable r) { }
>
> that sets the thread locals (so the application instance that has the
> executor) and
>
> protected void afterExecute(Runnable r, Throwable t) { }
>
> to release all thread locals.
>
> this way you use a pool (way better to control your web application) and all
> the resources you need are set just before and release right after.
>
> johanm
>
>
>
> On Wed, May 19, 2010 at 23:41, James Carman <ja...@carmanconsulting.com>wrote:
>
>> Will the inheritance of the application really work correctly? For pooled
>> threads that are created at application startup, the threadlocal will be
>> null, because the parent thread is the thread that starts the container.
>> For threads that are created within the context of the request thread, they
>> will get the current application object, which would be fine if that thread
>> executes and finishes. But, for threads that are going to be reused
>> (executor threads in a pool), they will see the original application object
>> because the value is set at thread creation time. If you have multiple
>> wicket filters in the same context, that could be incorrect, meaning a
>> request thread for a different application submitted a task to be executed.
>>
>> On May 19, 2010 4:13 PM, "Adriano dos Santos Fernandes" <
>> adrianosf@gmail.com>
>> wrote:
>>
>> On 19/05/2010 17:03, Jeremy Thomerson wrote:
>> >>
>> >>
>> >> To clarify this, I use Application.set and App...
>> Well, forgetting to unset it would not leak any more than have it
>> implicitly
>> set like it's going to be. And I do think forgetting this is developer
>> fault.
>>
>> What you all do not want to understand is what I said about Java library
>> spawning its own threads, and that is not documented, as its for cleanup in
>> the case I shown.
>>
>>
>> Adriano
>>
>
Re: WICKET-2846 - Store Application in InheritableThreadLocal instead
of ThreadLocal
Posted by Johan Compagner <jc...@gmail.com>.
If you where using threads in your application
Then i would do it this way:
Your WebApplication class has a method:
getExecutor() that returns a ThreadPoolExecutor
That threadpoolexecutor (that you extend) overrides 2 methods
protected void beforeExecute(Thread t, Runnable r) { }
that sets the thread locals (so the application instance that has the
executor) and
protected void afterExecute(Runnable r, Throwable t) { }
to release all thread locals.
this way you use a pool (way better to control your web application) and all
the resources you need are set just before and release right after.
johanm
On Wed, May 19, 2010 at 23:41, James Carman <ja...@carmanconsulting.com>wrote:
> Will the inheritance of the application really work correctly? For pooled
> threads that are created at application startup, the threadlocal will be
> null, because the parent thread is the thread that starts the container.
> For threads that are created within the context of the request thread, they
> will get the current application object, which would be fine if that thread
> executes and finishes. But, for threads that are going to be reused
> (executor threads in a pool), they will see the original application object
> because the value is set at thread creation time. If you have multiple
> wicket filters in the same context, that could be incorrect, meaning a
> request thread for a different application submitted a task to be executed.
>
> On May 19, 2010 4:13 PM, "Adriano dos Santos Fernandes" <
> adrianosf@gmail.com>
> wrote:
>
> On 19/05/2010 17:03, Jeremy Thomerson wrote:
> >>
> >>
> >> To clarify this, I use Application.set and App...
> Well, forgetting to unset it would not leak any more than have it
> implicitly
> set like it's going to be. And I do think forgetting this is developer
> fault.
>
> What you all do not want to understand is what I said about Java library
> spawning its own threads, and that is not documented, as its for cleanup in
> the case I shown.
>
>
> Adriano
>
Re: WICKET-2846 - Store Application in InheritableThreadLocal instead
of ThreadLocal
Posted by James Carman <ja...@carmanconsulting.com>.
On Thu, May 20, 2010 at 7:08 AM, Adriano dos Santos Fernandes
<ad...@gmail.com> wrote:
> I just don't get if (and what/why) you agree or disagree with what I've
> said...
>
Apparently I was agreeing with you. I guess I didn't read your post
closely enough. :) NEED MORE COFFEE!
Re: WICKET-2846 - Store Application in InheritableThreadLocal instead
of ThreadLocal
Posted by Adriano dos Santos Fernandes <ad...@gmail.com>.
On 20/05/2010 08:02, James Carman wrote:
> On Thu, May 20, 2010 at 6:59 AM, Adriano dos Santos Fernandes
> <ad...@gmail.com> wrote:
>
>> If its thread pool starts during a request, all of its created child threads
>> will have a reference to the initial application, even when running jobs of
>> others applications.
>>
>>
> That's a big "if" (and usually an incorrect one).
...
> Even "if" you start
> the threads in the pool during one particular request cycle, what
> happens when someone else submits a task to be run by the executor?
> It will also use this original application, but it may not necessarily
> be the application they're interested in. You can have multiple
> WicketFilter's (and thus applications) in the same webapp.
>
I just don't get if (and what/why) you agree or disagree with what I've
said...
Adriano
Re: WICKET-2846 - Store Application in InheritableThreadLocal instead
of ThreadLocal
Posted by James Carman <ja...@carmanconsulting.com>.
On Thu, May 20, 2010 at 6:59 AM, Adriano dos Santos Fernandes
<ad...@gmail.com> wrote:
> If its thread pool starts during a request, all of its created child threads
> will have a reference to the initial application, even when running jobs of
> others applications.
>
That's a big "if" (and usually an incorrect one). Even "if" you start
the threads in the pool during one particular request cycle, what
happens when someone else submits a task to be run by the executor?
It will also use this original application, but it may not necessarily
be the application they're interested in. You can have multiple
WicketFilter's (and thus applications) in the same webapp.
Re: WICKET-2846 - Store Application in InheritableThreadLocal instead
of ThreadLocal
Posted by Adriano dos Santos Fernandes <ad...@gmail.com>.
On 19/05/2010 18:46, Martijn Dashorst wrote:
> I wondered about this too: would this work with a job framework like
> Quartz? The thread is not started in a wicket context, but by the
> thing that quartz is managing. Therefore the inherited thing would not
> work and the Application would not be set.
>
I do not know Quartz, but I suppose a lot of problem can arrive if the
library is shared by all applications.
If its thread pool starts during a request, all of its created child
threads will have a reference to the initial application, even when
running jobs of others applications.
Adriano
Re: WICKET-2846 - Store Application in InheritableThreadLocal instead
of ThreadLocal
Posted by Martijn Dashorst <ma...@gmail.com>.
I wondered about this too: would this work with a job framework like
Quartz? The thread is not started in a wicket context, but by the
thing that quartz is managing. Therefore the inherited thing would not
work and the Application would not be set.
Martijn
On Wed, May 19, 2010 at 11:41 PM, James Carman
<ja...@carmanconsulting.com> wrote:
> Will the inheritance of the application really work correctly? For pooled
> threads that are created at application startup, the threadlocal will be
> null, because the parent thread is the thread that starts the container.
> For threads that are created within the context of the request thread, they
> will get the current application object, which would be fine if that thread
> executes and finishes. But, for threads that are going to be reused
> (executor threads in a pool), they will see the original application object
> because the value is set at thread creation time. If you have multiple
> wicket filters in the same context, that could be incorrect, meaning a
> request thread for a different application submitted a task to be executed.
>
> On May 19, 2010 4:13 PM, "Adriano dos Santos Fernandes" <ad...@gmail.com>
> wrote:
>
> On 19/05/2010 17:03, Jeremy Thomerson wrote:
>>>
>>>
>>> To clarify this, I use Application.set and App...
> Well, forgetting to unset it would not leak any more than have it implicitly
> set like it's going to be. And I do think forgetting this is developer
> fault.
>
> What you all do not want to understand is what I said about Java library
> spawning its own threads, and that is not documented, as its for cleanup in
> the case I shown.
>
>
> Adriano
>
--
Become a Wicket expert, learn from the best: http://wicketinaction.com
Apache Wicket 1.4 increases type safety for web applications
Get it now: http://www.apache.org/dyn/closer.cgi/wicket/1.4.8
Re: WICKET-2846 - Store Application in InheritableThreadLocal instead
of ThreadLocal
Posted by Igor Vaynberg <ig...@gmail.com>.
thinking about this more, in 1.5 these short-lived threads will not
even be needed. most often you need to start a new thread so you have
a properly configured wicket environment which depends on
threadlocals. this environment is usually configured by wicket tester
which sets up mock session, request cycle threadlocal. in 1.5 you can
simply "push" wicket's threadcontext and replace it with a separate
temporary one and when you are done simply pop it off. in 1.4 because
all these threadlocals are seperate it is massier to manage them, thus
its simply easier to do the work in another thread.
-igor
On Wed, May 19, 2010 at 2:52 PM, Igor Vaynberg <ig...@gmail.com> wrote:
> why would a request thread start your executor pool? the pool would
> most likely be started from a context listener which uses a separate
> thread. the usecase for the inheritable is for short-lived threads
> started from a request thread.
>
> -igor
>
>
> On Wed, May 19, 2010 at 2:41 PM, James Carman
> <ja...@carmanconsulting.com> wrote:
>> Will the inheritance of the application really work correctly? For pooled
>> threads that are created at application startup, the threadlocal will be
>> null, because the parent thread is the thread that starts the container.
>> For threads that are created within the context of the request thread, they
>> will get the current application object, which would be fine if that thread
>> executes and finishes. But, for threads that are going to be reused
>> (executor threads in a pool), they will see the original application object
>> because the value is set at thread creation time. If you have multiple
>> wicket filters in the same context, that could be incorrect, meaning a
>> request thread for a different application submitted a task to be executed.
>>
>> On May 19, 2010 4:13 PM, "Adriano dos Santos Fernandes" <ad...@gmail.com>
>> wrote:
>>
>> On 19/05/2010 17:03, Jeremy Thomerson wrote:
>>>>
>>>>
>>>> To clarify this, I use Application.set and App...
>> Well, forgetting to unset it would not leak any more than have it implicitly
>> set like it's going to be. And I do think forgetting this is developer
>> fault.
>>
>> What you all do not want to understand is what I said about Java library
>> spawning its own threads, and that is not documented, as its for cleanup in
>> the case I shown.
>>
>>
>> Adriano
>>
>
Re: WICKET-2846 - Store Application in InheritableThreadLocal instead
of ThreadLocal
Posted by Igor Vaynberg <ig...@gmail.com>.
why would a request thread start your executor pool? the pool would
most likely be started from a context listener which uses a separate
thread. the usecase for the inheritable is for short-lived threads
started from a request thread.
-igor
On Wed, May 19, 2010 at 2:41 PM, James Carman
<ja...@carmanconsulting.com> wrote:
> Will the inheritance of the application really work correctly? For pooled
> threads that are created at application startup, the threadlocal will be
> null, because the parent thread is the thread that starts the container.
> For threads that are created within the context of the request thread, they
> will get the current application object, which would be fine if that thread
> executes and finishes. But, for threads that are going to be reused
> (executor threads in a pool), they will see the original application object
> because the value is set at thread creation time. If you have multiple
> wicket filters in the same context, that could be incorrect, meaning a
> request thread for a different application submitted a task to be executed.
>
> On May 19, 2010 4:13 PM, "Adriano dos Santos Fernandes" <ad...@gmail.com>
> wrote:
>
> On 19/05/2010 17:03, Jeremy Thomerson wrote:
>>>
>>>
>>> To clarify this, I use Application.set and App...
> Well, forgetting to unset it would not leak any more than have it implicitly
> set like it's going to be. And I do think forgetting this is developer
> fault.
>
> What you all do not want to understand is what I said about Java library
> spawning its own threads, and that is not documented, as its for cleanup in
> the case I shown.
>
>
> Adriano
>
Re: WICKET-2846 - Store Application in InheritableThreadLocal instead
of ThreadLocal
Posted by James Carman <ja...@carmanconsulting.com>.
Will the inheritance of the application really work correctly? For pooled
threads that are created at application startup, the threadlocal will be
null, because the parent thread is the thread that starts the container.
For threads that are created within the context of the request thread, they
will get the current application object, which would be fine if that thread
executes and finishes. But, for threads that are going to be reused
(executor threads in a pool), they will see the original application object
because the value is set at thread creation time. If you have multiple
wicket filters in the same context, that could be incorrect, meaning a
request thread for a different application submitted a task to be executed.
On May 19, 2010 4:13 PM, "Adriano dos Santos Fernandes" <ad...@gmail.com>
wrote:
On 19/05/2010 17:03, Jeremy Thomerson wrote:
>>
>>
>> To clarify this, I use Application.set and App...
Well, forgetting to unset it would not leak any more than have it implicitly
set like it's going to be. And I do think forgetting this is developer
fault.
What you all do not want to understand is what I said about Java library
spawning its own threads, and that is not documented, as its for cleanup in
the case I shown.
Adriano
Re: WICKET-2846 - Store Application in InheritableThreadLocal
instead of ThreadLocal
Posted by Michael Mosmann <mi...@mosmann.de>.
Am Mittwoch, den 19.05.2010, 16:48 -0300 schrieb Adriano dos Santos
Fernandes:
> 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.
I use the BaseWicketTester-Class for this kind of stuff..
http://www.wicket-praxis.de/blog/2009/12/01/sending-html-email-from-wicket-app/
mm:)
Re: WICKET-2846 - Store Application in InheritableThreadLocal instead
of ThreadLocal
Posted by James Carman <ja...@carmanconsulting.com>.
So, your "itch" or usecase is that you're trying to use Wicket in a
way it was not intended.
On Wed, May 19, 2010 at 3:48 PM, Adriano dos Santos Fernandes
<ad...@gmail.com> wrote:
> 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
>
>
Re: WICKET-2846 - Store Application in InheritableThreadLocal instead
of ThreadLocal
Posted by Adriano dos Santos Fernandes <ad...@gmail.com>.
On 19/05/2010 17:03, Jeremy Thomerson wrote:
>>
>> To clarify this, I use Application.set and Application.unset there.
>> Although public, they're "not part of Wicket public API". So IMO, the
>> initial issue would be better done making these methods part of the public
>> API.
>>
>>
>>
> And then people all over will have memory leak issues because they'll forget
> a finally block.
>
Well, forgetting to unset it would not leak any more than have it
implicitly set like it's going to be. And I do think forgetting this is
developer fault.
What you all do not want to understand is what I said about Java library
spawning its own threads, and that is not documented, as its for cleanup
in the case I shown.
Adriano
Re: WICKET-2846 - Store Application in InheritableThreadLocal instead
of ThreadLocal
Posted by Jeremy Thomerson <je...@wickettraining.com>.
>
>
> To clarify this, I use Application.set and Application.unset there.
> Although public, they're "not part of Wicket public API". So IMO, the
> initial issue would be better done making these methods part of the public
> API.
>
>
And then people all over will have memory leak issues because they'll forget
a finally block. We tell you those are not part of the public API because
you should *not* use them. If you want to use them that way, then fine, but
you have to deal with the consequences - not us.
Re: WICKET-2846 - Store Application in InheritableThreadLocal instead
of ThreadLocal
Posted by Adriano dos Santos Fernandes <ad...@gmail.com>.
On 19/05/2010 16:48, Adriano dos Santos Fernandes wrote:
> 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.
To clarify this, I use Application.set and Application.unset there.
Although public, they're "not part of Wicket public API". So IMO, the
initial issue would be better done making these methods part of the
public API.
Adriano
Re: WICKET-2846 - Store Application in InheritableThreadLocal instead
of ThreadLocal
Posted by Adriano dos Santos Fernandes <ad...@gmail.com>.
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
Re: WICKET-2846 - Store Application in InheritableThreadLocal instead
of ThreadLocal
Posted by James Carman <ja...@carmanconsulting.com>.
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
<ad...@gmail.com> 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
>
>
Re: WICKET-2846 - Store Application in InheritableThreadLocal instead
of ThreadLocal
Posted by tetsuo <ro...@gmail.com>.
On Wed, May 19, 2010 at 4:30 PM, Adriano dos Santos Fernandes <
adrianosf@gmail.com> 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.
Re: WICKET-2846 - Store Application in InheritableThreadLocal instead
of ThreadLocal
Posted by Adriano dos Santos Fernandes <ad...@gmail.com>.
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
Re: WICKET-2846 - Store Application in InheritableThreadLocal
instead of ThreadLocal
Posted by Johan Compagner <jc...@gmail.com>.
----- 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.
Re: WICKET-2846 - Store Application in InheritableThreadLocal instead
of ThreadLocal
Posted by Adriano dos Santos Fernandes <ad...@gmail.com>.
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
Re: WICKET-2846 - Store Application in InheritableThreadLocal
instead of ThreadLocal
Posted by Alex Objelean <al...@gmail.com>.
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
--
View this message in context: http://apache-wicket.1842946.n4.nabble.com/WICKET-2846-Store-Application-in-InheritableThreadLocal-instead-of-ThreadLocal-tp2222639p2223101.html
Sent from the Wicket - Dev mailing list archive at Nabble.com.
Re: WICKET-2846 - Store Application in InheritableThreadLocal instead
of ThreadLocal
Posted by Adriano dos Santos Fernandes <ad...@gmail.com>.
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
Re: WICKET-2846 - Store Application in InheritableThreadLocal
instead of ThreadLocal
Posted by Alex Objelean <al...@gmail.com>.
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.
--
View this message in context: http://apache-wicket.1842946.n4.nabble.com/WICKET-2846-Store-Application-in-InheritableThreadLocal-instead-of-ThreadLocal-tp2222639p2223064.html
Sent from the Wicket - Dev mailing list archive at Nabble.com.
Re: WICKET-2846 - Store Application in InheritableThreadLocal instead
of ThreadLocal
Posted by Adriano dos Santos Fernandes <ad...@gmail.com>.
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
Re: WICKET-2846 - Store Application in InheritableThreadLocal
instead of ThreadLocal
Posted by Alex Objelean <al...@gmail.com>.
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...
--
View this message in context: http://apache-wicket.1842946.n4.nabble.com/WICKET-2846-Store-Application-in-InheritableThreadLocal-instead-of-ThreadLocal-tp2222639p2222771.html
Sent from the Wicket - Dev mailing list archive at Nabble.com.