Skip to content

Conversation

@pujagani
Copy link
Contributor

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

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • I have read the contributing document.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@pujagani pujagani changed the title Add Netty support for Http/2 Add Http/2 in Netty Server Oct 23, 2020
@pujagani pujagani changed the title Add Http/2 in Netty Server Add Http/2 support in Netty Server Oct 23, 2020
@pujagani pujagani force-pushed the http2_netty_multiplexing branch from 8d7bb0c to 9497ec6 Compare October 23, 2020 07:49
Copy link
Member

@shs96c shs96c left a 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.

import org.openqa.selenium.remote.http.HttpMethod;
import org.openqa.selenium.remote.http.HttpRequest;

public class Http2RequestConverter extends SimpleChannelInboundHandler<HttpObject> {
Copy link
Member

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

Copy link
Contributor Author

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.

}

private void addHttp2SpecificHandlers(ChannelPipeline pipeline) {
pipeline.addLast("http2-codec", new Http2StreamFrameToHttpObjectCodec(true));
Copy link
Member

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

Copy link
Contributor Author

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.

@pujagani pujagani force-pushed the http2_netty_multiplexing branch from 9497ec6 to 31907ee Compare November 4, 2020 12:21
@pujagani pujagani force-pushed the http2_netty_multiplexing branch from 82aaf1b to b7245eb Compare November 4, 2020 13:54
@barancev barancev added the B-grid Everything grid and server related label Jan 24, 2021
@github-actions github-actions bot added the Stale label Jan 24, 2022
@github-actions github-actions bot closed this Feb 8, 2022
@titusfortner titusfortner added J-stale Applied to issues that become stale, and eventually closed. and removed Stale labels Mar 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

B-grid Everything grid and server related J-stale Applied to issues that become stale, and eventually closed.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants