Skip to content

Fix async request cancellation #1112

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

Merged
merged 1 commit into from
Mar 7, 2019
Merged

Conversation

dagnir
Copy link
Contributor

@dagnir dagnir commented Mar 5, 2019

Description

  • Correctly pass along the cancelled signal down to the async client if the
    future returned for an async operation is cancelled.

  • Don't wait for acquire to finish to cancel; instead fail the promise if not
    complete yet

    CancellableAcquireChannelPool added to take care of closing and releasing a
    cancelled acquire.

Motivation and Context

Bug fix.

Testing

Added/updated tests in codegen, codegen-generated-classes-test, and netty-nio-client.

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

Copy link
Contributor

@varunnvs92 varunnvs92 left a comment

Choose a reason for hiding this comment

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

Nice work.

@dagnir dagnir force-pushed the async-request-cancel-fix branch from 80ffda9 to 2bfe7c7 Compare March 6, 2019 22:56
CompletableFuture<AllTypesResponse> responseFuture = client.allTypes(r -> {});
responseFuture.cancel(true);
Thread.sleep(500);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is 500 sufficient or will this cause transient failures?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure, I'm just trying to figure out if this will fix the test fails in Travis ATM. I'll probably increase to 1s if it works

@dagnir dagnir force-pushed the async-request-cancel-fix branch 2 times, most recently from 266f50e to d176350 Compare March 7, 2019 00:56
@codecov-io
Copy link

codecov-io commented Mar 7, 2019

Codecov Report

Merging #1112 into master will increase coverage by 0.03%.
The diff coverage is 87.65%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1112      +/-   ##
============================================
+ Coverage     58.49%   58.52%   +0.03%     
- Complexity     4494     4507      +13     
============================================
  Files           737      738       +1     
  Lines         22663    22715      +52     
  Branches       1690     1698       +8     
============================================
+ Hits          13256    13295      +39     
- Misses         8718     8729      +11     
- Partials        689      691       +2
Impacted Files Coverage Δ Complexity Δ
.../AsyncExecutionFailureExceptionReportingStage.java 92.3% <100%> (+1.39%) 4 <1> (ø) ⬇️
...eline/stages/AsyncApiCallTimeoutTrackingStage.java 100% <100%> (ø) 5 <0> (ø) ⬇️
...dk/core/client/handler/BaseAsyncClientHandler.java 84.31% <100%> (+0.64%) 9 <2> (ø) ⬇️
...awssdk/http/nio/netty/internal/RequestContext.java 100% <100%> (ø) 6 <2> (+1) ⬆️
...awssdk/http/nio/netty/NettyNioAsyncHttpClient.java 65.49% <100%> (-1.89%) 20 <0> (ø)
...internal/http/pipeline/RequestPipelineBuilder.java 100% <100%> (ø) 10 <0> (ø) ⬇️
...rnal/http/pipeline/stages/AsyncRetryableStage.java 84.72% <100%> (-2.24%) 3 <0> (ø)
.../http/nio/netty/internal/NettyRequestExecutor.java 65.55% <64.7%> (-0.92%) 28 <5> (ø)
...re/amazon/awssdk/utils/CompletableFutureUtils.java 36.36% <80%> (+36.36%) 2 <2> (+2) ⬆️
.../netty/internal/CancellableAcquireChannelPool.java 89.47% <89.47%> (ø) 8 <8> (?)
... and 7 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 360b8b0...84714ac. Read the comment docs.

 - Correctly pass along the cancelled signal down to the async client if the
   future returned for an async operation is cancelled.

 - Don't wait for acquire to finish to cancel; instead fail the promise if not
   complete yet

   CancellableAcquireChannelPool added to take care of closing and releasing a
   cancelled acquire.
@dagnir dagnir force-pushed the async-request-cancel-fix branch from d176350 to 84714ac Compare March 7, 2019 01:28
@dagnir dagnir merged commit 1e9b309 into aws:master Mar 7, 2019
aws-sdk-java-automation pushed a commit that referenced this pull request Dec 17, 2020
…3363cff76

Pull request: release <- staging/0e6dd94a-3bd9-40aa-a91e-77f3363cff76
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