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

[sdk/python] - Await all async tasks #6606

Merged
merged 9 commits into from
Mar 30, 2021
Merged

[sdk/python] - Await all async tasks #6606

merged 9 commits into from
Mar 30, 2021

Conversation

komalali
Copy link
Member

@komalali komalali commented Mar 24, 2021

Currently, only RPC futures are awaited in the python SDK. When RPCs are complete, we collect all remaining tasks and cancel them. This means that code within .apply() and/or un-exported outputs are not always awaited and may be canceled in-flight.

This PR awaits the remaining outstanding tasks until either an exception is raised or all futures are resolved. Once asyncio.wait returns we cancel any pending tasks if they exist and check the done pile for any futures that raised an exception, re-raising if so.

We then repeat the cycle if the "extra async work" has itself scheduled any RPCs, only breaking when there are no remaining RPCs at the end of the cycle.

Python part of #3991

@komalali komalali force-pushed the komalali/await-python branch 2 times, most recently from 81222bd to 8cb5c67 Compare March 24, 2021 05:32
Comment on lines -57 to -58
# We will occasionally start tasks deliberately that we know will never complete. We must
# cancel them before shutting down the event loop.
Copy link
Member Author

@komalali komalali Mar 24, 2021

Choose a reason for hiding this comment

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

Is this actually true? I haven't found any instances where we start tasks "forever" in practice. And I don't really know why we would have a run-forever task within the pulumi_func especially.

The solution here depends on this statement being false and an assumption that all futures will resolve unless an exception is thrown.

@komalali komalali marked this pull request as ready for review March 24, 2021 16:02
@komalali komalali force-pushed the komalali/await-python branch 2 times, most recently from f9373b5 to 8e2c2b4 Compare March 24, 2021 18:03
Copy link
Member

@lblackstone lblackstone left a comment

Choose a reason for hiding this comment

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

LGTM with the caveat that I'm not very familiar with this part of the codebase.

sdk/python/lib/pulumi/runtime/stack.py Outdated Show resolved Hide resolved
@komalali
Copy link
Member Author

Had a chat with @pgavlin about this PR and he brought up the fact that the "other async tasks" could themselves schedule new RPCs so I need to do a little more work here, at the very least to add tests for that particular scenario.

@pgavlin
Copy link
Member

pgavlin commented Mar 26, 2021

"other async tasks" could themselves schedule new RPCs so I need to do a little more work here, at the very least to add tests for that particular scenario.

One concrete example is the creation of a resource inside of an Apply callback: the callback will not run until the corresponding RPC has finished, and running the callback will add work to the RPC queue.

@komalali
Copy link
Member Author

@pgavlin I've added the "create a resource inside apply" test and fixed up the code accordingly. Should be ready for your review 🙏🏽

@komalali komalali merged commit 3532115 into master Mar 30, 2021
@komalali komalali deleted the komalali/await-python branch March 30, 2021 17:56
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.

None yet

3 participants