Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Fix make_deferred_yieldable to work with coroutines #6505

Merged
merged 3 commits into from
Dec 10, 2019

Conversation

erikjohnston
Copy link
Member

No description provided.

@erikjohnston erikjohnston requested a review from a team December 10, 2019 11:24
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

lgtm other than nitpicking over comments.

@@ -624,6 +626,11 @@ def make_deferred_yieldable(deferred):

(This is more-or-less the opposite operation to run_in_background.)
"""
if inspect.isawaitable(deferred):
# If we're given a coroutine we need to convert it to a deferred so that
# we can attach callbacks (and not immediately return).
Copy link
Member

Choose a reason for hiding this comment

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

I'm not convinced that's the reason we need to convert it to a deferred (we can do similar things by awaiting awaitables).

What it does do is to start running the coroutine and find out if it has finished or not, which saves us clearing and immediately restoring the logcontext in the common case that the coroutine completes synchronously.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, fair. I guess I was thinking of this in terms of converting to a future/deferred so that we could put the callbacks on it (since you can't do that on raw coroutines). Will reword

@erikjohnston erikjohnston merged commit 35f3c36 into develop Dec 10, 2019
@erikjohnston erikjohnston deleted the erikj/make_deferred_yiedable branch January 9, 2020 15:47
babolivier pushed a commit that referenced this pull request Sep 1, 2021
* commit '35f3c366e':
  Update comment
  Newsfile
  Fix make_deferred_yieldable to work with coroutines
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants