Repository navigation
Change base class for futures.CancelledError #76709
Description
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
assignee = None
closed_at = <Date 2019-05-27.12:45:50.368>
created_at = <Date 2018-01-10.16:31:15.055>
labels = ['3.8', 'type-feature', 'library', 'expert-asyncio']
title = 'Change base class for futures.CancelledError'
updated_at = <Date 2020-07-24.14:19:20.996>
user = 'https://github.com/socketpair'bugs.python.org fields:
activity = <Date 2020-07-24.14:19:20.996>
actor = 'miss-islington'
assignee = 'none'
closed = True
closed_date = <Date 2019-05-27.12:45:50.368>
closer = 'yselivanov'
components = ['Library (Lib)', 'asyncio']
creation = <Date 2018-01-10.16:31:15.055>
creator = 'socketpair'
dependencies = []
files = []
hgrepos = []
issue_num = 32528
keywords = ['patch']
message_count = 21.0
messages = ['309772', '309775', '309776', '309777', '309779', '309796', '309799', '309800', '309894', '317705', '326274', '326275', '337839', '343618', '343619', '373166', '373510', '373511', '373650', '373940', '374185']
nosy_count = 11.0
nosy_names = ['gvanrossum', 'gustavo', 'asvetlov', 'chris.jerdonek', 'socketpair', 'Dima.Tisnek', 'yselivanov', 'achimnol', 'miss-islington', 'bmerry', 'JustAnotherArchivist']
pr_nums = ['13528', '21474', '21475', '21476', '21606']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue32528'
versions = ['Python 3.8']
Activity
socketpair commentedon Jan 10, 2018
I have discoverd one very ugly pattern connected with asyncio. Many times I see code like this:
try:
await something()
except Exception:
log.error('Opertaion failed -- will retry later.')
Seems, everything is fine, but asyncio.CancelledError unintentionally
also suppressed in that code. So, sometimes coroutines are not cancellable.
In order to mitigate thi, we had to write:
try:
await something()
except CancelledError:
raise
except Exception:
log.error('Opertaion failed. will retry later.')
So, what I propose: Basically is to change base class for asyncio.CancelledError
from Exception (yes, I know concurrent.futures and it's
Errorclass) to BaseException.Just like
SystemExitand other SPECIAL exceptions.Yes, I know that it would be incompatible change. But I suspect that impact will be minimal. Documentation for concurrent.futures and asyncio does not say that this exception is derived from Exception.
1st1 commentedon Jan 10, 2018
This is a backwards incompatible change. IMO it's too late to change this.
socketpair commentedon Jan 10, 2018
Will you accept PR if I fix that ?
I think we may merge that in Python 3.8
Who can also judge us? @asvetlov, what do you think about my idea ?
1st1 commentedon Jan 10, 2018
While I understand the reasons for the proposed change, I'd still be -1 for it. Solely on the basis of "we don't know how much this change will break, but it will surely break something in very subtle ways".
Another problem is that asyncio currently doesn't handle BaseExceptions that well.
I'll put Guido in the nosy list, maybe he'll have a different opinion on this.
gvanrossum commentedon Jan 10, 2018
I agree with Yury, this feels too risky to consider. The "except Exception:
<log>" code is at fault.
socketpair commentedon Jan 11, 2018
@gvanrossum
More real code:
Why sucha a code is fault ?
asvetlov commentedon Jan 11, 2018
Inheritance from Exception is very annoying, I saw issues with unexpected suppressing CancelledError many times.
Even experienced people constantly forget to add a separate
except asyncio.CancelledErrorclause everywhere.But proposed change is backward incompatible, sure.
asvetlov commentedon Jan 11, 2018
Honestly I have no strong opinion.
Correct code should not be affected, if somebody want to handle task cancellation explicitly -- he already have
except CancelledErrorin his code.What are use cases for intentional catching Exception for task cancellation prevention?
Could we add a warning for this case without base exception class change? I don't see how to do it but maybe somebody has an idea?
achimnol commentedon Jan 13, 2018
I strongly agree to have discretion of CancelledError and other general exceptions, though I don't have concrete ideas on good unobtrusive ways to achieve this.
If I write my codes carefully I could control most of cancellation explicitly, but it is still hard to control it in 3rd-party libraries that I depend on. Often they just raise random errors, or CancelledError is swallowed.
Also it would be nice to have some documentation and examples on how to write "cancellation-friendly" coroutine codes.
1st1 commentedon May 25, 2018
Closing this issue as I, personally, don't see this happening and there's no point in keeping it open.
2 remaining items
1st1 commentedon Sep 24, 2018
Actually, Andrew and I changed our opinion on this, so I'm re-opening the issue.
After visiting three conferences this summer and talking to asyncio users, it seems that this is a very serious pitfall. At least 8 different people shared stories about really hard to debug problems caused by "except Exception" code blocking cancellation.
I now think we should fix this and make CancelledError a BaseException. Doing that isn't as straightforward as it seems as we have to first fix how asyncio handles BaseExceptions (my next ToDo).
dimaqq commentedon Mar 13, 2019
ping
1st1 commentedon May 27, 2019
New changeset 431b540 by Yury Selivanov in branch 'master':
bpo-32528: Make asyncio.CancelledError a BaseException. (GH-13528)
431b540
1st1 commentedon May 27, 2019
Done. 🤞 we don't regret this.
bmerry commentedon Jul 6, 2020
FYI this has just bitten me after updating my OS to one that ships Python 3.8. It is code that was written with asyncio cancellation in mind and which expected CancelledError to be caught with "except Exception" (the exception block unwound incomplete operations before re-raising the exception).
It's obviously too late to do anything about Python 3.8, but I'm mentioning this as a data point in support of having a deprecation period if similar changes are made in future.
On the plus side, while fixing up my code and checking all instances of "except Exception" I found some places where this change did fix latent cancellation bugs. So I'm happy with the change, just a little unhappy that it came as a surprise.
JustAnotherArchivist commentedon Jul 11, 2020
As another datapoint, this also broke some of my code on 3.8 because I was using
concurrent.futures.CancelledErrorrather thanasyncio.CancelledErrorto handle cancelled futures. And I'm certainly not the only one to have done this given that it's mentioned in at least two Stack Overflow answers: https://stackoverflow.com/a/38655063 and https://stackoverflow.com/a/36277556While I understand the rationale behind this change, it would've been good to include this inheritance detail in the 3.8 release notes.
gvanrossum commentedon Jul 11, 2020
Can you send a PR against what’s new 3.8?
On Fri, Jul 10, 2020 at 20:14 JustAnotherArchivist <report@bugs.python.org>
wrote:
--
--Guido (mobile)
miss-islington commentedon Jul 14, 2020
New changeset 2a51818 by JustAnotherArchivist in branch 'master':
bpo-32528: Document the change in inheritance of asyncio.CancelledError (GH-21474)
2a51818
miss-islington commentedon Jul 19, 2020
New changeset 700cb66 by Miss Islington (bot) in branch '3.8':
bpo-32528: Document the change in inheritance of asyncio.CancelledError (GH-21474)
700cb66
miss-islington commentedon Jul 24, 2020
New changeset ba07d4a by Miss Islington (bot) in branch '3.9':
bpo-32528: Document the change in inheritance of asyncio.CancelledError (GH-21474)
ba07d4a
Cartroo commentedon Jun 18, 2022
I know this is an old issue, but I just wanted to mention the parallel I note with the same change made for
GeneratorExit, way back in Python 2.6 — as I recall, the justification then was much the same as the justification now.Given the pain of these changes, I wonder if it's worth considering a particularly strenuous review process for exceptions added to any standard library modules to try to make the correct decisions of
ExceptionvsBaseExceptionup front, rather than having to have this debate about fixing them later.