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

getHttpServletMapping for async dispatch #4741

Closed
gregw opened this issue Apr 1, 2020 · 4 comments · Fixed by #4851 or #4898
Closed

getHttpServletMapping for async dispatch #4741

gregw opened this issue Apr 1, 2020 · 4 comments · Fixed by #4851 or #4898
Assignees
Labels
Specification For all industry Specifications (IETF / Servlet / etc) TCK For various Specification Test Compatibility Kits (eg: Servlet, WebSocket, HTTP/2, etc)

Comments

@gregw
Copy link
Contributor

gregw commented Apr 1, 2020

Unlike the path methods (contextPath, servletPath, pathInfo), the new 4.0 getHttpServletMapping() method does not change when an AsyncContext.dispatch() is done, making it more like include.

@gregw gregw added Specification For all industry Specifications (IETF / Servlet / etc) TCK For various Specification Test Compatibility Kits (eg: Servlet, WebSocket, HTTP/2, etc) labels Apr 1, 2020
@gregw
Copy link
Contributor Author

gregw commented Apr 6, 2020

See jakartaee/servlet#309 for some clarification (hopefully?)

gregw added a commit that referenced this issue Apr 6, 2020
Added unit tests for expected behavior

Signed-off-by: Greg Wilkins <[email protected]>
@gregw
Copy link
Contributor Author

gregw commented Apr 18, 2020

I think this is related to the work that I'm doing in #4777, where I've made HttpURI, HttpFields and MetaData immutable. When doing dispataches the entire fields and uri instances are replace and then reverted, rather than their component parts.

I think a similar approach can be taken with pathInContext, servletPath, pathInfo and pathMapping. Rather than handling each individually, I think the request can carry an immutable PathInfo object that would have those 4 fields. It would be set and replaced as a whole when the request is scoped to a servlet and when it is dispatched to another target. More over, in the cases of exact matches, the PathInfo instant can be the same for all requests (associated with the ServletMapping?). There can be special types of PathInfo implementations for some of the bizarre cases outline in jakartaee/servlet#309

@gregw
Copy link
Contributor Author

gregw commented Apr 18, 2020

@lachlan-roberts to get your brain into this space, can you write a little webapp that can be deployed on tomcat and undertow to verify if they follow the TCK with respect to [2] in jakartaee/servlet#309

@gregw
Copy link
Contributor Author

gregw commented Apr 22, 2020

Note also that the current implementation of AsyncContext.dispatch() handles the request attributes required by the spec in an entirely different way to the attributes for Dispatcher. Testing by @lorban has indicated that for a simple async request, the creation of the attribute map is a significant source of garbage. So are part of this issue, it might be good to look at fixing those attributes as well.

lachlan-roberts added a commit that referenced this issue May 6, 2020
lachlan-roberts added a commit that referenced this issue May 6, 2020
lachlan-roberts added a commit that referenced this issue May 6, 2020
lachlan-roberts added a commit that referenced this issue May 7, 2020
lachlan-roberts added a commit that referenced this issue May 8, 2020
lachlan-roberts added a commit that referenced this issue May 13, 2020
Signed-off-by: Lachlan Roberts <[email protected]>
gregw added a commit that referenced this issue May 13, 2020
Greatly increased the scope of this PR by combining the servletPath and
pathInfo into the ServletPathMapping class that implements the
HttpServletPathMapping interface.    This allows us to greatly simplify
the matching of servlets and reduce the number of times we need to
actually to the match per request.

Signed-off-by: Greg Wilkins <[email protected]>
gregw added a commit that referenced this issue May 13, 2020
Fixed problems with previous commit
more cleanup of attributes in dispatcher.

Signed-off-by: Greg Wilkins <[email protected]>
@gregw gregw linked a pull request May 13, 2020 that will close this issue
gregw added a commit that referenced this issue May 13, 2020
More code cleanups

Signed-off-by: Greg Wilkins <[email protected]>
gregw added a commit that referenced this issue May 14, 2020
Named dispatch cleanup

Signed-off-by: Greg Wilkins <[email protected]>
gregw added a commit that referenced this issue May 14, 2020
misc cleanup

Signed-off-by: Greg Wilkins <[email protected]>
gregw added a commit that referenced this issue May 18, 2020
Added tests for named dispatchers
Do not use ServletPathMapping for named dispatch

Signed-off-by: Greg Wilkins <[email protected]>
gregw added a commit that referenced this issue May 18, 2020
renamed confusing isDefault method on ServletMapping

Signed-off-by: Greg Wilkins <[email protected]>
gregw added a commit that referenced this issue May 18, 2020
simplified setAttribute

Signed-off-by: Greg Wilkins <[email protected]>
gregw added a commit that referenced this issue May 18, 2020
added javadoc about AsyncAttributes

Signed-off-by: Greg Wilkins <[email protected]>
gregw added a commit that referenced this issue May 19, 2020
Fixed javadoc

Signed-off-by: Greg Wilkins <[email protected]>
gregw added a commit that referenced this issue May 20, 2020
* Issue #4741 - fixes to jetty implementation of HttpServletMapping

Signed-off-by: Lachlan Roberts <[email protected]>

* Issue #4741 - don't lazily generate HttpServletMapping to preserve servletName

Signed-off-by: Lachlan Roberts <[email protected]>

* Issue #4741 - tests should expect no leading / for matchValue

Signed-off-by: Lachlan Roberts <[email protected]>

* resolving TODOs from review

- removed pathSpec from Request
- getServletMapping moved to ServletHandler

Signed-off-by: Lachlan Roberts <[email protected]>

* Issue #4741 - only create HttpServletMapping for exact matches once

Signed-off-by: Lachlan Roberts <[email protected]>

* use wrapped attributes for async dispatch

Signed-off-by: Lachlan Roberts <[email protected]>

* Issue #4741 - Changes from review, revert async attribute wrapping

Signed-off-by: Lachlan Roberts <[email protected]>

* Issue #4741 - Changes from review

Signed-off-by: Lachlan Roberts <[email protected]>

* Issue #4741 Async ServletMapping

Greatly increased the scope of this PR by combining the servletPath and
pathInfo into the ServletPathMapping class that implements the
HttpServletPathMapping interface.    This allows us to greatly simplify
the matching of servlets and reduce the number of times we need to
actually to the match per request.

Signed-off-by: Greg Wilkins <[email protected]>

* Issue #4741 Async ServletMapping

Fixed problems with previous commit
more cleanup of attributes in dispatcher.

Signed-off-by: Greg Wilkins <[email protected]>

* Issue #4741 Async ServletMapping

More code cleanups

Signed-off-by: Greg Wilkins <[email protected]>

* Issue #4741 Async ServletMapping

Named dispatch cleanup

Signed-off-by: Greg Wilkins <[email protected]>

* Issue #4741 Async ServletMapping

misc cleanup

Signed-off-by: Greg Wilkins <[email protected]>

* Issue #4741 Async HttpServletMapping

Added tests for named dispatchers
Do not use ServletPathMapping for named dispatch

Signed-off-by: Greg Wilkins <[email protected]>

* Issue #4741 Async HttpServletMapping

renamed confusing isDefault method on ServletMapping

Signed-off-by: Greg Wilkins <[email protected]>

* Issue #4741 Async HttpServletMapping

simplified setAttribute

Signed-off-by: Greg Wilkins <[email protected]>

* Issue #4741 Async HttpServletMapping

added javadoc about AsyncAttributes

Signed-off-by: Greg Wilkins <[email protected]>

* Issue #4741 Async HttpServletMapping

Fixed javadoc

Signed-off-by: Greg Wilkins <[email protected]>

Co-authored-by: Greg Wilkins <[email protected]>
gregw added a commit that referenced this issue May 20, 2020
This completes the refactoring started in #4851, using
the HttpServletMapping field to avoid having the servletPath field
in the Request and instead have a pathInContext field.

Signed-off-by: Greg Wilkins <[email protected]>
gregw added a commit that referenced this issue May 21, 2020
reverted ResourceService changes

Signed-off-by: Greg Wilkins <[email protected]>
gregw added a commit that referenced this issue May 25, 2020
fixed gzip handler

Signed-off-by: Greg Wilkins <[email protected]>
gregw added a commit that referenced this issue May 26, 2020
Fixed several TODOs left in the code
removed _contextPath field and used an attributes lookup for include
replaced setContextPaths with setContext

Signed-off-by: Greg Wilkins <[email protected]>
gregw added a commit that referenced this issue May 27, 2020
Used the same pattern from the contextPath changes for servletPath and pathInfo.   Now the servletPathMapping is always set on the request and only if the dispatch is an include do the effected methods look deeper for the source values.

Signed-off-by: Greg Wilkins <[email protected]>
gregw added a commit that referenced this issue May 27, 2020
Improved javadoc

Signed-off-by: Greg Wilkins <[email protected]>
gregw added a commit that referenced this issue May 28, 2020
* Issue #4741 HttpServletMapping

This completes the refactoring started in #4851, using
the HttpServletMapping field to avoid having the servletPath field
in the Request and instead have a pathInContext field.

Signed-off-by: Greg Wilkins <[email protected]>

* Issue #4741 HttpServletMapping

reverted ResourceService changes

Signed-off-by: Greg Wilkins <[email protected]>

* Issue #4741 HttpServletMapping

fixed gzip handler

Signed-off-by: Greg Wilkins <[email protected]>

* Issue #4741 HttpServletMapping

Fixed several TODOs left in the code
removed _contextPath field and used an attributes lookup for include
replaced setContextPaths with setContext

Signed-off-by: Greg Wilkins <[email protected]>

* Issue #4741 HttpServletMapping

Used the same pattern from the contextPath changes for servletPath and pathInfo.   Now the servletPathMapping is always set on the request and only if the dispatch is an include do the effected methods look deeper for the source values.

Signed-off-by: Greg Wilkins <[email protected]>

* Issue #4741 HttpServletMapping

Improved javadoc

Signed-off-by: Greg Wilkins <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Specification For all industry Specifications (IETF / Servlet / etc) TCK For various Specification Test Compatibility Kits (eg: Servlet, WebSocket, HTTP/2, etc)
Projects
None yet
2 participants