fix(smithy-core): guard cancelled request future in event stream pipeline#730
Open
valter-silva-au wants to merge 1 commit into
Conversation
…line - input_stream/duplex_stream run the request in a background task and set the request future once the transport send starts. If the consumer awaiting that future is cancelled, asyncio cancels the future and the unguarded set_result raised asyncio.InvalidStateError. - Guard the set with `if not request_future.cancelled()`, the same remedy suggested in awslabs/aws-sdk-python#13 (whose original CRTResponseBody site was removed by smithy-lang#573; the failure mode relocated here). - Add a regression test driving the pipeline with a cancelled request future; verified RED without the guard, GREEN with it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue #, if available: Relates to awslabs/aws-sdk-python#13 — same failure mode, in the upstream runtime where the code now lives.
Description of changes:
When an input or duplex event stream is invoked,
RequestPipelineruns the request in a background task and resolves arequest_futureonce the transport send has been kicked off, so the caller gets a stream that's ready to send. If the consumer awaiting that future is cancelled (the caller is cancelled, or times out), asyncio cancels the future. The still-running background task then reaches the unguardedrequest_future.set_result(...)on a now-cancelled future, which raisesasyncio.InvalidStateError(surfaced as aSmithyError).This guards the set:
This is the same remedy suggested upstream in awslabs/aws-sdk-python#13. That issue originally pointed at
CRTResponseBody._on_completeinsmithy_http, but that class was removed in #573 (the CRT client moved to the unified-stream API); the same race relocated to the request pipeline insmithy-core, which is the one remaining unguardedset_resultin the runtime.Notes:
retryable()returnsFalsewhen an input stream member is present), soset_resultruns at most once — the failure is specifically the cancellation race, not a double-set. Guarding oncancelled()is therefore sufficient and precise.packages/smithy-core/tests/unit/aio/test_client.py) that drives the pipeline with a cancelled request future. Verified it fails (InvalidStateError) without the guard and passes with it. There were no prior unit tests for the pipeline; the test introduces minimal protocol/transport doubles.bugfixchangelog fragment forsmithy-core.make check-pyandmake test-pypass locally (1482 passed, 3 skipped).By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.