-
-
Notifications
You must be signed in to change notification settings - Fork 8.6k
Add Http/2 support in Netty Server #8815
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
Conversation
8d7bb0c to
9497ec6
Compare
shs96c
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can probably simplify the logic of the stream initialisation, and make better use of the existing code we have, particularly when it comes to converting from Netty's http primitives to Selenium's.
java/server/src/org/openqa/selenium/netty/server/Http2ResponseConverter.java
Outdated
Show resolved
Hide resolved
| import org.openqa.selenium.remote.http.HttpMethod; | ||
| import org.openqa.selenium.remote.http.HttpRequest; | ||
|
|
||
| public class Http2RequestConverter extends SimpleChannelInboundHandler<HttpObject> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is mirroring the functionality of the RequestConverter, but requires http object aggregation in order to work, and so is likely to lead to problems when there's very little memory. In addition, it duplicates logic from that class, making fixing discovered issues more work and opening us up to forgetting to change both places.
I'd suggest deleting this and using RequestConverter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for pointing it out. Since Http/2 uses frames with stream ids, I am leveraging to Http2StreamFrameToHttpObjectCodec get the HttpObject to pass it to Selenium handlers. I have moved the Http/2 request handling to RequestConverter.
java/server/src/org/openqa/selenium/netty/server/Http2ResponseConverter.java
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| private void addHttp2SpecificHandlers(ChannelPipeline pipeline) { | ||
| pipeline.addLast("http2-codec", new Http2StreamFrameToHttpObjectCodec(true)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These should be added to the SeleniumInitializer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are using Http2MultiplexHandler for Http/2 support of streams. It leverages child channels of Netty. The handler takes in only a single handler to be added for child channels and their respective channel context. In order to add multiple handlers, Http2StreamInitializer was created. An instance of this sharable handler is passed while creating Http2MultiplexHandler.
java/server/src/org/openqa/selenium/netty/server/SeleniumHttp2Handler.java
Outdated
Show resolved
Hide resolved
java/server/src/org/openqa/selenium/netty/server/SeleniumHttp2Handler.java
Outdated
Show resolved
Hide resolved
9497ec6 to
31907ee
Compare
…request conversion. Move stream id check to SeleniumHandler.
82aaf1b to
b7245eb
Compare
Description
Add support for Http/2 in the Netty server. It is related to #8719.
Motivation and Context
Current Netty Server supports Http/1.1 for secure communication. The changes add support for Http/2 for a client requesting the use of Http/2.
This will allow clients to leverage features of Http/2 where a single connection is used along with streams for communication.
Netty supports stream multiplexing but leveraging child channels i.e. every new stream has a dedicated child channel.
Types of changes
Checklist