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

Cancel trio_as_aio tasks before stopping the trio-asyncio loop #96

Merged
merged 7 commits into from
Jan 6, 2021

Conversation

oremanj
Copy link
Member

@oremanj oremanj commented Jan 4, 2021

trio-asyncio creates a Trio nursery with one task that runs the asyncio event loop, and more tasks that run any Trio tasks started from asyncio context using loop.run_trio() or trio_asyncio.trio_as_aio(). Previously, it would first stop the asyncio event loop and then cancel the Trio tasks in the nursery. This created problems in some cases, because the Trio tasks run inside a wrapper that propagates their result to an asyncio future, and that propagation stops working when the asyncio event loop stops running. Depending on the exact circumstances, this could lead to an "Event loop is closed" error (#95) or a deadlock (#89).

This PR resolves the issue by cancelling the relevant Trio tasks and waiting for them to exit before shutting down the trio-asyncio loop.

Fixes #56
Fixes #89
Fixes #95

@oremanj oremanj mentioned this pull request Jan 4, 2021
13 tasks
Copy link
Contributor

@shamrin shamrin left a comment

Choose a reason for hiding this comment

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

Ha, nested nurseries! Looks good to me.

matryoshka

But maybe don't ignore every RuntimeError :)

oremanj#1

try:
await loop.synchronize()
except RuntimeError: # "Event loop is closed"
pass
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks a bit dangerous to ignore every RuntimeError here. I've made a PR (against this branch) with a possible improvement :)

oremanj#1

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it's the lesser of multiple evils, since it's the exception that asyncio raises in this case and we want to mimic their API; and the operation is simple enough that there aren't other plausible sources of RuntimeError. Moreover, if something is screwy and this raises RuntimeError for a different reason, I contend that skipping this synchronization step (and accepting whatever error ensues) is better than deadlocking (which is the likely other outcome). Your PR doesn't export the EventLoopClosedError exception, so it's possible for it to create user confusion if it shows up in a traceback but they can't catch it.

IOW: it would've been nice if asyncio used a custom exception for this condition, but given that they don't, I think it's better for us not to add our own.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not

try:
    loop._check_closed()
except RuntimeError:
    pass
else:
    await loop.synchronize()

?
That'd do this check twice, but that's too cheap to be noticeable IMHO.

Copy link
Member Author

Choose a reason for hiding this comment

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

That seems fine to me. I didn't go that route because I didn't want to rely on a private member, but we already do elsewhere, so...

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's the lesser of multiple evils

Agreed!

Copy link
Contributor

@shamrin shamrin Jan 5, 2021

Choose a reason for hiding this comment

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

@oremanj I like @smurfix's idea to call _check_closed() beforehand.

(As for it being a private member - in a separate PR we can probably switch to using public is_closed() method everywhere. See #98.)

Copy link
Member Author

Choose a reason for hiding this comment

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

is_closed() is perfect here, I didn't realize it existed! Then we don't have to worry about the exception at all.

@shamrin
Copy link
Contributor

shamrin commented Jan 5, 2021

Regarding loop= test failures on nightly python. Cherry-picked @smurfix commits do not fix them because they come from PR #94, which removed a lot of tests, including loop=-using tests. I think the best way the proceed is to revert those commits from here and live with nightly python failures a little bit more (we've had them before). PR #94 will fix those failures anyhow.

Update: 👇 I took the liberty of reverting those commits myself 👇 (please let me if it was a mistake 😬).

This reverts (cherry-picked) commits 8930bf2 and d1051b0.

We will do the de-`loop=`-ification in PR python-trio#94.
@oremanj
Copy link
Member Author

oremanj commented Jan 6, 2021

I've temporarily made the Travis checks not be required to merge PRs. We can reenable them once we remove the loop= args. or just stop using Travis :-)

@oremanj oremanj merged commit 723a7f7 into python-trio:master Jan 6, 2021
shamrin added a commit to shamrin/trio-asyncio that referenced this pull request Jan 6, 2021
Intermittent test failures are now fixed by PR python-trio#96
@oremanj oremanj deleted the fix-close-order branch November 6, 2023 22:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants