JCgH4164838Gh792C124B5 commented on pull request #489: URL: https://github.com/apache/struts/pull/489#issuecomment-860077589
Hi @yasserzamani . Thanks for taking a look, and asking some good questions, which made me review and re-think things. 😄 Apologies in advance for the long response ... Unfortunately, from what I could find, performing state cleanup (or initialization) for custom JSP tags seems to rely a lot on "null setting" to ensure explicit consistency of state. It looks ugly (and is ugly to implement), but seems to be the only way to handle the situation. In terms of specifications and references, there was various material that I came across: - There is the JSP 2.0 specification (https://jcp.org/aboutJava/communityprocess/final/jsr152/index.html), which talks about JSP tag handlers and the JSP lifecycle. - There is the package javax.servlet.jsp.tagext Description (https://docs.oracle.com/javaee/7/api/javax/servlet/jsp/tagext/package-summary.html). It provides an overview and general background of the JSP Tag Libraries. The section on "Tag Handler as a Container-Managed Object" briefly mentions Tag Pooling. - There is the JSP Tag interface release method (https://docs.oracle.com/javaee/7/api/javax/servlet/jsp/tagext/Tag.html#release--). It mentions that `doStartTag()` and `doEndTag()` can be invoked multiple times before the JSP page implementation (container) calls `release()`. So if a container uses tag pooling, there is no guarantee as to when it `release()` might be invoked by the container. That appears to make `doEndTag()` the only choice for cleaning up state between potential calls to the same Tag object instance ... but it does not make it clear if it is "_correct_" or safe to do so. - There are also some SO posts that mention using `doEndTag()` to perform state cleanup (https://stackoverflow.com/questions/2481159/how-to-prevent-jsp-tags-from-being-reused-after-being-classloaded and https://stackoverflow.com/questions/616064/jsp-tag-lifecycle). - There is a blog on the topic of memory leaks with JSP Tag Pooling (https://blog.codecentric.de/en/2009/08/jsp-tag-pooling-memory-leaks-2/), that shows using `doEndTag()` to clean up references as well. - There is an old Tomcat Bugzilla (https://bz.apache.org/bugzilla/show_bug.cgi?id=33934) which **argues against** cleaning up state in `doEndTag()`, and instead only do so in the `release()` method. The issue appears to be that if a tag is reused on a page with the same attribute set, the container is not required to invoke setters for attributes that have the same value - which can result in incorrect behaviour if the tag itself has cleared them (such as cleanup in `doEndTag()`). There are seem to be different opinions on how to "properly" handle JSP Tag state cleanup when Tag Pooling is involved, but unfortunately I could not find any "crystal clear" answer on the topic. Attempting a solution like in this PR (clearing state in `doEndTag()`) appears to resolve the specific JIRA issue itself, but could potentially introduce unexpected / incorrect behaviour in other circumstances (such as if the container determines it does not need to call a setter again due to the attribute being the same on the same page). So **we should consider it carefully** before moving forward with such a change, as it may "_break more than it fixes_". We could certainly modify the PR to switch to performing the state cleanup in the `release()` method, which might be the **safer** move overall. It would still have _some_ benefits, even if that choice might not resolve the reported JIRA issue itself. What do you (and others) think ? However, if only `release()` cleanup is implemented, I think the reported issue seen running in a JSP container with Tag Pooling enabled would **still** occur (with old values from previous tag instance usage affecting subsequent usage). I suppose the question is _whether there is **any** way to reconcile the JSP 2.0 specification 2-54 statement_ (in the "Methods" section): "_After the doEndTag invocation, the tag handler is available for further invocations (and it is expected to have retained its properties)_", **and** _the weird behaviour that can happen with Tag Pooling if the tag handler instance's properties are retained in a subsequent usage_. So maybe the weird behaviour is "normal", due to the requirements of the JSP specification ? The only **other option** I can think of, would be to perform state cleanup only in `release()` (to be consistent with the absolute wording of the JSP specification), but provide a _new tag attribute_ that would allow users to choose to reset tag state in cases of Tag Pooling. Something like a `boolean` "resetTagStateAfterUse" ... it might suffer side effects of its own due to Tag Pooling, but at least the user would have _opted-in_ to potentially breaking activity. After the above review, I am leaning towards refactoring the PR to move cleanup to happen only via the `release()` method, and then possibly looking at introducing a "resetTagStateAfterUse" attribute to optionally allow some cleanup in the `doEndTag()` method. Please let me know what you think. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: [email protected]
