You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@struts.apache.org by GitBox <gi...@apache.org> on 2021/06/07 02:58:33 UTC
[GitHub] [struts] JCgH4164838Gh792C124B5 opened a new pull request #489: WW-5124 - Proposed fix for Struts JSP tag behaviour with tag pooling
JCgH4164838Gh792C124B5 opened a new pull request #489:
URL: https://github.com/apache/struts/pull/489
WW-5124 - Proposed fix for Struts JSP tag behaviour on application servers that use tag pooling.
- Prior to this change, on application serves such as Glassfish, tag pools that re-use tag instances demonstrated incorrect behaviour, due to previous / old tag state still being present.
- Added new clearTagStateForTagPoolingServers() to StrutsBodyTagSupport. It is called in doEndTag(), and descendants that override doEndTag() should call the ancestor method or directly call clearTagStateForTagPoolingServers().
- Unit test support methods were added to StrutsInternalTestCase and AbstractTagTest.
- Basic sanity checks for existing unit tests were added where feasible. They reflection-compare a tag's state after doEndTag() is called with that of a new instance, to help catch if future changes miss calls in the
clearTagStateForTagPoolingServers() hierarchy.
- Removed some unused imports, added some missing override annotations to classes that were modified as part of this change.
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
For additional commands, e-mail: dev-help@struts.apache.org
[GitHub] [struts] lukaszlenart commented on pull request #489: WW-5124 - Proposed fix for Struts JSP tag behaviour with tag pooling
Posted by GitBox <gi...@apache.org>.
lukaszlenart commented on pull request #489:
URL: https://github.com/apache/struts/pull/489#issuecomment-860245983
This is astonish work 👏 Yet I have some concerns about potentially breaking backward compatibility, is there a chance/way to make this configurable? To have a flag to enable tag pooling support if needed?
--
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:
users@infra.apache.org
[GitHub] [struts] JCgH4164838Gh792C124B5 commented on pull request #489: WW-5124 - Proposed fix for Struts JSP tag behaviour with tag pooling
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [struts] lukaszlenart closed pull request #489: WW-5124 - Proposed fix for Struts JSP tag behaviour with tag pooling
Posted by GitBox <gi...@apache.org>.
lukaszlenart closed pull request #489:
URL: https://github.com/apache/struts/pull/489
--
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.
To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
For additional commands, e-mail: dev-help@struts.apache.org
[GitHub] [struts] lukaszlenart commented on pull request #489: WW-5124 - Proposed fix for Struts JSP tag behaviour with tag pooling
Posted by GitBox <gi...@apache.org>.
lukaszlenart commented on pull request #489:
URL: https://github.com/apache/struts/pull/489#issuecomment-992206440
@JCgH4164838Gh792C124B5 I'm closing this PR as the related changes have been merged into `master` - thanks a lot for all your work 💯
--
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.
To unsubscribe, e-mail: issues-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [struts] yasserzamani commented on pull request #489: WW-5124 - Proposed fix for Struts JSP tag behaviour with tag pooling
Posted by GitBox <gi...@apache.org>.
yasserzamani commented on pull request #489:
URL: https://github.com/apache/struts/pull/489#issuecomment-872000787
@JCgH4164838Gh792C124B5 sorry for the late answer, I was offline for a while. And thanks a lot for the sophisticated description! Meanwhile I try to study them and let you know if I have something to say, but anyway it's LGTM as well when you say that it's best possible solution we could investigate online :)
--
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.
To unsubscribe, e-mail: issues-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [struts] JCgH4164838Gh792C124B5 commented on pull request #489: WW-5124 - Proposed fix for Struts JSP tag behaviour with tag pooling
Posted by GitBox <gi...@apache.org>.
JCgH4164838Gh792C124B5 commented on pull request #489:
URL: https://github.com/apache/struts/pull/489#issuecomment-917715659
The latest push fixed the previous failure in the build, but now there are two failures reported in `DefaultDispatcherErrorHandlerTest` (`testHandleErrorIllegalStateException()`, `testHandleErrorIOException()`). The failure reported is the same for both: "Unexpected method call HttpServletResponse.setContentType("text/html").".
This does not make sense to me, as neither `DefaultDispatcherErrorHandler` nor its unit test was changed in the PR. When I build this locally (Windows 10, Oracle JDK7 or OpenJDK 11), there are no failures, and manual builds complete normally against the PR's Git branch.
Maybe it is happening due to something with the automated build environment (or Linux vs. Windows) ?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [struts] lukaszlenart commented on pull request #489: WW-5124 - Proposed fix for Struts JSP tag behaviour with tag pooling
Posted by GitBox <gi...@apache.org>.
lukaszlenart commented on pull request #489:
URL: https://github.com/apache/struts/pull/489#issuecomment-869679190
Great, thanks a lot! I will go ahead with 2.5.27 and then we can add this feature to 2.5.28 (I would like to focus on 2.6 but that's ok ).
--
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.
To unsubscribe, e-mail: issues-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [struts] coveralls edited a comment on pull request #489: WW-5124 - Proposed fix for Struts JSP tag behaviour with tag pooling
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #489:
URL: https://github.com/apache/struts/pull/489#issuecomment-855544281
[](https://coveralls.io/builds/43074096)
Coverage increased (+0.6%) to 47.882% when pulling **9143b2aafdb2cbdc017d8e2614769b1f978fcf74 on JCgH4164838Gh792C124B5:localS2_25_WW-5124_fix** into **a96dd31be6bd16fd84ca3d683208cae8774d75b4 on apache:struts-2-5-x**.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [struts] JCgH4164838Gh792C124B5 commented on pull request #489: WW-5124 - Proposed fix for Struts JSP tag behaviour with tag pooling
Posted by GitBox <gi...@apache.org>.
JCgH4164838Gh792C124B5 commented on pull request #489:
URL: https://github.com/apache/struts/pull/489#issuecomment-855547053
Hello Apache Struts Team.
This PR ended up involving many more files than originally planned, due to the number of unit test changes that were made. This is a first draft attempt to find a solution to the behaviour issue seen with tag pooling application servers, but others who review it may be able to point to a better alternative.
Interactive tests with Glassfish 5.0.1 appear to show the idea works, and the Showcase application seems to also work properly too. The call overhead of these changes may exceed the benefits, but should make the JSP tag behaviour correct (or mostly correct) when tag class instances are re-used by tag pools.
If anyone has suggestions on improvements, or alternatives, please let me know.
--
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:
users@infra.apache.org
[GitHub] [struts] yasserzamani edited a comment on pull request #489: WW-5124 - Proposed fix for Struts JSP tag behaviour with tag pooling
Posted by GitBox <gi...@apache.org>.
yasserzamani edited a comment on pull request #489:
URL: https://github.com/apache/struts/pull/489#issuecomment-918063695
@JCgH4164838Gh792C124B5 I think it's something related to threading! Looks like that some tests have called `PrepareOperations.overrideDevMode(true);` and at Travis side, `DefaultDispatcherErrorHandlerTest` is executed in same thread so those tests with `devMode=false` have been failed :)
I think the solution (i.e. to improve these tests), firstly we should add `PrepareOperations.resetOverridenDevMode()` method which resets it to `null`. And then call it in `DefaultDispatcherErrorHandlerTest.setUp` :)
--
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.
To unsubscribe, e-mail: issues-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [struts] coveralls commented on pull request #489: WW-5124 - Proposed fix for Struts JSP tag behaviour with tag pooling
Posted by GitBox <gi...@apache.org>.
coveralls commented on pull request #489:
URL: https://github.com/apache/struts/pull/489#issuecomment-855544281
[](https://coveralls.io/builds/40345438)
Coverage increased (+0.3%) to 47.653% when pulling **e7fac4d921f3225dbbf9c201e9fd0e780a22aa2b on JCgH4164838Gh792C124B5:localS2_25_WW-5124_fix** into **620b8c3a0ce4b880c4cb2a0c857d2cea1eabe92f on apache:struts-2-5-x**.
--
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:
users@infra.apache.org
[GitHub] [struts] yasserzamani commented on pull request #489: WW-5124 - Proposed fix for Struts JSP tag behaviour with tag pooling
Posted by GitBox <gi...@apache.org>.
yasserzamani commented on pull request #489:
URL: https://github.com/apache/struts/pull/489#issuecomment-918063695
@JCgH4164838Gh792C124B5 I think it's something related to threading! Looks like that some tests have called `PrepareOperations.overrideDevMode(true);` and at Travis side, `DefaultDispatcherErrorHandlerTest` is executed in same thread so those tests with `devMode=false` has been failed :)
I think the solution i.e. to improve this tests, firstly we should add `PrepareOperations.resetOverridenDevMode()` method which reset it to `null`. And then call it in `DefaultDispatcherErrorHandlerTest.setUp` :)
--
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.
To unsubscribe, e-mail: issues-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [struts] JCgH4164838Gh792C124B5 commented on pull request #489: WW-5124 - Proposed fix for Struts JSP tag behaviour with tag pooling
Posted by GitBox <gi...@apache.org>.
JCgH4164838Gh792C124B5 commented on pull request #489:
URL: https://github.com/apache/struts/pull/489#issuecomment-913899875
Local build worked, but something is out of state. It may be a bit before I can untangle the mess (the number of changes grew more than what I originally intended).
--
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.
To unsubscribe, e-mail: issues-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [struts] JCgH4164838Gh792C124B5 commented on pull request #489: WW-5124 - Proposed fix for Struts JSP tag behaviour with tag pooling
Posted by GitBox <gi...@apache.org>.
JCgH4164838Gh792C124B5 commented on pull request #489:
URL: https://github.com/apache/struts/pull/489#issuecomment-869257253
Hi @lukaszlenart .
The concern about breaking backwards-compatibility that you mention (plus the possibility of doing something non-compliant with the JSP specs) is definitely there, and probably makes this change too risky, at least as it is currently structured.
I will think about the scenario, and see if I can refactor this PR so that the state-clearing behaviour in `doEndTag()` is controlled by a flag (either globally, or as a tag attribute). It might take me a little while to work on an alternative, so please bear with me.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [struts] yasserzamani edited a comment on pull request #489: WW-5124 - Proposed fix for Struts JSP tag behaviour with tag pooling
Posted by GitBox <gi...@apache.org>.
yasserzamani edited a comment on pull request #489:
URL: https://github.com/apache/struts/pull/489#issuecomment-872000787
@JCgH4164838Gh792C124B5 sorry for the late answer, I was offline for a while. And thanks a lot for the sophisticated description! Meanwhile I try to study them and let you know if I have something to say, but anyway it's LGTM as well when you say that it's best possible solution that we could investigate online :)
--
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.
To unsubscribe, e-mail: issues-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [struts] yasserzamani commented on pull request #489: WW-5124 - Proposed fix for Struts JSP tag behaviour with tag pooling
Posted by GitBox <gi...@apache.org>.
yasserzamani commented on pull request #489:
URL: https://github.com/apache/struts/pull/489#issuecomment-856057675
Looks awesome thanks a lot!
Could you please also support this implementation with some official references? e.g. links for the tag pooling basics and also for similar implementations in well known frameworks or simply describe how you concluded with this solution - TBH a lot of null settings is weird and suspicious for me!
--
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:
users@infra.apache.org
[GitHub] [struts] JCgH4164838Gh792C124B5 commented on pull request #489: WW-5124 - Proposed fix for Struts JSP tag behaviour with tag pooling
Posted by GitBox <gi...@apache.org>.
JCgH4164838Gh792C124B5 commented on pull request #489:
URL: https://github.com/apache/struts/pull/489#issuecomment-927385594
Hi @yasserzamani .
Thanks for pointing out the possibility that `ThreadLocal` side-effects were at play. 👏
I implemented changes similar to what you suggested (but applied the clear/reset to `StrutsInternalTestCase` instead), and the automated build finally passed again.
This PR probably now has the elements needed for a flexible solution, but it is a lot more changes than I had envisioned at the start. I am not sure the tag control flag name is very elegant, and there may be some "rough edges", but the idea can be seen at least.
From recent discussion, it seems like the intent may be to re-target open PR changes against the 2.6 branch instead. It should be possible to do so with the ideas in this PR, but could take a bit of time .
--
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.
To unsubscribe, e-mail: issues-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [struts] yasserzamani edited a comment on pull request #489: WW-5124 - Proposed fix for Struts JSP tag behaviour with tag pooling
Posted by GitBox <gi...@apache.org>.
yasserzamani edited a comment on pull request #489:
URL: https://github.com/apache/struts/pull/489#issuecomment-856057675
@JCgH4164838Gh792C124B5 looks awesome thanks a lot!
Could you please also support this implementation with some official references? e.g. links for the tag pooling basics and also for similar implementations in well known frameworks or simply describe how you concluded with this solution - TBH a lot of null settings is weird and suspicious for me!
--
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:
users@infra.apache.org
[GitHub] [struts] lukaszlenart commented on pull request #489: WW-5124 - Proposed fix for Struts JSP tag behaviour with tag pooling
Posted by GitBox <gi...@apache.org>.
lukaszlenart commented on pull request #489:
URL: https://github.com/apache/struts/pull/489#issuecomment-869679190
Great, thanks a lot! I will go ahead with 2.5.27 and then we can add this feature to 2.5.28 (I would like to focus on 2.6 but that's ok ).
--
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.
To unsubscribe, e-mail: issues-unsubscribe@struts.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org