classification
Title: Change base class for futures.CancelledError
Type: enhancement Stage: resolved
Components: asyncio, Library (Lib) Versions: Python 3.8
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: Dima.Tisnek, JustAnotherArchivist, achimnol, asvetlov, bmerry, chris.jerdonek, gustavo, gvanrossum, miss-islington, socketpair, yselivanov
Priority: normal Keywords: patch

Created on 2018-01-10 16:31 by socketpair, last changed 2020-07-24 14:19 by miss-islington. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 13528 merged yselivanov, 2019-05-23 21:38
PR 21474 merged JustAnotherArchivist, 2020-07-14 16:49
PR 21475 closed miss-islington, 2020-07-14 17:22
PR 21476 merged miss-islington, 2020-07-14 17:23
PR 21606 merged miss-islington, 2020-07-24 14:12
Messages (21)
msg309772 - (view) Author: Марк Коренберг (socketpair) * Date: 2018-01-10 16:31
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.
msg309775 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2018-01-10 18:27
This is a backwards incompatible change.  IMO it's too late to change this.
msg309776 - (view) Author: Марк Коренберг (socketpair) * Date: 2018-01-10 18:35
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 ?
msg309777 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2018-01-10 18:43
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.
msg309779 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2018-01-10 19:15
I agree with Yury, this feels too risky to consider. The "except Exception:
<log>" code is at fault.
msg309796 - (view) Author: Марк Коренберг (socketpair) * Date: 2018-01-11 08:52
@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 ?
msg309799 - (view) Author: Andrew Svetlov (asvetlov) * (Python committer) Date: 2018-01-11 09:43
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.
msg309800 - (view) Author: Andrew Svetlov (asvetlov) * (Python committer) Date: 2018-01-11 09:48
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?
msg309894 - (view) Author: Joongi Kim (achimnol) * Date: 2018-01-13 18:03
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.
msg317705 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2018-05-25 19:40
Closing this issue as I, personally, don't see this happening and there's no point in keeping it open.
msg326274 - (view) Author: Gustavo J. A. M. Carneiro (gustavo) * Date: 2018-09-24 17:42
What a shame, I've seen this error many times as well.

Surely making it BaseException will not break that much code?...
msg326275 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2018-09-24 17:47
> Closing this issue as I, personally, don't see this happening and there's no point in keeping it open.

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).
msg337839 - (view) Author: Dima Tisnek (Dima.Tisnek) * Date: 2019-03-13 09:25
ping
msg343618 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2019-05-27 12:45
New changeset 431b540bf79f0982559b1b0e420b1b085f667bb7 by Yury Selivanov in branch 'master':
bpo-32528: Make asyncio.CancelledError a BaseException. (GH-13528)
https://github.com/python/cpython/commit/431b540bf79f0982559b1b0e420b1b085f667bb7
msg343619 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2019-05-27 12:45
Done. 🤞 we don't regret this.
msg373166 - (view) Author: Bruce Merry (bmerry) * Date: 2020-07-06 19:56
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.
msg373510 - (view) Author: (JustAnotherArchivist) * Date: 2020-07-11 03:14
As another datapoint, this also broke some of my code on 3.8 because I was using `concurrent.futures.CancelledError` rather than `asyncio.CancelledError` to 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/36277556

While I understand the rationale behind this change, it would've been good to include this inheritance detail in the 3.8 release notes.
msg373511 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2020-07-11 03:48
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:

>
> JustAnotherArchivist <justanotherarchivist@riseup.net> added the comment:
>
> As another datapoint, this also broke some of my code on 3.8 because I was
> using `concurrent.futures.CancelledError` rather than
> `asyncio.CancelledError` to 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/36277556
>
> While I understand the rationale behind this change, it would've been good
> to include this inheritance detail in the 3.8 release notes.
>
> ----------
> nosy: +JustAnotherArchivist
>
> _______________________________________
> Python tracker <report@bugs.python.org>
> <https://bugs.python.org/issue32528>
> _______________________________________
>
-- 
--Guido (mobile)
msg373650 - (view) Author: miss-islington (miss-islington) Date: 2020-07-14 17:22
New changeset 2a5181829af394b82e8e8c917183c709ee72a2b7 by JustAnotherArchivist in branch 'master':
bpo-32528: Document the change in inheritance of asyncio.CancelledError (GH-21474)
https://github.com/python/cpython/commit/2a5181829af394b82e8e8c917183c709ee72a2b7
msg373940 - (view) Author: miss-islington (miss-islington) Date: 2020-07-19 07:51
New changeset 700cb6617545cdb8a9e16bb2e6efe90788a60d4d by Miss Islington (bot) in branch '3.8':
bpo-32528: Document the change in inheritance of asyncio.CancelledError (GH-21474)
https://github.com/python/cpython/commit/700cb6617545cdb8a9e16bb2e6efe90788a60d4d
msg374185 - (view) Author: miss-islington (miss-islington) Date: 2020-07-24 14:19
New changeset ba07d4a0c30b4d817b4c31a052388a68cc17bc3b by Miss Islington (bot) in branch '3.9':
bpo-32528: Document the change in inheritance of asyncio.CancelledError (GH-21474)
https://github.com/python/cpython/commit/ba07d4a0c30b4d817b4c31a052388a68cc17bc3b
History
Date User Action Args
2020-07-24 14:19:20miss-islingtonsetmessages: + msg374185
2020-07-24 14:12:20miss-islingtonsetpull_requests: + pull_request20747
2020-07-19 07:51:01miss-islingtonsetmessages: + msg373940
2020-07-14 17:23:05miss-islingtonsetpull_requests: + pull_request20621
2020-07-14 17:22:58miss-islingtonsetpull_requests: + pull_request20620
2020-07-14 17:22:51miss-islingtonsetnosy: + miss-islington
messages: + msg373650
2020-07-14 16:49:22JustAnotherArchivistsetpull_requests: + pull_request20619
2020-07-11 03:48:08gvanrossumsetmessages: + msg373511
2020-07-11 03:14:16JustAnotherArchivistsetnosy: + JustAnotherArchivist
messages: + msg373510
2020-07-06 19:56:10bmerrysetnosy: + bmerry
messages: + msg373166
2019-05-27 12:45:50yselivanovsetstatus: open -> closed
resolution: fixed
messages: + msg343619

stage: patch review -> resolved
2019-05-27 12:45:15yselivanovsetmessages: + msg343618
2019-05-23 21:38:08yselivanovsetkeywords: + patch
stage: patch review
pull_requests: + pull_request13442
2019-03-13 09:25:39Dima.Tisneksetnosy: + Dima.Tisnek
messages: + msg337839
2019-02-20 05:15:27chris.jerdoneksetnosy: + chris.jerdonek
2018-09-24 17:47:33yselivanovsetstatus: closed -> open
versions: - Python 3.7
messages: + msg326275

resolution: rejected -> (no value)
stage: resolved -> (no value)
2018-09-24 17:42:14gustavosetnosy: + gustavo
messages: + msg326274
2018-05-25 19:40:43yselivanovsetstatus: open -> closed
resolution: rejected
messages: + msg317705

stage: resolved
2018-01-13 18:03:34achimnolsetmessages: + msg309894
2018-01-12 16:35:46achimnolsetnosy: + achimnol
2018-01-11 09:48:19asvetlovsetmessages: + msg309800
2018-01-11 09:43:18asvetlovsetmessages: + msg309799
2018-01-11 08:52:28socketpairsetmessages: + msg309796
2018-01-10 19:15:33gvanrossumsetmessages: + msg309779
2018-01-10 18:43:49yselivanovsetnosy: + gvanrossum
messages: + msg309777
2018-01-10 18:35:07socketpairsetmessages: + msg309776
2018-01-10 18:27:31yselivanovsetmessages: + msg309775
2018-01-10 16:37:14socketpairsetcomponents: + Library (Lib)
2018-01-10 16:31:24socketpairsettype: enhancement
2018-01-10 16:31:15socketpaircreate