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

Fix NPE in Http1ClientCodec #2210

Merged
merged 2 commits into from
Oct 24, 2019
Merged

Fix NPE in Http1ClientCodec #2210

merged 2 commits into from
Oct 24, 2019

Conversation

minwoox
Copy link
Member

@minwoox minwoox commented Oct 24, 2019

Motivation:
If the remote peer sends multiple responses for one request, which is not allowed by the spec but may still be possible,
NPE can be raised in Http1ClientCodec.

Modification:

  • Copy Http1ClientCodec from the upstream Netty 4.1.42 at 39cc7a673939dec96258ff27f5b1874671838af0

Result:

  • No more NPE in Http1ClientCodec

Motivation:
If the remote peer sends multiple responses for one request, which is not allowed by the spec but may still be possible,
NPE can be raised in `Http1ClientCodec`.

Modification:
- Copy `Http1ClientCodec` from the upstream Netty 4.1.42 at 39cc7a673939dec96258ff27f5b1874671838af0

Result:
- No more NPE in `Http1ClientCodec`
@minwoox minwoox added the defect label Oct 24, 2019
@minwoox minwoox added this to the 0.95.0 milestone Oct 24, 2019
@ikhoon
Copy link
Contributor

ikhoon commented Oct 24, 2019

Related: netty/netty#9465

Copy link
Contributor

@ikhoon ikhoon left a comment

Choose a reason for hiding this comment

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

Thanks @minwoox !

@codecov
Copy link

codecov bot commented Oct 24, 2019

Codecov Report

Merging #2210 into master will decrease coverage by 0.02%.
The diff coverage is 18.18%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #2210      +/-   ##
============================================
- Coverage     73.66%   73.64%   -0.03%     
- Complexity     9610     9614       +4     
============================================
  Files           839      839              
  Lines         37000    37001       +1     
  Branches       4557     4558       +1     
============================================
- Hits          27256    27249       -7     
- Misses         7418     7427       +9     
+ Partials       2326     2325       -1
Impacted Files Coverage Δ Complexity Δ
...om/linecorp/armeria/internal/Http1ClientCodec.java 48.27% <18.18%> (+0.6%) 6 <0> (ø) ⬇️
...om/linecorp/armeria/client/HttpSessionHandler.java 61.01% <0%> (-8.48%) 29% <0%> (-2%)
...meria/common/stream/RegularFixedStreamMessage.java 81.63% <0%> (-6.13%) 13% <0%> (-1%)
...necorp/armeria/server/GracefulShutdownSupport.java 97.43% <0%> (-2.57%) 4% <0%> (ø)
...a/com/linecorp/armeria/client/HttpChannelPool.java 72.76% <0%> (-2.34%) 59% <0%> (-2%)
...inecorp/armeria/server/grpc/ArmeriaServerCall.java 87.5% <0%> (+0.39%) 86% <0%> (+1%) ⬆️
...com/linecorp/armeria/server/HttpServerHandler.java 82.05% <0%> (+0.66%) 83% <0%> (+1%) ⬆️
...m/linecorp/armeria/client/HttpResponseDecoder.java 83.62% <0%> (+0.86%) 19% <0%> (ø) ⬇️
...inecorp/armeria/server/HttpResponseSubscriber.java 85.47% <0%> (+1.28%) 76% <0%> (+3%) ⬆️
.../com/linecorp/armeria/server/docs/ServiceInfo.java 80.7% <0%> (+1.75%) 21% <0%> (+1%) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e8c424a...6d29c1f. Read the comment docs.

@minwoox minwoox merged commit 00c0430 into line:master Oct 24, 2019
@minwoox minwoox deleted the fix_NPE branch October 24, 2019 06:58
@minwoox
Copy link
Member Author

minwoox commented Oct 24, 2019

Thanks for reviewing!

normanmaurer pushed a commit to netty/netty that referenced this pull request Oct 28, 2019
Motivation:

HTTP 102 (WebDAV) is not correctly treated as an informational response

Modification:

Delegate all `1XX` status codes to superclass, not just `100` and `101`.

Result:

Supports WebDAV response.
Removes a huge maintenance [headache](line/armeria#2210) in Armeria which has forked the class for these features
normanmaurer pushed a commit to netty/netty that referenced this pull request Oct 28, 2019
Motivation:

HTTP 102 (WebDAV) is not correctly treated as an informational response

Modification:

Delegate all `1XX` status codes to superclass, not just `100` and `101`.

Result:

Supports WebDAV response.
Removes a huge maintenance [headache](line/armeria#2210) in Armeria which has forked the class for these features
eugene70 pushed a commit to eugene70/armeria that referenced this pull request Nov 10, 2019
* Fix NPE in Http1ClientCodec
Motivation:
If the remote peer sends multiple responses for one request, which is not allowed by the spec but may still be possible,
NPE can be raised in `Http1ClientCodec`.

Modification:
- Copy `Http1ClientCodec` from the upstream Netty 4.1.42 at 39cc7a673939dec96258ff27f5b1874671838af0

Result:
- No more NPE in `Http1ClientCodec`
fmguerreiro pushed a commit to fmguerreiro/armeria that referenced this pull request Sep 19, 2020
* Fix NPE in Http1ClientCodec
Motivation:
If the remote peer sends multiple responses for one request, which is not allowed by the spec but may still be possible,
NPE can be raised in `Http1ClientCodec`.

Modification:
- Copy `Http1ClientCodec` from the upstream Netty 4.1.42 at 39cc7a673939dec96258ff27f5b1874671838af0

Result:
- No more NPE in `Http1ClientCodec`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants