Bug 33934

Summary: [standard] memory leak in jstl c:set tag
Product: Taglibs Reporter: Andreas <av>
Component: Standard TaglibAssignee: Tomcat Developers Mailing List <dev>
Severity: critical    
Priority: P2    
Version: 1.0.6   
Target Milestone: ---   
Hardware: PC   
OS: All   
Attachments: Fix for memory leak in c:set

Description Andreas 2005-03-09 17:24:44 UTC
<c:set ..> does not clear its target/value fields after usage, it keeps
references to these objects when the application server places the tag instance
back into a tag pool.

Example: I have a session attribute named hugeBean and set a property of that
bean with <c:set target="${hugeBean}" ... />. In doStartTag() the EL ${hugeBean}
is evaluated and the result is placed into the field "target" - see
org.apache.taglibs.standard.tag.el.core.SetTag and
org.apache.taglibs.standard.tag.common.core.SetSupport. In doEndTag() the
property of hugeBean is changed, but the field "target" in SetSupport is not
cleared afterwards. After calling doEndTag() the application server may (and
tomcat does) put the tag into a tag pool for recycling - release() is not called
before this (which is correct I think). Now the tag pool contains the SetTag
instance that still contains a reference to the hugeBean. If the application
removes hugeBean from the session, it can not be garbage collected because of
the reference from the tag pool.

The fix should be not too difficult, I think doEndTag() should clear all
references to appliaction objects.
Comment 1 Martin Cooper 2005-03-10 06:13:31 UTC
This isn't something that should be changed. To adhere to the JSP spec, a tag 
cannot clear out values that correspond to tag attributes. See the description 
of AttSets (attribute sets) in the spec.

(I'm leaving it to one of the Standard folks to change the status of this bug.)
Comment 2 Andreas 2005-03-10 09:59:42 UTC
Quote from the spec:

Note that since there are no guarantees on the state of the properties, a tag 
handler that had some optional properties set can only be reused if those 
properties are set to a new (known) value. This means that tag handlers can 
only be reused within the same "AttSet" (set of attributes that have been set).

I'd say, this means that there is no requirement for tag handlers 
to "remember" their state after doEndTag(). 

I understand this like this example: a tag has attributes a1, a2, a3, a4, a5. 
When its used the first time, e.g. the attributes a1, a2 and a4 are set. Then 
it can be reused only in situations where  a1, a2 and a4 are set again. It can 
not be reused in a situation where only a1 and a2 are set again. Its not 
assumed that a4 will still contain the value from the previous call here. 

Also it would not make much sense to remember the value I think, because it 
would lead to some random behaviour. Another example:

<c:out value="${bean.property}" escapeXml="false"/>
<c:out value="${bean.property}"/>

Is escapeXml true or false in the second call? Of course it should be true, so 
the container can not reuse the first instance whose escapeXml attribute does 
not have the default value. If the container were allowed to reuse the tag 
handler, the behaviour would be random (depending on tag handler caching). But 
the container may reuse the c:out tag handler here:

<c:out value="${bean.property}" escapeXml="true"/>

because the same attributes are set again.

Comment 3 Martin Cooper 2005-03-10 21:03:57 UTC
Your quote from the spec refers to the invocation of the release() method. 
However, the release() method is not required to be called between uses of the 
tag within the same page. If a tag is reused on a page with the same AttSet, 
then the container is not required to invoke the setter for attributes that 
have the same value. Therefore, the tag itself cannot clear out the values, 
because the container would then be assuming the old values, which would no 
longer be there.

Your example with the 'escapeXml' attribute is not valid, because the two tag 
usages do not have the same AttSet, and so the rule for reusing tag handlers 
with the same AttSets does not apply.
Comment 4 Andreas 2005-03-11 09:26:39 UTC
Oops - you are right, I missed the paragraph above where the spec says:

... After the doEndTag invocation, the tag handler is available for further 
invocations (and it is expected to have retained its properties.

But it still seems to me that the memory leak is not necessary for the EL 
version of c:set. In SetTag.java, doStartTag() calls evaluateExpressions() 
which means that new values are assigned to the fields value, target and 
property. If these are assigned on every doStartTag() there is no need to 
retain their state after doEndTag(). 

The question may be whether the string "${bean.property}" should be retained 
or the result of the evaluation of that string. c:set retains both but 
discards the result of evaluation on every doStartTag().

So I think its valid to clear value, target and property fields after doEndTag
Comment 5 Andreas 2005-03-22 09:25:27 UTC
Let me summarize the bug once more:

The field "target" of the EL version of the c:set tag contains a reference to
the bean whose property shall be modified, lets call that e.g. "hugeBean". After
doEndTag() was called the application server puts the c:set tag instance into a
tag pool for recycling. The field "target" still holds the reference to
"hugeBean", so it can not be garbage collected. This is not necessary, because
"target" is assigned a new value on every usage of the c:set tag instance in
doStartTag(). doStartTag() evaluates the EL string (e.g. "${hugeBean}") and
assigns the result to the field "target".

Whenever the c:set tag is used - for the first time or recycled from the pool -
the method doStartTag() is called. doStartTag() evaluates the EL expression and
assigns the result to the field "target". So there is no need to retain the
result of that evaluation in the field "target" after doEndTag() was processed,
because on the next usage it will overwritten anyway. Its sufficient to retain
the EL string "${hugeBean}" but retaining the result of its evaluation causes
the memory leak.
Comment 6 Tim Burrell 2007-01-12 11:11:02 UTC
NOTE:  I'm new to asf bugzilla, so please forgive any mis-use of this website and please let me know if 
there is a better way for me to submit a request like this in the future!  :\

I am having the same problem (memory "leak") with this tag along with the c:forEach tag and I was 
wondering if there's any progress being made on this issue?  It appears from the last two comments 
this bug's state that this issue is still open.  I think Andreas' last post suggests a way to conform to the 
spec and still fix the memory leak.  From perusing the code for the forEach tag, I believe a similar 
approach can be used to solve the same problem.  The following "rules" (which are merely re-stating 
Andreas' last suggesting to something more general) could be applied to all tags in the jstl 

1.  If a tag has a reference that is in the AttSet, only set it to null in the release() method.
2.  Otherwise, set it to null at the end of doEndTag.

This would solve the memory leak that Andreas observed 2 years ago and it would solve the memroy 
leak I'm seeing in the forEach tag (there is a reference to "item" in the LoopTagSupport class that is not 
in the AttSet and keeping it around is causing my application some memory problems).
Comment 7 Henri Yandell 2007-03-26 17:22:55 UTC
Apologies for the delay in reply Tim.

This issue is one of the ones I've listed for trying to fix for a 1.1.3 release
(slowly getting there). Patches are very welcomed - otherwise I'll slowly plod
along and hopefully get somewhere with it.

The specific one mentioned in Andreas' last comment sounds good to me, though
I'm not planning to go through all of the tags. Currently I have:

c:set - release 'target' in doEndTag
c:forEach - release 'item' in doEndTag

Comment 8 Kris Schneider 2007-03-26 19:53:45 UTC
(In reply to comment #7)
> c:set - release 'target' in doEndTag
> c:forEach - release 'item' in doEndTag

But you're not allowed to do that sort of thing in doEndTag, it's only valid for
the release method (which usually calls a private init method). However, even if
the properties are cleared by the release method, there's no guarantee as to
when the container will call it, so it can still behave like a leak.

I haven't looked at <c:forEach>, but the init method for <c:set> should at least
set target and property to null (it already sets value to null).

Beyond that, I can remember hacking something together that used reference
objects but also required implementing the TryCatchFinally interface. I'd have
to double-check, but I think that would still maintain binary compatibility. The
real question is whether reference objects would violate the spec's definition
of persistent:

Once properly set, all properties are expected to be persistent, so that if the
JSP container ascertains that a property has already been set on a given tag
handler instance, it must not set it again.

The JSP container may reuse classic tag handler instances for multiple
occurrences of the corresponding custom action, in the same page or in different
pages, but only if the same set of attributes are used for all occurrences. If a
tag handler is used for more than one occurence, the container must reset all
attributes where the values differ between the custom action occurrences.
Attributes with the same value in all occurrences must not be reset. If an
attribute value is set as a request-time attribute value (using a scripting or
an EL expression), the container must reset the attribute between all reuses of
the tag handler instance.
Comment 9 Andreas 2007-03-27 00:39:22 UTC
The class org.apache.taglibs.standard.tag.el.core.SetTag (in 1.0.6) has two 
fields, "_target" and "target". The field "_target" is set by the setTarget 
method and contains the EL expression, e.g. "${hugeBean}". This field may not 
be cleared between invocations because the container is allowed to cache tag 

The other field "target" contains the result of the evaluation of the EL 
expression of "_target" and is assigned a new value on each and every call to 
doStartTag() which the container must call. So it makes no sense to keep the 
reference in "target".

Same applies to the fields "_value" and "value".
Comment 10 Henri Yandell 2007-03-27 12:03:24 UTC
Tim's forEach problem would seem to be the one referred to in
Comment 11 Henri Yandell 2007-03-27 12:15:30 UTC
My fix in https://issues.apache.org/bugzilla/show_bug.cgi?id=34896 should be
looked at too. Same concept of leakage, except in this case it was a connection
not closing.
Comment 12 Henri Yandell 2007-09-07 07:44:02 UTC
Looking at SetSupport, its init() already sets target and property to null:

    private void init() {
        value = target = property = var = null;
        scopeSpecified = valueSpecified = false;
        scope = PageContext.PAGE_SCOPE;

Looking at the JSTL LoopTagSupport, its init() sets item to null.

So the easy part of Kris' comment is already taken care of. 

Andreas points out that the attributes themselves are handled by target_,
property_ etc in the EL SetTag and suggests the transient target, property etc
could be released. They're not in the RT SetTag though, so I presume we can't
release them in the support class.

ie) Andreas' suggested change is the addition of a doEndTag method to EL SetTag
and (presumably as it's simplest) a call to init() from it.

Looking at the EL ForEachTag, the same could be done there.

Kris - is that still going to be against the spec?
Comment 13 Kris Schneider 2007-09-07 08:42:44 UTC
(In reply to comment #12)

Are we thinking about making changes on the STANDARD_1_0_BRANCH branch (e.g. for
a 1.0.7 release)? That's the only place where changes to
org.apache.taglibs.standard.tag.el.core.SetTag would make a difference. If we
are, then I think we can safely reset value, target and property in doFinally -
assuming that implementing TryCatchFinally maintains binary compatibility. This
*only* applies to the EL version, not the RT version. Which means it *only*
applies to Standard 1.0, not 1.1.
Comment 14 Henri Yandell 2007-09-09 00:54:34 UTC
I hadn't realized that - so this is only a 1.0.x bug and not a 1.1.x bug?

I don't think we've any intent to do a 1.0.7.
Comment 15 Kris Schneider 2007-09-10 10:20:00 UTC
The bug that Andreas refers to in comment #5 and comment #9 is specific to 1.0.

For 1.1, the only thing I can see attempting is what I mentioned in comment #8:

Beyond that, I can remember hacking something together that used reference
objects but also required implementing the TryCatchFinally interface.
Comment 16 Henri Yandell 2007-12-07 05:54:11 UTC
It doesn't look to me like we'll be doing anything here for the 1.1.3 release.
Comment 17 Jeremy Boynes 2010-10-16 13:47:22 UTC
Created attachment 26179 [details]
Fix for memory leak in c:set

This would be a problem for all branches when a user still uses the old 1.0 tag libraries.

Patch attached to refactor SetSupport to defer evaluation of the expressions until needed allowing only the attribute values to be retained (per the spec).

Patch also switches to using the JSP container's EL evaluator and pre-parses the expressions (avoiding reparsing if the container is pooling tags).
Comment 18 Jeremy Boynes 2010-10-23 14:00:32 UTC
Fix in 1.2 standard-jstlel