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

Throws IOException for an h2 connection acquiring race condition #1614

Merged
merged 1 commit into from
Jan 24, 2020

Conversation

zoewangg
Copy link
Contributor

@zoewangg zoewangg commented Jan 24, 2020

…s picked up at the same time it gets closed

Description

  • Reduce the chances of the race condition where an HTTP2 connection gets reused at the same time it gets inactive

  • Throw IOException instead of IllegalStatementException so the failed requests can be retried.

Testing

Added unit tests

Screenshots (if appropriate)

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

Checklist

  • I have read the CONTRIBUTING document
  • Local run of mvn install succeeds
  • My code follows the code style of this project
  • My change requires a change to the Javadoc documentation
  • I have updated the Javadoc documentation accordingly
  • I have read the README document
  • I have added tests to cover my changes
  • All new and existing tests passed
  • A short description of the change has been added to the CHANGELOG
  • My change is to implement 1.11 parity feature and I have updated LaunchChangelog

License

  • I confirm that this pull request can be released under the Apache 2 license

@zoewangg zoewangg force-pushed the zoewang-closedChannelException branch from b4b739d to e9d93a4 Compare January 24, 2020 20:15
if (!connection.isActive()) {
return false;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we know that this actually helps, or are we just guessing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As discussed offline, the check does not cover for all cases and we need to figure out a better way to reduce the race condition. Removed it for now.

… gets picked up at the same time it gets closed
@zoewangg zoewangg force-pushed the zoewang-closedChannelException branch from e9d93a4 to ca6fb14 Compare January 24, 2020 20:57
@zoewangg zoewangg changed the title Reduce the chance of the race condition where an http2 connection get… Throws IOException for an h2 connection acquiring race condition Jan 24, 2020
@sonarqubecloud
Copy link

SonarCloud Quality Gate failed.

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@zoewangg zoewangg merged commit fc58000 into master Jan 24, 2020
@zoewangg zoewangg deleted the zoewang-closedChannelException branch January 24, 2020 21:20
@codecov-io
Copy link

codecov-io commented Jan 24, 2020

Codecov Report

Merging #1614 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1614      +/-   ##
============================================
+ Coverage     75.69%   75.69%   +<.01%     
  Complexity      682      682              
============================================
  Files           905      905              
  Lines         28389    28389              
  Branches       2257     2257              
============================================
+ Hits          21488    21490       +2     
+ Misses         5880     5877       -3     
- Partials       1021     1022       +1
Flag Coverage Δ Complexity Δ
#unittests 75.69% <100%> (ø) 682 <0> (ø) ⬇️
Impacted Files Coverage Δ Complexity Δ
...netty/internal/http2/MultiplexedChannelRecord.java 77.5% <100%> (+4.16%) 0 <0> (ø) ⬇️
...nio/netty/internal/OldConnectionReaperHandler.java 81.81% <0%> (-9.1%) 0% <0%> (ø)

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 d21070c...ca6fb14. Read the comment docs.

aws-sdk-java-automation added a commit that referenced this pull request Aug 17, 2021
…3b717e14f

Pull request: release <- staging/31290513-fea1-4560-8201-54a3b717e14f
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