Skip to content

Conversation

fabianfett
Copy link
Member

NIOThrowingAsyncSequenceProducer throws when cancelled

Motivation:

Currently NIOThrowingAsyncSequenceProducer currently does not throw a CancellationError, if the AsyncSequence is cancelled while the buffer is not empty.

Modifications:

  • Make NIOThrowingAsyncSequenceProducer throw when cancelled

Result:

  • Postgres queries that are cancelled throw and do not give the impression that they succeed.

@fabianfett fabianfett requested a review from Lukasa April 28, 2023 13:28
@fabianfett fabianfett force-pushed the ff-async-sequence-producer-throws-when-cancelled branch from 6745e64 to f8a3175 Compare April 28, 2023 13:29
@@ -603,6 +618,9 @@ extension NIOThrowingAsyncSequenceProducer {
failure: Failure?
)

/// The state once a call to next has been cancelled. To reach this state we must cancel the source.
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment is a bit confused: presumably we're cancelling the iterator, not the source.

Copy link
Member Author

Choose a reason for hiding this comment

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

            /// The state once a call to next has been cancelled. Cancel the source when entering this state.

@Lukasa Is this better?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, LGTM.

@fabianfett fabianfett requested a review from Lukasa April 28, 2023 14:56
@Lukasa Lukasa added the 🔨 semver/patch No public API change. label Apr 28, 2023
Copy link
Contributor

@Lukasa Lukasa left a comment

Choose a reason for hiding this comment

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

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔨 semver/patch No public API change.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants