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.

Sign up for GitHub

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

Jump to bottom

Issue #4713 Async dispatch with query. #4721

Merged
merged 3 commits into from
Mar 30, 2020
Merged

Issue #4713 Async dispatch with query. #4721

merged 3 commits into from
Mar 30, 2020
+170 −91

Conversation

gregw
Copy link
Contributor

@gregw gregw commented Mar 26, 2020

For issue #4713

  • Preserve the entire URI with query when startAsync(req,res) is used.
  • merge any query string from dispatch path with either original query or preserved query from forward

Signed-off-by: Greg Wilkins gregw@webtide.com

@gregw
Issue #4713 Async dispatch with query.
9831930 
+ Preserve the entire URI with query when startAsync(req,res) is used.
+ merge any query string from dispatch path with either original query or preserved query from forward

Signed-off-by: Greg Wilkins <gregw@webtide.com>
@gregw gregw requested review from olamy and lorban March 26, 2020 15:11
@gregw
Copy link
Contributor Author

gregw commented Mar 26, 2020

The main change was in two places:

In AsyncContextEvent we were previously using _dispatchPath to remember the path when startAsync(req,res) was called and to hold any path passed to AsyncContext.dispatch(String). We now have separate field for this and the remember path for startAsync is now a copy of the HttpURI which has the decoded elements including any query.

In Server.handleAsync we now consider the original URI (decoded with query), the optional remembered URI (decoded with query) and the optional dispatch path (encoded with query). The query is merged of necessary and the request parameters reset.

lorban
lorban reviewed Mar 27, 2020
View reviewed changes
jetty-server/src/main/java/org/eclipse/jetty/server/Request.java Outdated
@@ -2165,7 +2168,7 @@ public AsyncContext startAsync() throws IllegalStateException
HttpChannelState state = getHttpChannelState();
if (_async == null)
_async = new AsyncContextState(state);
AsyncContextEvent event = new AsyncContextEvent(_context, _async, state, this, this, getResponse());
AsyncContextEvent event = new AsyncContextEvent(_context, _async, state, this, this, getResponse(), false);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if you shouldn't keep the old constructor here, and not use an extra boolean argument.

@lorban
Copy link
Contributor

lorban commented Mar 27, 2020

I added a few nitpicking comments, but overall I get what this change is about and it LGTM.

@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 Mar 30, 2020
@gregw gregw mentioned this pull request Mar 30, 2020
Closed
@gregw gregw requested a review from lorban March 30, 2020 13:57
@gregw
Copy link
Contributor Author

gregw commented Mar 30, 2020

Ah @lorban can't give me the green tick (yet)! But I'll assume it anyway and merge!

@gregw gregw merged commit e3d670d into jetty-10.0.x Mar 30, 2020
@gregw gregw deleted the jetty-10.0.x-4713-asyncDispatchQuery branch March 30, 2020 14:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@olamy olamy Awaiting requested review from olamy

@lorban lorban Awaiting requested review from lorban

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@gregw @lorban