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

Remove precondition: yielding to terminated AsyncSequence #432

Merged

Conversation

rnro
Copy link
Contributor

@rnro rnro commented Dec 20, 2023

Motivation:

If a task iterating over inbound streams using the new async API is cancelled then it can lead to the process crashing as we hit a precondition which guards against yielding to a terminated NIOHTTP2AsyncSequence.

Modifications:

Remove the precondition.

Result:

The code no longer crashes, instead it silently drops the yielded value.

Doing more than this such as closing the connection requires rethinking more of the API.

Motivation:

If a task iterating over inbound streams using the new async API is
cancelled then it can lead to the process crashing as we hit a
precondition which guards against yielding to a terminated
NIOHTTP2AsyncSequence.

Modifications:

Remove the precondition.

Result:

The code no longer crashes, instead it silently drops the yielded value.

Doing more than this such as closing the connection requires rethinking
more of the API.
@rnro rnro requested a review from glbrntt December 20, 2023 11:11
@Lukasa Lukasa added the 🔨 semver/patch No public API change. label Dec 20, 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.

Can you please add a test to cover this?

@rnro rnro requested a review from Lukasa December 20, 2023 13:32
@Lukasa Lukasa merged commit 69e838f into apple:main Dec 20, 2023
7 of 8 checks passed
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.

3 participants