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

Attempted fix for "unexpected cancel" issue in #34 #35

Merged
merged 5 commits into from
Feb 28, 2022

Conversation

bdowning
Copy link
Contributor

@bdowning bdowning commented Feb 19, 2022

This attempts to fix #34 . I'm not quite sure what the intended logic behind the base_error stuff was, and as such I'm not sure if I've messed something up here. But the tests pass! :)

@codecov
Copy link

codecov bot commented Feb 19, 2022

Codecov Report

Merging #35 (1db75c9) into main (02be382) will decrease coverage by 0.10%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #35      +/-   ##
==========================================
- Coverage   78.74%   78.63%   -0.11%     
==========================================
  Files          11       11              
  Lines         941      941              
==========================================
- Hits          741      740       -1     
- Misses        200      201       +1     
Impacted Files Coverage Δ
src/aiotools/taskgroup.py 91.07% <100.00%> (-0.60%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 02be382...1db75c9. Read the comment docs.

@bdowning
Copy link
Contributor Author

@achimnol Here's another one, though I'd appreciate a careful look at my solution to ensure I haven't broken something unintentionally (though all tests pass!)

@achimnol
Copy link
Owner

achimnol commented Feb 21, 2022

I think this would be better reviewed by the original authors of the module, @1st1 and @elprans.
As you may know, our taskgroup module is imported from EdgeDB's _taskgroup module.

Note that Python 3.11 will introduce the concept of ExceptionGroup and BaseExceptionGroup (where CancelledError belongs to) with a new syntax except*: (PEP-654). aiotools currently targets Python 3.6, but I'm considering migration of taskgroup and ptaskgroup modules to Python 3.11 once it gets released, while keeping other modules (aiotools.context, ...) compatible with Python 3.6. This means that current MultiError is a kind of interim solution which needs to be replaced soon.

@bdowning
Copy link
Contributor Author

That does sound really nice (was unaware of that PEP), though because a lot of us will be stuck on older Pythons for quite a while (Lambda is a good example, it's still on 3.9 unless you roll your own runtime) having a compatibility mode would be great.

@achimnol
Copy link
Owner

achimnol commented Feb 21, 2022

That does sound really nice (was unaware of that PEP), though because a lot of us will be stuck on older Pythons for quite a while (Lambda is a good example, it's still on 3.9 unless you roll your own runtime) having a compatibility mode would be great.

I'm not sure yet how Python 3.11 will organize the TaskGroup API in the stdlib asyncio (which is the main motivation of ExceptionGroup), but if it turns out good, we could leave the current aiotools' implementation as an alternative option for old Python users.

At that time, we could bump the version of aiotools to 2.0 and drop backward-compatilibility to Python 3.6 and 3.7, as 3.6 is already EOL and 3.7 would become EOL in June the next year, while 3.11 is expected to be released on October this year.

@achimnol
Copy link
Owner

Also take a look at python/cpython#31270!

@bdowning
Copy link
Contributor Author

That's awesome, I've lost count of how many times I've shot myself in the foot due to dangling tasks. Having this in stdlib should help a lot.

@bdowning
Copy link
Contributor Author

Any word on this? This is actually somewhat of an issue for an application I'm trying to use task groups for. I can of course just copy an implementation into my code, but an upstream fix would be nicer. :)

* Take the implementation of `_is_base_error()` from Python 3.11's
  `asyncio.TaskGroup` instead of changing `__aexit__()` method.
* Use more specific exception class in the test case to prevent
  unintended catch-all situation in terms of regression check.
@achimnol
Copy link
Owner

I think the problem is _is_base_error() implementation.
Updated it to use the same logic from Python 3.11's asyncio.TaskGroup._is_base_error().

@bdowning
Copy link
Contributor Author

👍 LGTM

I can confirm that just the _is_base_error patch fixes the "expected cancel" errors I was seeing in my actual codebase.

@achimnol achimnol merged commit 0051cd9 into achimnol:main Feb 28, 2022
@bdowning bdowning deleted the unexpected-cancel-fix branch February 28, 2022 03:34
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.

TaskGroup propagates CancelledError rather than a MultiError on exception
2 participants