TagScript doesn't clear its cached tags after run()

• Key: JELLY-85
• Type: Bug
• Status: Resolved
• Resolution: Fixed
• Priority: Major
• Assignee: Unassigned
• Reporter: Scott Howlett
• Component/s: core / taglib.core
• Affects Version/s: 1.0-beta-4
• Fix Version/s: 1.0-RC2

Description

TagScript caches the tags it generates in a ThreadLocal. At the beginning of run() it checks to see if the context wants to cache tags - if not, it clears the cache and regenerates it.

But there is no corresponding check and cache clearing at the end of run(). So if a tag holds onto some significant resource, that resource will hang around until the thread goes away or until the tag is run again.

I am using Jelly Swing extensively, and various tags end up attached to the AWT Event thread for the lifetime of my application.

As a quick fix, I have a patch that simply repeats the check-and-clear-cache behavior at the end of TagScript.run(). I also have a patch that adds this behavior to StaticTagScript, whose run() never seems to clear cached tags.

I am probably just unclear, but it seems to me that there is a deeper issue as well - the context is being asked whether it wants to cache tags, but the result of this question affects the TagScript, which is really independent of the context. It seems like if context wants to cache tags, perhaps the ThreadLocal used for their storage ought to belong to the context somehow.

Comments

[dion gillard]

This patch has broken the define taglib and it's test cases.

I'm backing it out until a replacement patch can be made that doesn't do this.

[Hans Gilde]

I suggest that the problem is not the fact that the TagScript caches the Tag implementation, but the fact that the Tag implementation caches the heavy object after execution.

This is fortunate, because it's a whole lot easier for a Tag to stop caching it's state than it is for a TagScript to stop caching it's Tag implementation. It has to do with the way that caching works, which could be changed but not without big impact.

[Hans Gilde]

See JELLY-122 for a partial fix.

[Hans Gilde]

Having looked at this some more, this TagScript should not clear its cache after run().

The cache reuses a Tag implementation between several runs of the same script, in the same thread. It does not reuse the same Tag implementation multiple times within the same script.

The cache is a ThreadLocal object that is a member variable of TagScript. A seperate TagScript object is always created for every instance of a tag encountered in an XML document. So, there is no choice but to have a Tag implementation for every XML element in the document.

What the cache does do is ensure that the second time you run the script in the same thread, it will not create a second set of Tag implementations.

[Guido Anzuoni]

With a TagScript instance created by XMLParser in
its startElement and with every instance of TagScript
declaring a ThreadLocal, jelly is unusable in tipical servlet
environment.
Caching should be at higher level.
Jelly core should allow the run of scripts without
leaving any trace of itself at the end.

[Paul Libbrecht]

The attached pair of scripts seems to constantly raise the exception "Attempt to use a script that has been garbage collected.".
It combines a defined tag and an included script, quite a common situation, e.g. for Maven.
I could not find a way to reproduce the bug with only one of the two ingredients.

Invoke it with "java -jar target/fatJelly.jar blop.jelly".
paul

[Hans Gilde]

this is a problem with code introduced in the patch for JELLY-148

Hans and Paul,

'The attached pair of scripts seems to constantly raise the exception "Attempt to use a script that has been garbage collected.".'

Can we put the attachment and work against JELLY-166, that's where the error above is first reported.

I don't think it has much to do with JELLY-85

[Paul Libbrecht]

I think this is the right issue to post three possible approaches towards the tag-caching problem which was solved incompletely by the weak-reference wrapper (see JELLY-166). This problem is even known hard to James, the father of Jelly, as he stated once in an interview.

Just to summarize the requirement: jelly scripts are structured using Script objects. Script functionality is provided by Tags and there is a need to reference tags from scripts and this maybe several times.
As we want jelly-scripts to be able to run in simultaneous threads, an approach using ThreadLocal instance-members was made originally. It was impossible to clean-up. This was changed to the usage of a WeakReference... this got cleaned-up too quickly.

The three approaches that I see are as follows:
1 keep the instance ThreadLocal but maintain, as static ThreadLocal, a list of all these thread-locals and clear them when we're finished. This requires the least changes as it only adds a public static method to clean-up-tags-for-this-thread. It may be incomplete though.
2 use a static ThreadLocal which is a map that associates scripts to tags, and clear when finished. This adds just two or three methods but has the advantage of making this map explicit which can be useful in case the same tags want to be run again (e.g. Jelly-Swing's action tags)
3 use an explicit tag-holder mechanism in jelly-context... entirely disjoint from the thread. This is the cleanest but it requires all invocations to getTag() configureTag() to be added with the jelly-context as parameter. I think this may break a lot of things!

I think that, if we want to reach the release soon, we'd probably be better for 2.

In cases 1 and 2, clean-up has to be made explicit (in case 3, gc-ing the context should be enough). So the lack of change in the API-signatures results in a change in the API documentation: users-scripts will need to add another call to clean-up.

Thanks to bring your opinions !

paul

[Paul Libbrecht]

I have now committed modifications to TagScript and JellyContext to prevent any caching outside of the JellyContext.
At least disposing of the JellyContext, the script objects, or calling the context.clear() will clear all references to tags.

Jelly now builds fine.
It would be nice if users that experienced this give a stab at the latest snapshot, preferably from cvs.

thanks

paul

[Hans Gilde]

Paul, I made some changes to your code based on my experience using Jelly. And I fixed the part about thread guarantees for Tag instances. But, it's basically unchanged, I hope you approve.

[dion gillard]

Hans, looks ok to me.

Will run some tests tonight

[Paul Libbrecht]

Is "thread guarantees" the following excerpt of Tag interface javadoc?

> A Tag is only ever used by a single thread so that Tag developers
> do not need to concern themselves with mutli-threading
> issues when writing a Tag.

Wouldn't i be simpler to say that Thread safety is guaranteed by the usage of a single context per thread ? Or do you have situations where a single context is used by several threads ?

My tests and personal usages are fine with the current status of the tags, even with your memory-leak-test.

paul

[Hans Gilde]

Absolutely, this would be much easier. But it gets complicated when you look at what creating a new context means. Like, contexts search up the ie parent tree for variables and other stuff. And tags can search up from a thread tag to fiind parent...

[Paul Libbrecht]

Mmmh, that means that any operation that intend to use the script-to-tag association have to be run within the same thread. Maybe this could be more explicit somewhere.

Now... this means that JellySwing scripts should directly run in the AWT thread if they intend to run any jelly within the AWT thread (e.g. the body of Action), right?

I've tried this and have managed to actually find back my tags and achieve a preliminary "reloadable" Swing components (so that a panel gets "re-populated" by jelly:swing as the result of an action).
But I'd need to enrich the main Jelly class to add a -awt or -swing flag to start right away in the AWT thread.
Anything against it ?

paul

[Hans Gilde]

Well... It's not how Jelly works now, so I'll have to think about it. I'm really in favor of this: splitting off the different bits of functionality from JellyContext. While we're doing this, we could think about which parts are thread safe and which parts need to be kept in the same thread.

[dion gillard]

This bug report is now redundant and should be closed, right Hans and Paul?

[Paul Libbrecht]

Indeed.
We can clearly now say that this is fixed.
paul