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

Fix FnStream generator polling to prevent poll-after-ready #1903

Merged
merged 4 commits into from
Oct 26, 2022
Merged

Conversation

rcoh
Copy link
Collaborator

@rcoh rcoh commented Oct 24, 2022

Motivation and Context

#1902 : FnStream had a bug where, under load, an async function was polled after already being done

Description

This change wraps the generator in an option, nulling it after completion to prevent double-polling

Testing

Unit tests + all of the current paginator integration tests

Checklist

  • I have updated CHANGELOG.next.toml if I made changes to the smithy-rs codegen or runtime crates
  • I have updated CHANGELOG.next.toml if I made changes to the AWS SDK, generated SDK code, or SDK runtime crates

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@rcoh rcoh requested a review from a team as a code owner October 24, 2022 19:43
@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

Comment on lines +175 to +178
// smithy-rs#1902: there was a bug where we could continue to poll the generator after it
// had returned Poll::Ready. This test case leaks the tx half so that the channel stays open
// but the send side generator completes. By calling `poll` multiple times on the resulting future,
// we can trigger the bug and validate the fix.
Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for the explanatory comment!

@Velfi Velfi requested a review from a team as a code owner October 25, 2022 15:09
@Velfi Velfi enabled auto-merge (squash) October 25, 2022 15:09
@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@Velfi Velfi merged commit 16e34a5 into main Oct 26, 2022
@Velfi Velfi deleted the async-panic branch October 26, 2022 17:32
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