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

Http2StreamFrameToHttpObjectCodec should handle 100-Continue properly #7334

Closed
wants to merge 1 commit into
base: 4.1
from

Conversation

@lionelli1
Contributor

lionelli1 commented Oct 23, 2017

Motivation:
Http2StreamFrameToHttpObjectCodec was not properly encoding and
decoding 100-Continue HttpResponse/Http2SettingsFrame properly. It was
encoding 100-Continue FullHttpResponse as an Http2SettingFrame with
endStream=true, causing the child channel to terminate. It was not
decoding 100-Continue Http2SettingsFrame (endStream=false) as
FullHttpResponse. This should be fixed as it would cause http2 child
stream to prematurely close, and could cause HttpObjectAggregator to
fail if it's in the pipeline.

Modification:

  • Fixed encode() to properly encode 100-Continue FullHttpResponse as
    Http2SettingsFrame with endStream=false
  • Fixed decode() to properly decode 100-Continue Http2SettingsFrame
    (endStream=false) as a FullHttpResponse

Result:
Now Http2StreamFrameToHttpObjectCodec should be properly handling
100-Continue responses.

@lionelli1

This comment has been minimized.

Show comment
Hide comment
@lionelli1

lionelli1 Oct 23, 2017

Contributor

@normanmaurer @Scottmitch PTAL thanks!

Contributor

lionelli1 commented Oct 23, 2017

@normanmaurer @Scottmitch PTAL thanks!

@normanmaurer

This comment has been minimized.

Show comment
Hide comment
@normanmaurer

normanmaurer Oct 24, 2017

Member

@lionelli1 also it seems there is a resource leak which means you not release the buffer correctly.

Please run with mvn -Pleak clean package and fix it.

Member

normanmaurer commented Oct 24, 2017

@lionelli1 also it seems there is a resource leak which means you not release the buffer correctly.

Please run with mvn -Pleak clean package and fix it.

@normanmaurer

This comment has been minimized.

Show comment
Hide comment
@normanmaurer

normanmaurer Oct 24, 2017

Member

@carl-mastrangelo I need to investigate but I think there may be something wrong with your leak-detector changes as we had no trace logged for the leak report here:

https://garage.netty.io/teamcity/viewLog.html?buildId=25308&buildTypeId=pulls_netty&guest=1

Member

normanmaurer commented Oct 24, 2017

@carl-mastrangelo I need to investigate but I think there may be something wrong with your leak-detector changes as we had no trace logged for the leak report here:

https://garage.netty.io/teamcity/viewLog.html?buildId=25308&buildTypeId=pulls_netty&guest=1

@normanmaurer

This comment has been minimized.

Show comment
Hide comment
@normanmaurer

normanmaurer Oct 24, 2017

Member

@carl-mastrangelo never mind.... I was blind and need more coffee ;) All good

Member

normanmaurer commented Oct 24, 2017

@carl-mastrangelo never mind.... I was blind and need more coffee ;) All good

@lionelli1

This comment has been minimized.

Show comment
Hide comment
@lionelli1

lionelli1 Oct 24, 2017

Contributor

@normanmaurer fixed the leak. thanks for the pointer!

Contributor

lionelli1 commented Oct 24, 2017

@normanmaurer fixed the leak. thanks for the pointer!

@normanmaurer normanmaurer self-assigned this Oct 24, 2017

@normanmaurer normanmaurer added this to the 4.1.17.Final milestone Oct 24, 2017

Http2StreamFrameToHttpObjectCodec should handle 100-Continue properly
Motivation:
Http2StreamFrameToHttpObjectCodec was not properly encoding and
decoding 100-Continue HttpResponse/Http2SettingsFrame properly. It was
encoding 100-Continue FullHttpResponse as an Http2SettingFrame with
endStream=true, causing the child channel to terminate. It was not
decoding 100-Continue Http2SettingsFrame (endStream=false) as
FullHttpResponse. This should be fixed as it would cause http2 child
stream to prematurely close, and could cause HttpObjectAggregator to
fail if it's in the pipeline.

Modification:
- Fixed encode() to properly encode 100-Continue FullHttpResponse as
  Http2SettingsFrame with endStream=false
- Reject 100-Continue HttpResponse that are NOT FullHttpResponse
- Fixed decode() to properly decode 100-Continue Http2SettingsFrame
  (endStream=false) as a FullHttpResponse
- made Http2StreamFrameToHttpObjectCodec sharable so that it can b used
  among child streams within the same Http2MultiplexCodec

Result:
Now Http2StreamFrameToHttpObjectCodec should be properly handling
100-Continue responses.
@normanmaurer

This comment has been minimized.

Show comment
Hide comment
@normanmaurer

normanmaurer Oct 25, 2017

Member

@lionelli1 thanks... Cherry-picked into 4.1 (424bb09)

Member

normanmaurer commented Oct 25, 2017

@lionelli1 thanks... Cherry-picked into 4.1 (424bb09)

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