<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.
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.)
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.
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.
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 ().
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.
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 and 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 implementation: 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).
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
(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.
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 instances. 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".
Tim's forEach problem would seem to be the one referred to in https://issues.apache.org/bugzilla/show_bug.cgi?id=25623.
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.
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?
(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.
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.
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.
It doesn't look to me like we'll be doing anything here for the 1.1.3 release.
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).
Fix in 1.2 standard-jstlel