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.

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

Set stream ID to the real value in Http2StreamFrameToHttpObjectCodec #7778

Closed
wants to merge 1 commit into from
Closed

Set stream ID to the real value in Http2StreamFrameToHttpObjectCodec #7778

wants to merge 1 commit into from

Conversation

jprante
Copy link

@jprante jprante commented Mar 13, 2018

Motivation:

Http2StreamFrameToHttpObjectCodec is useful to convert HTTP/2 streams to Netty's HTTP requests/responses. But the HTTP/2 stream ID is always 0 after conversion.

Modification:

Instead of always using 0 for the stream ID, the stream ID from frame.stream().id() is used.

Result:

The stream ID is set from the HTTP/2 stream and retained after conversion. This can be useful for users who wants to track HTTP/2 requests/responses, for example via header x-http2-stream-id.

This is a single line change. I think the contribution is so trivial that I did not sign the CLA yet.

@normanmaurer
Copy link
Member

normanmaurer commented Mar 13, 2018 via email

@jprante
Copy link
Author

jprante commented Mar 14, 2018

Yes, but it will take a little while. Seems tests are throwing NPE, not sure why.

@normanmaurer
Copy link
Member

@jprante I guess frame.stream() returns null which may be because of how the tests are setup. Please fix the tests and ping me once done.

normanmaurer added a commit that referenced this pull request Mar 16, 2018
…o HttpMessage

Motivation:

We did not correctly set the stream id in the headers of HttpMessage when converting a Http2HeadersFrame. This is based on #7778 so thanks to @jprante.

Modifications:

- Correctly set the id when possible in the header.
- Add test case

Result:

Correctly include stream id.
@normanmaurer
Copy link
Member

@jprante please check #7786.

@normanmaurer
Copy link
Member

And thanks for the fix ...

normanmaurer added a commit that referenced this pull request Mar 17, 2018
…o HttpMessage

Motivation:

We did not correctly set the stream id in the headers of HttpMessage when converting a Http2HeadersFrame. This is based on #7778 so thanks to @jprante.

Modifications:

- Correctly set the id when possible in the header.
- Add test case

Result:

Correctly include stream id.
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.

None yet

2 participants