Sylvain Wallez wrote:
Carsten Ziegeler wrote:
Sylvain Wallez wrote:
So the question is:
- why is the pipeline automatically released? Is to avoid memory leaks?
Yes, in 2.1.x there was no good place for explicitly releasing the
pipeline; this required some incompatible changes we did in 2.2; the
pipelines are released there explicitly and not automatically.
If the pipeline is not released properly, this means that all pipeline
components are not released, creating a hugh memory leak.
I see, and cleaning up is better than letting memory leak.
But I have a problem with the InheritableThreadLocal, which can also
create leaks, as one of its effects is that child threads inherits the
environment stack of their parent thread at the time where they (the
children) were created.
If these child threads are created and terminated within the scope of
the parent's thread request, then this is no problem (e.g. in parallel
cinclude), but it is a problem for long-running background threads such
as those created by the RunnableManager (used by the scheduler)
RunnableManager does not create threads by itself.
or, as in my case, by user code.
Please advise user to use RunnableManager, he should not (as far as possible),
create threads.
So IMO, we should change the way environment inheritance between threads
is managed:
- if a child thread runs in the scope of its parent, then a special
CocoonThread class (extends Thread) should be used, which will copy the
needed environment data before launching its Runnable.
- normal threads created with "new Thread()" inherit nothing and can
therefore work in a clean environment.
This approach requires two changes in the 2.1 code base (haven't checked
2.2 yet):
- DefaultIncludeCacheManager.load() in o.a.c.transformation.helpers
DefaultIncludeCacheManager has bigger problems than that; in one place it uses
RunnableManager (line 116), while in another it uses new Thread (line 186). It's
better be changed to RunnableManager, and thread.join() must go (line 234), so
in the end there is no need for CocoonThread here.
- maybe PortalManagerImpl.loadCoplet() in o.a.c.webapps.portal.components
Same, should be switched to RunnableManager.
In these two places, the CocoonThread proposed above have to be used to
inherit the parent environment. In all other places where "new Thread()"
is called, inheriting the parent environment is not needed and worse,
can be harmful.
WDYT?
After second look, does not look too harmful, even though unnecessary. Might
break some folks who are using Threads, though. Not sure if it can be changed in
2.1...
Vadim
