Re: WICKET-2846 - Store Application in InheritableThreadLocal instead of ThreadLocal
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-tp639p771.html Sent from the Wicket - Dev mailing list archive at Nabble.com.
Re: WICKET-2846 - Store Application in InheritableThreadLocal instead of ThreadLocal
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
Why would the application object itself need to be garbage collected? On Wed, May 19, 2010 at 8:53 AM, Adriano dos Santos Fernandes 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
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
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-tp639p2223064.html Sent from the Wicket - Dev mailing list archive at Nabble.com.
Re: WICKET-2846 - Store Application in InheritableThreadLocal instead of ThreadLocal
Use JRebel! Problem solved. :) On Wed, May 19, 2010 at 10:03 AM, Adriano dos Santos Fernandes 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
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 ThreadLocal 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 childValue * 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
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
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-tp639p2223101.html Sent from the Wicket - Dev mailing list archive at Nabble.com.
Re: WICKET-2846 - Store Application in InheritableThreadLocal instead of ThreadLocal
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
On Wed, May 19, 2010 at 11:56 AM, Adriano dos Santos Fernandes 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
On 19/05/2010 14:35, James Carman wrote: On Wed, May 19, 2010 at 11:56 AM, Adriano dos Santos Fernandes 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
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 wrote: > On Wed, May 19, 2010 at 11:56 AM, Adriano dos Santos Fernandes > 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
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 < adrian...@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
- 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
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
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 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
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
On Wed, May 19, 2010 at 4:30 PM, Adriano dos Santos Fernandes < adrian...@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
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
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 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
> > > 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
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
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" 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
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 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" > 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
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 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" > 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
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 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 > 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" >> 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
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 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" <
> adrian...@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
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 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 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" <
>> adrian...@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
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 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
> 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 >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" <
> >> adrian...@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
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
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
> 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
>> 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 > >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" <
>> >> adrian...@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
Agreed - I use pools for stuff like this. But, it's not broken for pools -
nothing functionally changed, right?
Jeremy Thomerson
http://www.wickettraining.com
-- sent from a wireless device
-Original Message-
From: James Carman
Sent: Wednesday, May 19, 2010 9:28 PM
To: dev@wicket.apache.org
Subject: Re: WICKET-2846 - Store Application in InheritableThreadLocal instead
of ThreadLocal
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
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
> 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
>> 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 > >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" <
>> >> adrian...@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
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 <
jer...@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 >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
> > 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" <
> > >> adrian...@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
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
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
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
On Thu, May 20, 2010 at 6:59 AM, Adriano dos Santos Fernandes 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
On 20/05/2010 08:02, James Carman wrote: On Thu, May 20, 2010 at 6:59 AM, Adriano dos Santos Fernandes 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
On Thu, May 20, 2010 at 7:08 AM, Adriano dos Santos Fernandes 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
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 <
adrian...@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
On Thu, May 20, 2010 at 10:29 AM, Johan Compagner 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
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 wrote: > On Thu, May 20, 2010 at 10:29 AM, Johan Compagner >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. >
