Opened 7 months ago

Last modified 4 months ago

#9679 defect new

Deferred object created from future returns wrong CancelledError type

Reported by: Adam Wierzbicki Owned by: Adam Wierzbicki
Priority: normal Milestone:
Component: core Keywords:
Cc: Branch:
Author:

Description

Caller of Deferred.cancel() expects the cancelled deferred object to return twisted.internet.defer.CancelledError. On the other hand, calling Future.cancel() results in concurrent.futures.CancelledError. This causes problems with deferred objects created with Deferred.fromFuture(). E.g. setting timeout for such objects won't work because _cancelledToTimedOutError only traps twisted.internet.defer.CancelledError and not concurrent.futures.CancelledError.

Change History (3)

comment:1 Changed 7 months ago by Glyph

Keywords: review added

comment:2 Changed 5 months ago by Kyle Altendorf

comment:3 Changed 4 months ago by Glyph

Keywords: review removed
Owner: set to Adam Wierzbicki

HI Adam,

Thanks for this contribution. Sorry it's taken so long to get to. (If you become a contributor and review code too, that queue will move faster :-))

This change raises some problematic compatibility concerns. The fact that the existing tests raise asyncio exceptions means it was a considered property of the implementation, not an accident; code using this integration in anger and properly handling these errors will be broken by this change, with very little warning. We have a policy that governs this sort of change: https://twistedmatrix.com/documents/current/core/development/policy/compatibility-policy.html

I'd say that probably the compatible way forward here is to make a new hybrid cancellation error, one that inherits from *both* asyncio and Twisted exception types, so that both kinds of thing can work; existing code that handles asyncio cancellation will keep working, but code which depends on twisted's exception types will also work.

There are other things we could do though, like fixing the timeout code. It's not generally valid to assume .cancel() will always give you a cancellation exception, since it might stop a process midway through, resulting in some other exception type, or perhaps even a partial success result.

When you have time to address this, please submit for re-review. Thanks!

This is sufficiently subtle and foundational that it might be a good idea to raise it on the mailing list as well.

Note: See TracTickets for help on using tickets.