Change base class for futures.CancelledError

[socketpair]

BPO	32528
Nosy	@gvanrossum, @asvetlov, @cjerdonek, @socketpair, @dimaqq, @1st1, @achimnol, @miss-islington, @bmerry, @JustAnotherArchivist
PRs	bpo-32528: Make asyncio.CancelledError a BaseException. #13528 bpo-32528: Document the change in inheritance of asyncio.CancelledError #21474 [3.9] bpo-32528: Document the change in inheritance of asyncio.CancelledError (GH-21474) #21475 [3.8] bpo-32528: Document the change in inheritance of asyncio.CancelledError (GH-21474) #21476 [3.9] bpo-32528: Document the change in inheritance of asyncio.CancelledError (GH-21474) #21606

^{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']

[socketpair]

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 Error class) to BaseException.

Just like SystemExit and 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]

This is a backwards incompatible change. IMO it's too late to change this.

[socketpair]

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]

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]

I agree with Yury, this feels too risky to consider. The "except Exception:
<log>" code is at fault.

[socketpair]

@gvanrossum

More real code:

async def xxxx():
    while True:
        try:
            result = await download()
            handle_result(result)
        except Exception as e:
            log.warning('Fail..%r', e)
        await asyncio.sleep()

Why sucha a code is fault ?

[asvetlov]

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.CancelledError clause everywhere.

But proposed change is backward incompatible, sure.

[asvetlov]

Honestly I have no strong opinion.

Correct code should not be affected, if somebody want to handle task cancellation explicitly -- he already have except CancelledError in 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]

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]

Closing this issue as I, personally, don't see this happening and there's no point in keeping it open.