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

@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 In progress in Jetty 10.0.1 via automation 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
Jetty 10.0.1 automation moved this from In progress to Review in progress Jan 28, 2021
@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
Jetty 10.0.1 automation moved this from Review in progress to Reviewer approved Feb 2, 2021
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
Jetty 10.0.1 automation moved this from Reviewer approved to Review in progress Feb 3, 2021
@lorban lorban requested a review from sbordet February 3, 2021 10:32
Jetty 10.0.1 automation moved this from Review in progress to Reviewer approved Feb 3, 2021
@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
Jetty 10.0.1 automation moved this from Reviewer approved to Done 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 added this to In progress in Jetty 11.0.1 via automation 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
@joakime joakime moved this from In progress to Done in Jetty 11.0.1 Feb 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gregw gregw gregw approved these changes

@sbordet sbordet sbordet approved these changes

@joakime joakime Awaiting requested review from joakime

Assignees

@lorban lorban

Labels
Enhancement
Projects
No open projects
Jetty 10.0.1
  
Done
Jetty 11.0.1
Done
Milestone
10.0.x
3 participants
@lorban @gregw @sbordet