|
ThreadLocal is working fine.
However, ThreadLocal uses a structure like WeakHashMap. The key to the map is the ThreadLocal object (it might as well be the TagScript instance b/c there's one ThreadLocal per TagScript instance). The value in the map is the Tag implementation. The problem is that the value (the tag implementation) keeps a reference to the key. This is because the Tag keeps a reference to the Script of its body, the body script keeps a reference to its parent, the parent is the TagScript that's used as the key in the map. So, once entered into the HashMap, the key and value stay forever. Solving this problem necessitates a change in the way TagScripts and Tags work together and is more complicated than I initially thought. The simplest way to fix this problem is to prevent a Tag from holding a hard reference to its body Script. Which this patch does.
But, the patch will break some stuff. It has an unfortunate side effect for Tags that want to keep their body Script around after they execute. For instance, Swing action tags execute their body as the action. If the Swing script is executed, then disposed of, the body script won't be available when the action is executed. However, the action Tag simply cannot be allowed to keep a hard reference to its body. If it keeps the hard reference, it will be trapped in the ThreadLocal forever. Argh. I'm trying to come up with a solution to this situation that doesn't horribly complicate things. here's a slightly better profiling class
If Tag could represent an "incarnated" TagScript wrt a JellyContext
than the various incarnations of the Script should be cached int the context itself rather than in a ThreadLocal. Adding a simple Map in JellyContext with TagScript as key will solve all memory leaks with no impact. Here follows the outline of changes. JellyContext { public Object getTagScriptData(TagScript t); public void setTagScriptData(TagScript t, Object d); }; TagScript { ... public void run(JellyContext context, XMLOutput output) if ( tag instanceof DynaTag ) { ....... }// end of public void run() }; Guido. Guido, this is a good solution, thanks much. I'll incorporate it into a patch tomorrow.
Hans, if you can do it without changing JellyContext that'd be even better.
Yes, sounds possible.
If we go that route, though, then there won't be an easy way to get an API on the cache itself. It'll be something that's totally within the TagScripts using special context variable names. No good way to get at the cache from the outside. About dion comment.
It seems to me that is hard to have the TagScript data not in About JellyContext. I suppose my comment is that I'm ok with it being stored in the context, but I'd rather not change the context API/interface.
Putting the thread local stuff in the JellyContext makes the memory leak go away as soon as the context is disposed of. Depending on how you use the context, the XMLParser might keep a reference to it. And it will have varying amounts of data in it depending on your "export" setting... this is all if we implement the tag cache in the context.
The Servlet stuff doesn't reuse the context or the parser so there's no leak for Servlets. But others might reuse the context, I know that I do. So, we have a few choices: 1) Let it leak memory into the context, tell people to trash the context and the parser as often as possible. If you don't, tough luck, you get a memory leak. 2) Implement a context method to clear the cache, tell people to clear the cache as often as possible. Clearing the cache will harm only those scripts that keep references to their body Script, like Swing Action or WindowListener. 3) Use the WeakReference thing, and tell everyone "if you want to use Action tags, where the action is a Jelly tag (as opposed to a class that implements ActionListener), you have to keep the Jelly Script around as long as the Swing screen stays up". 4) Some other solution that I can't think of due to my recent lack of sleep. Any suggestions/votes? I think I like #3. as someone not really following this, can I ask a question? (probably something dIon will need to answer).
Does this affect Maven while doing various things, in particular JSL of large XML documents? I just saw a swing reference, so wasn't sure it related. In regards to Maven, it only keeps contexts as long as needed, but an additional method to flush the cache would be handy to make sure of it if memory usage is a concern. (Assuming it is affected). option #3 (WeakRef) would mean no changes to JellyContext then, right? I'm all for no explicit API changes, but requiring people to keep explicit references to a Script when they want to re-use parts of it later.
(the blissed project, http://blissed.codehaus.org In terms of its affect on Maven, this issue affects anyone who compiles and runs many Jelly scripts over the course of their application. This is most noticed by Servlet users that execute Jelly scripts on every Servlet request, all in the same JVM.
Any Maven script that compiles and runs hundreds of sub scripts over the course of its execution should notice an increase in memory use. I suspect that no one has noticed this because people don't really care if their Maven script eats an extra 2 megs of memory, since it will exit eventually. Also, this bug has been in Jelly for a while, so I doubt that Maven users have been impacted. I just uploaded thread-local-memory-leak-fix-demo-parent.txt. This patch still needs JavaDoc and a unit test and isn't ready to be applied for real. I'm hoping to see if people like the idea first.
I'm not thrilled with the idea of caching stuff in the JellyContext because it opens up the problems of having to explicitly clear the cache or throw out the JellyContext after every use. Having to do either would force me to change my own, existing embedded Jelly code, so... I'm trying to avoid it. This is an alternate solution that I really like. My first solution was to prevent any tag from keeping a hard reference to any TagScript (like the tag's body). This works fine, except when tags want to keep a reference to their body script. The new solution is this: prevent tags from keeping hard references to their parents. All references to parents become weak. This eliminates the memory leak. It also doesn't break tags that want to execute their bodies at a later time. Except that... the bodies become headless. In other words, if a tag (like Swing's ActionTag) keeps it's body script, the body script may not be able to find parents. It will, though, have full access to its context. Basically, if you keep a reference to a tag's body script, you still can't prevent the tag's parent from being GC'd. Although this all might sound complicated, it seems to me that it's simpler than the scenarios that arise with caching in the JellyContext. In a future verison of Jelly, we can make proper API changes to allow caching in the JellyContext and to generally support caching in a better way. Would you like us to apply this locally and do some testing?
Hans, I'll apply 'thread-local-memory-leak-fix-demo-parent' to my local build and test with it this week. Your proposed API impact sounds very reasonable and ideal.
Naively, I'd hope for ways to automatically test the fact that the leak is present or not...
(i.e. without a profiler) Any hope for this ? paul Paul, to test, just parse + run a script in a loop, you should be able to hit OOM pretty quickly (or at least see a very noticeable jump)
Hans, the latest patch doesn't pass the unit tests. It breaks TagSupport.findAncestorWithClass()
Hans, an alternate solution would be to modify TagSupport so that its internal parent reference was a WeakReference. As long as code used getParent(), we can fully encapsulate it.
What will break is:
I looked over things a bit more.. I'm almost for just modifying the API to remove the use of the ThreadLocal to just fix the problem.
Really, its all about parent tags. I almost just assume:
The latter is cleaner imho, but the former would be less impact on user code I think, as JellyContext is a concrete class that we can force the methods to be in (vs potential user implementations of Script). Sorry about the unit test failure, I had it all working yesterday, I'll find/fix the problem.
In terms of fixing the Script API... sure! If we don't mind breaking any Script implementaitons that may be out there. But I know I can make it work this way because I saw the unit tests pass. I just broke something when I cleaned it up. Paul, we can test for the leak in the unit test. We 1) force finalizers and GC, 2) sample the memory used, 3) compile/execute a script several times, 4) null all references 5) force finalizers and GC, 6) memory used should not exceed original memory used +XYZ amount.
I uploaded jelly-nothreadlocal.ZIP that contains modified version of
JellyContext, TagScript and StaticTagScript. Sorry, I am not familiar with cvs, diff & patch  .The changes affect JellyContext for the inclusion of a simple management of a Map of userData. TagScript and StaticTagScript are modified in getTag, setTag and clearTag to expect a JellyContext arg. ThreadLocal member is suppressed. I made limited testing with a jsp (a Subversion browser) creating a new JellyContext at each invocation without problems and without memory leak too. Hope it helps somehow. I have failures on some unit tests:
org.apache.commons.jelly.core.TestInvokeStaticTag Any updates? Yes, sorry to take so long but I've been trying to find a better solution.
The idea with the parent-weak-references doesn't really work. The idea about keeping the cache in the regular context HashMap doesn't work either. It gets screwed up if you change the inherit/export setting on the JellyContext. This leaves these options:
The only problem with option 2 is that we're likely to make an API change, only to make another change later. I vote for option 1, which puts us back to the patch thread-local-memory-fix-demo.txt I like option 1 too. Will test the patch.
There is one bug relating to not trimming whitespace from text. I'm fixing this one now.
Just uploaded thread-local-memory-leak-fix-final.txt, which replaces thread-local-memory-leak-fix-demo.txt but fixes the prblem with trimming whitespace.
All that's left is unit tests that detect a memory leak. the xml taglib fails in it's transform unit tests
The XML failures don't seem to have anything to do with the patch. I think it's something to do with the base path (current working directory).
Backing out the change and re-running the test results in a clean build.
It's not the working directory AFAICT, it's the patch. I'm having truoble getting it to pass at all. You're running "maven test"?
ok, got it working, debugging the patch now.
First, my opinions are affected by my way of dealing with templates-context-transformation in servlet env.
From my point of view, templates are in-memory representation of (xml) files that can safely be used concurrently by different threads. Contexts are the environment in which actualisations (i.e. transformation) of templates takes place. In servlet env, context is strictly related to request. This means that a context is never reused, because of this relation. Using a ThreadLocal to hold the "actual" incarnation of a Tag causes an implicit dependency Context <-> Current Thread. More, with this way of using contexts in mind, tag cleaning must be called at the beginning of each transformation. Looking at the way ThreadLocal hold references, it can be seen that, in an environment where threads are resused, such a fine grain use of ThreadLocal instances leads to an uncontrollable memory usage which depends on nr. of alive thread (not necessairly "active") and nr. of tags. More, if it is considered the way XMLParser works, there is no easy way to add TagScript caching leading to, I think, huge gc activities. My architectural view of the classes should allow caching of parsed ScriptBlock that will avoid continuous xml source parsing. For what concerns JellyContext API change, I think that it is not a big problem because the added methods are used by TagScript, while library implementors use Tag. Anyway, a still better solution, in my opinion, could be the one that moves userData as a WeakHashMap into the TagScript with JellyContext as a key. Anyway, an open issue with this approach, and the previous as well, is the behaviour in case, for example, of a TagScript holding reference to the Tag via userData and the enclosing TagScript invoking run() with a child JellyContext. In this situation the Tag instance can be found only traversing the contexts from child to parent(s). In jelly-nothreadlocal2.ZIP you will find an implementation of this approach. Just my 2 cents. thread-local-memory-leak-fix-final-1.txt - patch to core for fix
thread-local-memory-leak-fix-final-xml-tags.txt - patch to XML tags for fix
I uploaded patches that fix the immediate problem. I also checked all existing tag libs for the pattern that causes the problem: iterating down through one's Scripts, checking their type with instanceof. Only the XML tags do this.
Of course, it's totaly bogus to do this and it should be explicitly disallowed. However, I'm actually am somewhat stumped by a particular behavior of the XML output and wasn't able to replace this functionality with something better. For 1.1/2.0/sometime soon, we should fix the underlying condition in the XML tags, which is harder than it looks at first. I'll file a jira about it. For now, the patch passes the unit tests. Guido, I agree with your ideas in principal. However, for 1.0 we don't want to change the API, especially to replace it with a potentially less stable API.
Going forward, we will undoubtedly have a discussion on the proper API to use. Maybe you could file a new issue in jira containing your suggestion? This way, we can hash out the API and implement something more stable in a future release. I'll attach a unit test to explicitly test for a memory leak tomorrow. In the mean time, the lack of a leak can be verified visually with the tester that I uploaded before.
Thanks Hans! I'll give the updated patches a spin today.
memory-leak-junit-test.txt - Non-scientific, yet useful memory leak tests.
hans, your latest patch helps, but it doesn't completely remove the leak in my application. i'm digging in to try to find more details about where the object retention actually is.
can i see the source? the first question is: is it coming from the parsing or the executing or a combination of both?
the last patch includes a base class for a unit test that'll parse and execute a script over and over. both, parse + execute. i have a script in my application that builds an object model, and its reparsed + executed when the file is changed. if i disable the caching and parse+execute every time the data is needed, memory blows up. i'm currently working to isolate a test case and see just what objects are being retained.
I have applied the patches with some small changes:
If everyone could test against this build from CVS, that'd be great. I've applied something very close to the patches. If anyone has a problem, please reopen this issue.
There's some problem with the patch for this issue, see
I'm looking into it. This is a simplistic approach towards the notion of "run-session". Namely... it's using static ThreadLocal instead of instance-based.
The ThreadLocal's content is a map whose keys are the script objects. (and disabling the WeakReference) The ThreadLocal's content is expected to be cleared at the complete end of a run but I didn't find the exact place yet for this (in JellyContext.run()?). The method to clear is called clearTagHolderMap() and is public static which makes it that every one should be able to call it, even, say, in a finally or such. Unit tests and my basic usage both work. It would be interesting to see how other's would get it working. paul |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
 |
|
Bug
Resolved
Critical
Huge memory leak resulting from the use of ThreadLocal
Also, I'm working on a fix that will keep the one-tag-inplementation-per-thread system that Jelly wants, while stopping the memory leak.
The only thing is that the current use of ThreadLocal in TagScript doesn't look to me like it should cause a memory leak. Odd.