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

ignore_after: patch Task.cancel to separate timeout vs external cancel #47

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

SomberNight
Copy link
Contributor

fixes #44

This is my patch from #44 (comment), and a unit test based on the opening post in that thread.
I have tried to make the test not too dependent on timing - experimentation suggests it is very reliable, it did not fail during any of my runs.

Note: timeout_after is also affected. Should I include a unit test for that too? Not sure how to avoid duplicating the code but I could try.

@kyuupichan
Copy link
Owner

What if the task is already cancelled? What if the task is cancelled externally but chooses to ignore it?

@SomberNight
Copy link
Contributor Author

Your points are valid, please interpret my lack of reply here as not knowing how to fix :/

What if the task is cancelled externally but chooses to ignore it?

We've had a bug that is sort of like that (spesmilo/electrum#7952 (comment))... I've now added a new unit test and code changes to fix that specific pattern in 8c0f3a6 .

@SomberNight
Copy link
Contributor Author

I feel like the complexities here are getting out of hand however...

Have you seen that Python 3.11 will have some form of TaskGroups, and async timeout context managers (but also ExceptionGroups, not sure yet if that will complicate things)? Maybe aiorpcx.curio could be deprecated at some point in favour of those.

SomberNight added a commit to spesmilo/electrum that referenced this pull request Aug 26, 2022
fixes #7952

see in particular #7952 (comment)
> So the issue is with the aiorpcx monkey patch in util.py, as it
> relies on side-effecting the asyncio.Task, and it patches Task.cancel.
> However, aiohttp also uses Task.cancel for its own timeouting of the
> http request, with the same Task object, and this confuses timeout_after.
> Ultimately FxThread.run exits.

related kyuupichan/aiorpcX#47

---

note: I am not content at all with this monkey-patching approach,
but at the same time I don't see how to properly fix this handling all
edge-cases in aiorpcx.

python 3.11 is finally adding an implementation of TaskGroup [0] and
an async timeout context manager [1] in the asyncio module of the stdlib.
Looking at the implementations, they look unfeasible to backport:
much of the implementation of asyncio.Task had to be changed for them
to work, and TaskGroup in particular relies on the new ExceptionGroups.
Some of these edge cases we are battling with aiorpcx.curio look
difficult to fix without those stdlib changes...
Anyway, when we bump the min python to 3.11, I look forward to switching
to that code instead, and ripping this stuff out.

[0]: https://docs.python.org/3.11/library/asyncio-task.html#task-groups
[1]: https://docs.python.org/3.11/library/asyncio-task.html#asyncio.timeout
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.

ignore_after cannot be cancelled sometimes (race?)
2 participants