Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Tomcat crossContext #63

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

shaneargo
Copy link

This pull request replaced pull request #62. After discussions with @rgrashel, I believe this is a better resolution to the issue.

This resolves issues related to the use of Tomcat's crossContext configuration item. When this config item is set to true a single request may include/forward control across servlets from different contexts. Which results in two different issues.

The first is that the final actionBean in the stack is left in the actionBean request attribute at the end, so when Stripes (running in a second context) attempts to service a request there is already a bean in this attribute. This is resolved by clearing the request attribute when the stack is empty. This make sense as the idea behind the restoreActionBean method is to remove the current action bean and restore the existing one. This change means that it clears the current bean (as expected) even when there is nothing left in the stack to put back in its place.

The second issue is that URL bindings may not be unique across contexts. This is a problem when stripes stores the actionBean for a URL in the session. This is resolved by prepending the context path to the request attribute name.

As with the last pull request, it is difficult to write tests for this as none of the mock round trip infrastructure was written with crossContext in mind. I could write some tests that check that the actionBean request attribute has been unset after servicing a request, but I feel this would be testing implementation, instead of behaviour. Please let me know your feelings on this.

All existing tests pass.

Please let me know if there is anything you'd like me to change or clarify.

Cheers,
Shane.

Tomcat's crossContext configuration item means that a single request may be forwarded to multiple servlets. This clears the actionBean request attribute when the stack is empty so that the next servlet is not polluted with an action bean from the previous servlet.

Some of the tests assumed that the last action bean to be restored from the stack would remain in the actionBean request attribute. To resolve this, a new interceptor was introduced for testing which saves out the last action bean prior to it being cleared.
Tomcat's crossContext configuration item means that a single request may be forwarded to multiple servlets. If multiple instances of stripes are running in different webapps in the same container, with this configuration it is possible that they'll share a context, and thus the URL binding may collide.

The MockRoundtrip class expected the attribute key to be the URL Binding, so this was updated to align with this change.
@rgrashel
Copy link
Member

Hi Shane,

I actually did some digging in old mailing list posts regarding cross-context and the actionbean stack. As you originally stated, while this code probably deals with the crossContext issue better than the previous pull request, I think we really need to look more deeply at the consequences of this. After doing some research, the actionbean stack was very intentionally made to behave the way it does. I also believe that leaving the actionbean in the request was very intentional. Likewise, UrlBinding is rock-solid right now and I am very hesitant to change this behavior right before a release.

Based on what I read in some old mailing list issues and looking through old JIRA tickets, we could definitely see a problem if a "parent" JSP page does a jsp:include of a stripes action bean... and then the "parent"... tries to access the actionbean created by the child. In that situation, it would appear that the actionbean would be null. Which we don't want. So I think deleting the actionbean from the request in the DispatcherServlet is probably not correct.

I also need to look closer at your proposal of unconditionally prepending the context to the UrlBinding -- after the Dispatcher does all of its work. Again, I am just unsure of the side effects or possible bugs. We're basically saying, a UrlBinding will never exist without a contextName as a prefix. There is a lot of code that people have already written which depends on the behavior of UrlBinding, so it really makes me uneasy trying to change that in the core. It's just scary to do this right before release.

I'm actually thinking a less invasive solution that meets all of your needs could be the following:

  1. Instead of changing DispatcherServlet to do the hardcoded prefixing, write your own ActionResolver that extends NameBasedActionResolver. Override the getUrlBinding() method so that it always prepends the contextName to the UrlBinding. Then, put that ActionResolver in your web.xml as a custom action resolver -- using the ActionResolver.Class init-param.

  2. If you want to remove the actionbean from the request after it completes, then write your own Interceptor which triggers on LifecycleStage.RequestComplete. In your interceptor, delete the actionbean stack from the request -- and delete the actionbean itself if you want. That will result in DispaterServlet.restoreActionBean() having no effect. But I still think you may have issues if you use jsp:include ... but I'm not sure.

So basically, I think a custom action resolver and a very small interceptor gives you the custom behavior for your portlet/cross-context situation, and it uses the power of Stripes pluggable componentry to implement that behavior without disrupting the core code at all.

@shaneargo
Copy link
Author

Hi Rick,

I understand your desire not to break things.

For documentation sake, this change was to prevent other peoples apps, whose source I do not control, which are running in the same container, from breaking mine. Unless each discrete developer implements the changes that you've suggested then it will break when running those apps with crossContext=true.

Not supporting crossContext=true very well might be a limitation you are willing to accept, and I'd understand that decision.

Cheers,
Shane.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants