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

Not add inboundStreamHandler for outbound streams created by Http2Mul… #7180

Closed
wants to merge 1 commit into
base: 4.1
from

Conversation

Projects
None yet
3 participants
@normanmaurer
Member

normanmaurer commented Sep 5, 2017

…tiplexCodec.

Motivation:

We must not add the inboundStreamHandler for outbound streams creates by Http2MultiplexCodec as the user will specify a handler via Http2StreamChannelBootstrap.

Modifications:

  • Check if the stream is for outbound and if so not add the inboundStreamHandler to the pipeline
  • Update tests so this is covered.

Result:

Fixes [#7178]

Not add inboundStreamHandler for outbound streams created by Http2Mul…
…tiplexCodec.

Motivation:

We must not add the inboundStreamHandler for outbound streams creates by Http2MultiplexCodec as the user will specify a handler via Http2StreamChannelBootstrap.

Modifications:

- Check if the stream is for outbound and if so not add the inboundStreamHandler to the pipeline
- Update tests so this is covered.

Result:

Fixes [#7178]

@normanmaurer normanmaurer requested a review from ejona86 Sep 5, 2017

@normanmaurer

This comment has been minimized.

Show comment
Hide comment
@normanmaurer
Member

normanmaurer commented Sep 5, 2017

Also @Apache9.

@ejona86

ejona86 approved these changes Sep 5, 2017

@ejona86

This comment has been minimized.

Show comment
Hide comment
@ejona86

ejona86 Sep 5, 2017

Contributor

This seems fine for now. I think we'll eventually want to improve Http2StreamChannelBootstrap so that the handler is added before the channel is registered (e.g., we could pass handler to newOutboundStream()). But it looks like Http2StreamChannelBootstrap could use other improvements during init(), like how late it sets channel options/attributes. It'll probably need a refactor later.

Contributor

ejona86 commented Sep 5, 2017

This seems fine for now. I think we'll eventually want to improve Http2StreamChannelBootstrap so that the handler is added before the channel is registered (e.g., we could pass handler to newOutboundStream()). But it looks like Http2StreamChannelBootstrap could use other improvements during init(), like how late it sets channel options/attributes. It'll probably need a refactor later.

@normanmaurer

This comment has been minimized.

Show comment
Hide comment
@normanmaurer

normanmaurer Sep 5, 2017

Member
Member

normanmaurer commented Sep 5, 2017

@Apache9

This comment has been minimized.

Show comment
Hide comment
@Apache9

Apache9 Sep 5, 2017

Contributor

+1

Contributor

Apache9 commented Sep 5, 2017

+1

@normanmaurer

This comment has been minimized.

Show comment
Hide comment
@normanmaurer

normanmaurer Sep 6, 2017

Member

Cherry-picked into 4.1 (870b5f5)

Member

normanmaurer commented Sep 6, 2017

Cherry-picked into 4.1 (870b5f5)

@normanmaurer normanmaurer deleted the http2_multiplex_outbound branch Sep 6, 2017

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