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

Implementation of HttpServletRequest.upgrade #5926

Merged
merged 1 commit into from
Feb 3, 2021
Merged

Implementation of HttpServletRequest.upgrade #5926

merged 1 commit into from
Feb 3, 2021

Conversation

lorban
Copy link
Contributor

@lorban lorban commented Jan 28, 2021 •
edited
Loading

@lorban lorban requested a review from gregw January 28, 2021 15:04
@lorban lorban self-assigned this Jan 28, 2021
@lorban lorban added this to the 10.0.x milestone Jan 28, 2021
gregw
gregw requested changes Jan 28, 2021
View reviewed changes
Copy link
Contributor

@gregw gregw left a comment

Choose a reason for hiding this comment

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

This is exactly the kind of minimal implementation that I was imagining.
However I think that we need to:

  1. send a real 101 response
  2. stop the generator chunking the output and don't have a Transfer-Encoding header
  3. perhaps do a check or two

Details below

jetty-server/src/main/java/org/eclipse/jetty/server/Request.java Show resolved Hide resolved
jetty-server/src/main/java/org/eclipse/jetty/server/Request.java Show resolved Hide resolved
jetty-server/src/main/java/org/eclipse/jetty/server/Request.java Outdated Show resolved Hide resolved
@lorban lorban requested a review from gregw January 29, 2021 09:15
@lorban lorban changed the title hacky implementation of HttpServletRequest.upgrade that passes the TCK Implementation of HttpServletRequest.upgrade Jan 29, 2021
gregw
gregw requested changes Jan 29, 2021
View reviewed changes
Copy link
Contributor

@gregw gregw left a comment

Choose a reason for hiding this comment

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

Still looking good.... even the ugly dirty bits are not too ugly dirty.... but still some improvements and/or clarifications possible I think

jetty-http/src/main/java/org/eclipse/jetty/http/HttpGenerator.java Outdated Show resolved Hide resolved
jetty-server/src/main/java/org/eclipse/jetty/server/Request.java Show resolved Hide resolved
jetty-server/src/main/java/org/eclipse/jetty/server/Request.java Show resolved Hide resolved
jetty-server/src/main/java/org/eclipse/jetty/server/Response.java Outdated Show resolved Hide resolved
@lorban lorban force-pushed the jetty-10.0.x-tck-run-12-servlet-upgrade branch from d44a532 to 5222eb0 Compare January 29, 2021 15:56
@lorban lorban requested a review from gregw February 1, 2021 10:49
@lorban lorban requested a review from gregw February 1, 2021 13:28
@lorban lorban marked this pull request as ready for review February 2, 2021 07:52
gregw
gregw approved these changes Feb 2, 2021
View reviewed changes
Copy link
Contributor

@gregw gregw left a comment

Choose a reason for hiding this comment

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

LGTM. @sbordet your thoughts?

sbordet
sbordet requested changes Feb 3, 2021
View reviewed changes
jetty-server/src/main/java/org/eclipse/jetty/server/Request.java Show resolved Hide resolved
jetty-server/src/main/java/org/eclipse/jetty/server/Request.java Show resolved Hide resolved
jetty-server/src/main/java/org/eclipse/jetty/server/Request.java Outdated Show resolved Hide resolved
jetty-server/src/main/java/org/eclipse/jetty/server/Request.java Show resolved Hide resolved
jetty-server/src/main/java/org/eclipse/jetty/server/Request.java Outdated Show resolved Hide resolved
jetty-server/src/main/java/org/eclipse/jetty/server/Request.java Show resolved Hide resolved
jetty-servlet/src/test/java/org/eclipse/jetty/servlet/ServletUpgradeTest.java Outdated Show resolved Hide resolved
jetty-servlet/src/test/java/org/eclipse/jetty/servlet/ServletUpgradeTest.java Show resolved Hide resolved
@lorban lorban requested a review from sbordet February 3, 2021 10:32
@lorban
implementation of HttpServletRequest.upgrade
421ed6b 
Signed-off-by: Ludovic Orban <lorban@bitronix.be>
@lorban lorban force-pushed the jetty-10.0.x-tck-run-12-servlet-upgrade branch from 7868027 to 421ed6b Compare February 3, 2021 12:11
@lorban lorban merged commit 04cb3f3 into jetty-10.0.x Feb 3, 2021
@lorban lorban deleted the jetty-10.0.x-tck-run-12-servlet-upgrade branch February 3, 2021 12:11
@lorban lorban removed this from the 10.0.x milestone Feb 3, 2021
@lorban lorban added this to the 11.0.x milestone Feb 3, 2021
@lorban lorban modified the milestones: 11.0.x, 10.0.x Feb 3, 2021
@lorban lorban linked an issue Feb 15, 2021 that may be closed by this pull request
Implement minimal support for Servlet upgrade method #5596
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sbordet sbordet sbordet approved these changes

@gregw gregw gregw approved these changes

@joakime joakime Awaiting requested review from joakime

Assignees

@lorban lorban

Labels
Enhancement
Projects
None yet
Milestone
10.0.x
3 participants
@lorban @gregw @sbordet