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 #250 - Implement HTTP CONNECT for HTTP/2. #3539

Merged
merged 23 commits into from Aug 13, 2019
Merged

Issue #250 - Implement HTTP CONNECT for HTTP/2. #3539

merged 23 commits into from Aug 13, 2019

Conversation

sbordet
Copy link
Contributor

@sbordet sbordet commented Apr 9, 2019

#250.

Modified HTTP/2 implementation to support the CONNECT method.

Signed-off-by: Simone Bordet simone.bordet@gmail.com

@sbordet
Copy link
Contributor Author

sbordet commented Apr 9, 2019

@lachlan-roberts this is a draft pull request where I started the work to support #250 and ultimately #3537.

@joakime
Copy link
Contributor

joakime commented Apr 9, 2019

Is there any value in doing the jetty-http2 specifics on jetty-9.4.x?

@sbordet
Copy link
Contributor Author

sbordet commented Apr 9, 2019

Right now the ConnectHandler is hardcoded on HTTP/1.1.
Changing it to support also HTTP/2 may be a breaking change.
Greg and I discussed in the past that it won't make much sense to have 2 different mechanisms (one per protocol) to support the same thing.
That's mainly the reason I'm targetting this for Jetty 10.

@joakime
Copy link
Contributor

joakime commented Apr 9, 2019

fair enough

@gregw
Copy link
Contributor

gregw commented Apr 18, 2019

I think this should be implemented with a Tunnel abstraction.
Rather than co-opting the connection upgrade mechanism for HTTP/1, the ConnectHandler or other handlers should be able to make specific Tunnels and then pass them to their transport (in a similar way to upgrade). The specific transport would then be responsible for running the tunnel - so for HTTP/1 it would create a TunnelConnection that wraps the Tunnel and do a real upgrade. For HTTP/2 the Tunnel instance would be set as the IStream attachment and the onData listener would just know how to deliver data frames to/from a Tunnel. We would then have a HttpChannelTunnel, WebSocketTunnel and ConnectTunnel which would be transport independent.

@sbordet sbordet force-pushed the jetty-10.0.x-250-http_connect_for_http2 branch from 452bbd0 to 7743708 Compare April 30, 2019 14:28
@sbordet
Issue #250 - Implement HTTP CONNECT for HTTP/2.
cc54ca1 
Modified HTTP/2 implementation to support the CONNECT method.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
@sbordet
Issue #250 - Implement HTTP CONNECT for HTTP/2.
8e2da6b 
Implemented semantic defined by RFC 8441.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
@sbordet
First draft pass at the implementation of CONNECT for HTTP2.
d55d7a9 
Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
@sbordet
Second draft pass at the implementation of CONNECT for HTTP2.
090ee92 
Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
@sbordet sbordet force-pushed the jetty-10.0.x-250-http_connect_for_http2 branch from 7743708 to 090ee92 Compare May 3, 2019 17:24
@sbordet
Third draft pass at the implementation of CONNECT for HTTP2.
7cb9d37 
Implemented handling of client-side idle timeout.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
@sbordet
Small refactoring for the implementation of CONNECT for HTTP2.
c3553f1 
Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
@sbordet
Split HTTP2Channel into Client and Server.
fb3e0e4 
Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
@sbordet
Merged branch 'jetty-10.0.x' into 'jetty-10.0.x-250-http_connect_for_…
db43840 
…http2'.
@sbordet sbordet mentioned this pull request Jun 5, 2019
Merged
@gregw
Copy link
Contributor

gregw commented Jun 13, 2019

@sbordet what is the status of this? Should we merge it for an alpha 10?

@sbordet
Copy link
Contributor Author

sbordet commented Jun 13, 2019

@gregw there is still work to do, hopefully I'll finish today.

@sbordet
Copy link
Contributor Author

sbordet commented Jun 13, 2019

@gregw nope, I got a bunch of test failures and things to review and polish, so this won't make it for 10alpha0.

sbordet and others added 8 commits July 1, 2019 16:57
@sbordet
Merged branch 'jetty-10.0.x' into 'jetty-10.0.x-250-http_connect_for_…
daeb8c8 
…http2'.
@sbordet
Fourth draft pass at the implementation of CONNECT for HTTP2.
911e2aa 
Implemented handling of client-side idle timeout.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
@sbordet
Merged branch 'jetty-10.0.x' into 'jetty-10.0.x-250-http_connect_for_…
01ce4eb 
…http2'.
@sbordet
Fixed compilation errors after merge.
bbd2b9e 
Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
@sbordet
Issue #250 - Implement HTTP CONNECT for HTTP/2.
564d5d0 
Implemented section 8.3 of RFC 7540.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
@sbordet
Merged branch 'jetty-10.0.x' into 'jetty-10.0.x-250-http_connect_for_…
68c7869 
…http2'.
@olamy
fix javadoc failure
e423e35 
Signed-off-by: olivier lamy <oliver.lamy@gmail.com>
@olamy
some checkstyle fixes
7d31b1e 
Signed-off-by: olivier lamy <oliver.lamy@gmail.com>
@sbordet sbordet marked this pull request as ready for review July 10, 2019 07:34
@gregw
Copy link
Contributor

gregw commented Jul 11, 2019

@sbordet CI test failed?

@gregw
Copy link
Contributor

gregw commented Jul 11, 2019

This is too big to review without a walk through. Let's do it tomorrow.

@sbordet
Copy link
Contributor Author

sbordet commented Jul 22, 2019

@sbordet CI test failed?

Yes the issue is the idle timeout race between the EndPoint connected to the proxy and the HTTP/2 stream that tunnels requests to the proxy onto that EndPoint.
If the HTTP/2 stream idle timeout wins, we correctly send a DATA frame with endStream=true to the proxy to close the tunnel.
If the EndPoint idle timeout wins, we wrongly send a RST_STREAM to the proxy.

@sbordet
Merged branch 'jetty-10.0.x' into 'jetty-10.0.x-250-http_connect_for_…
51cb107 
…http2'.
@sbordet
Fixed racy test.
ceaa417 
Introduced HTTP2Client.streamIdleTimeout.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
@sbordet
Merged 'jetty-10.0.x' into 'jetty-10.0.x-250-http_connect_for_http2'.
a2d7a30
@sbordet
Issue #250 - Implement HTTP CONNECT for HTTP/2.
85b455d 
Implemented a few TODOs and improved logging.
Some small code cleanups.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
gregw
gregw approved these changes Aug 13, 2019
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 - certainly good enough to be merged to alpha jetty-10. Let's dogfood it from here.

jetty-http/src/main/java/org/eclipse/jetty/http/MetaData.java Outdated Show resolved Hide resolved
jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/HTTP2Channel.java Show resolved Hide resolved
jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannel.java Outdated Show resolved Hide resolved
@sbordet
Fixes #250 - Implement HTTP CONNECT for HTTP/2.
2dd5896 
Final modifications after review.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
@sbordet sbordet merged commit a700907 into jetty-10.0.x Aug 13, 2019
@sbordet sbordet deleted the jetty-10.0.x-250-http_connect_for_http2 branch August 13, 2019 16:07
@gsusanto gsusanto mentioned this pull request Jun 23, 2022
Closed
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

@lachlan-roberts lachlan-roberts Awaiting requested review from lachlan-roberts

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@sbordet @joakime @gregw @olamy