Skip to content

Conversation

@buchgr
Copy link
Contributor

@buchgr buchgr commented Oct 12, 2016

Port the netty layer to use the Http2FrameCodec. This change requires netty/netty#5914, which is currently under reviews.

Performance has yet to be evaluated. I currentlly don't have hardware to run benchmarks on.

The plan is to maintain this in a separate branch, until the Http2FrameCodec API is mature and released and performance is on bar with the Http2ConnectionHandler API.

@ejona86 ejona86 self-assigned this Nov 1, 2016
@ejona86
Copy link
Member

ejona86 commented Aug 18, 2018

I updated this to current Netty and reduced the diff size in https://github.com/ejona86/grpc-java/tree/netty_http2_framecodec . Grep for FIXMEs. It currently will leak all streams, as it keeps them in a map and never removes them (since managedState isn't available). To remove them, it looks like we should override Abstract{Server,Client}Stream.TransportState.deframerClosed() but that's not available at the version of gRPC being modified here. It could be added fairly easily to this without upgrading grpc (see a446b53 where it was added), but meh.

I looked through the conflicts to update to grpc master, and there didn't appear to be anything too hard in the non-test code to merge such that we would want to go with a different approach. There were some non-trivial conflicts in the Handlers, but they also seems pretty self-contained and mostly clear what is going on.

@thelinuxfoundation
Copy link

Thank you for your pull request. Before we can look at your contribution, we need to ensure all contributors are covered by a Contributor License Agreement.

After the following items are addressed, please respond with a new comment here, and the automated system will re-verify.

Regards,
CLA GitHub bot

// Set the frame listener on the decoder.
decoder().frameListener(new FrameListener());
@Override
public void channelRead(ChannelHandlerContext ctx, Object msg) throws Exception {
Copy link
Member

Choose a reason for hiding this comment

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

This need to propagate the messages or make sure to release them (reference counting). Right now any unknown message is leaked.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants