Opened 4 years ago

Closed 4 years ago

#6639 enhancement closed fixed (fixed)

Add cancellation support to twisted.internet.defer.DeferredList

Reported by: Kai Zhang Owned by: Kai Zhang
Priority: normal Milestone:
Component: core Keywords:
Cc: Branch: branches/deferredlist-cancellation-6639
branch-diff, diff-cov, branch-cov, buildbot
Author: kaizhang

Description

Add cancellation support to twisted.internet.defer.DeferredList. When a DeferredList is cancelled by calling the cancel method, cancel every Deferred in the list. If the DeferredList has already fired, including the case where the fireOnOneCallback/fireOnOneErrback flag is set and the DeferredList fires because one Deferred in the list fires with a non-failure/failure result, do nothing in the cancel method.

Change History (27)

comment:1 Changed 4 years ago by Kai Zhang

Author: kaizhang
Branch: branches/deferredlist-cancellation-6639

(In [39276]) Branching to 'deferredlist-cancellation-6639'

comment:2 Changed 4 years ago by Kai Zhang

Keywords: review added

comment:3 Changed 4 years ago by Kai Zhang

Keywords: review removed
Owner: set to Kai Zhang

Change the deferredList instance variable to private.

comment:4 Changed 4 years ago by Kai Zhang

Status: newassigned

comment:5 Changed 4 years ago by Kai Zhang

Keywords: review added
Owner: Kai Zhang deleted
Status: assignednew

comment:6 Changed 4 years ago by habnabit

Owner: set to habnabit

comment:7 Changed 4 years ago by habnabit

Keywords: review removed
Owner: changed from habnabit to Kai Zhang
  1. This breaks backward compatibility by iterating over deferredList twice. There is likely to be code that is passing DeferredList a generator or some other single-use iterator. The easy fix is to iterate over self._deferredList.
  1. The docstring for gatherResults doesn't seem correct. What's guaranteeing that the returned Deferred will errback? All of the passed-in Deferreds can callback instead of errback on cancellation.
  1. Similar to #2 for the DeferredList.cancel docstring.
  1. Similar to #2, there don't seem to be any tests for the case where cancelling a Deferred callbacks instead of errbacks for DeferredList or gatherResults.
  1. Several places in test_defer.py say L{defer.Deferrd}.
  1. You might consider making a function or method to instantiate a deferred and set its cancel method to MockCancel, considering this is a thing you do a lot.

Thank you for your contribution!

comment:8 Changed 4 years ago by Itamar Turner-Trauring

I'd also add the MockCall is completely unnecessary, you don't need to mock anything to test that a Deferred has been cancelled.

comment:9 Changed 4 years ago by Jean-Paul Calderone

Thanks for your work on this so far kaizhang, habnabit, itamar.

First, I agree with Itamar. I'd suggest getting rid of MockCancel entirely. It's better to test the actual effects of cancellation. You can assert that the Deferreds have been cancelled by asserting that they have a CancelledError failure result.

Also, think about not logging any errors at all here. As far as I can tell, the only exception that might get logged is AlreadyCalledError - which actually just reflects a shortcoming of the implementation of cancellation here. If DeferredList.cancel didn't try to cancel already-called Deferreds, then it wouldn't provoke this exception. If there's another case, please demonstrate it with a unit test (using flushLoggedErrors(Exception) makes the test less clear than it could be about what's going to happen - every possible logged error is an Exception, so who knows what this will flush?).

Also, try writing unit tests that only have a single assertion in them. These are more useful, since you get to see the results of all assertions in a single test run. When you have two assertions, the first can fail and prevent the second from running. Then you can't tell if the second would have passed or failed.

comment:10 Changed 4 years ago by Itamar Turner-Trauring

cancel() never throws an exception, not even AlreadyCalledError (except for something that is arguably a bug in the implementation and we should probably fix, exceptions from cancellation functions).

comment:11 in reply to:  8 Changed 4 years ago by Kai Zhang

Replying to itamar:

I'd also add the MockCall is completely unnecessary, you don't need to mock anything to test that a Deferred has been cancelled.

The MockCancel is not used to test that a Deferred has been cancelled. It's used to test that when the DeferredList has already fired, the Deferreds in the list will NOT be cancelled. Is there anyway else to test this?

comment:12 in reply to:  9 Changed 4 years ago by Kai Zhang

Replying to exarkun:

Also, think about not logging any errors at all here. As far as I can tell, the only exception that might get logged is AlreadyCalledError - which actually just reflects a shortcoming of the implementation of cancellation here. If DeferredList.cancel didn't try to cancel already-called Deferreds, then it wouldn't provoke this exception. If there's another case, please demonstrate it with a unit test (using flushLoggedErrors(Exception) makes the test less clear than it could be about what's going to happen - every possible logged error is an Exception, so who knows what this will flush?).

I was worried about the user might supply a canceller which raise a exception accidentally.

comment:13 Changed 4 years ago by Jean-Paul Calderone

I was worried about the user might supply a canceller which raise a exception accidentally.

Okay. That seems ... a bit more reasonable. Please change the log.err call to pass a reason argument explaining that this is the case. It's sort of a historical accident and a misfeature that you can call log.err without a reason - this produces unexplained errors in the log that are often very difficult to track down.

The MockCancel is not used to test that a Deferred has been cancelled. It's used to test that when the DeferredList has already fired, the Deferreds in the list will NOT be cancelled. Is there anyway else to test this?

You can use TestCase.assertNoResult to verify that a Deferred has not been fired (which also means it has not been cancelled).

comment:14 in reply to:  13 Changed 4 years ago by Kai Zhang

Replying to exarkun:

The MockCancel is not used to test that a Deferred has been cancelled. It's used to test that when the DeferredList has already fired, the Deferreds in the list will NOT be cancelled. Is there anyway else to test this?

You can use TestCase.assertNoResult to verify that a Deferred has not been fired (which also means it has not been cancelled).

I used TestCase.assertNoResult in test_cancelFiredOnOneCallbackDeferredList to verify the Deferred has not been fired.

However, in test_cancelDeferredListFiredWithNonFailure and test_cancelDeferredListFiredWithFailure all the Deferreds have already fired(thus make the DeferredList fire), so I can not use TestCase.assertNoResult to verify the DeferredList.cancel didn't try to cancel all the Deferreds(after they fired). That's why I used the MockCancel.

comment:15 Changed 4 years ago by Jean-Paul Calderone

However, in test_cancelDeferredListFiredWithNonFailure and test_cancelDeferredListFiredWithFailure all the Deferreds have already fired(thus make the DeferredList fire), so I can not use TestCase.assertNoResult to verify the DeferredList.cancel didn't try to cancel all the Deferreds(after they fired). That's why I used the MockCancel.

What's the point of this test? Or put another way, I don't think that it is an important feature of DeferredList.cancel that it not cancel its element Deferreds when they all have results already. Is there any observable difference in behavior if you don't provide this guarantee?

comment:16 in reply to:  15 Changed 4 years ago by Kai Zhang

Replying to exarkun:

What's the point of this test? Or put another way, I don't think that it is an important feature of DeferredList.cancel that it not cancel its element Deferreds when they all have results already. Is there any observable difference in behavior if you don't provide this guarantee?

There is no observable difference since Deferred.cancel itself won't try to cancel an already fired Deferred. This test is more about testing the logic is right(do not cancel an already fired DeferredList). So is it OK to just remove this test?

comment:17 Changed 4 years ago by Jean-Paul Calderone

I'd say it's okay to remove both the test *and* the logic. :) There's no sense having code that doesn't produce observable differences.

comment:18 in reply to:  17 Changed 4 years ago by Kai Zhang

Replying to exarkun:

I'd say it's okay to remove both the test *and* the logic. :) There's no sense having code that doesn't produce observable differences.

I think we can remove the test but keep the logic. The logic doesn't only cover the case where the DeferredList fires because all the Deferreds in the list has fired. It also covers the case where the DeferredList fires because one Deferred in the list has fired but the rest of the Deferreds may haven't fired(when the fireOnOneCallback/fireOnOneErrback flag is set). In this case, cancel the Deferred directly will case a problem.

comment:19 Changed 4 years ago by Jean-Paul Calderone

I think we can remove the test but keep the logic.

This is always the wrong conclusion. :) Untested code is not allowed in Twisted.

It also covers the case where the DeferredList fires because one Deferred in the list has fired but the rest of the Deferreds may haven't fired(when the fireOnOneCallback/fireOnOneErrback flag is set). In this case, cancel the Deferred directly will case a problem.

Great! It sounds like there is some observable behavior after all. Perhaps you can write a test for that case to replace the test we've been discussing?

comment:20 in reply to:  19 Changed 4 years ago by Kai Zhang

Replying to exarkun:

Great! It sounds like there is some observable behavior after all. Perhaps you can write a test for that case to replace the test we've been discussing?

We do have the tests for that case, the test_cancelFiredOnOneCallbackDeferredList and test_cancelFiredOnOneErrbackDeferredList. So can we keep the logic as long as we keep these two tests?

comment:21 Changed 4 years ago by Kai Zhang

Keywords: review added
Owner: Kai Zhang deleted

I have revised my code according to the comments and also added the news file.

Build Results

comment:22 Changed 4 years ago by habnabit

Just a quick note: you did not address one of my review points:

There don't seem to be any tests for the case where cancelling a Deferred callbacks instead of errbacks for DeferredList or gatherResults.

comment:23 Changed 4 years ago by Kai Zhang

Keywords: review removed
Owner: set to Kai Zhang
Status: newassigned

comment:24 in reply to:  22 Changed 4 years ago by Kai Zhang

Replying to habnabit:

Just a quick note: you did not address one of my review points:

There don't seem to be any tests for the case where cancelling a Deferred callbacks instead of errbacks for DeferredList or gatherResults.

I will address this soon.

comment:25 Changed 4 years ago by Kai Zhang

Keywords: review added
Owner: Kai Zhang deleted
Status: assignednew

comment:26 Changed 4 years ago by Itamar Turner-Trauring

Keywords: review removed
Owner: set to Kai Zhang

Thanks for all you work.

  1. News file should also mention gatherResults.
  2. In test_cancelDeferredListWithException you raise Exception, and then assert later on that the one Exception was logged. This is a bit of a problem, since you might have a bug in the code causing AttributeError or NameError. Since these are subclasses of Exception, the flushLoggedErrors will still catch those I believe, erroneously making the test pass. It's better to therefore use a subclass of Exception that is unlikely to be raised by the actual code, e.g. RuntimeError.
  3. test_cancelDeferredListWithFireOnOneErrback should assert the FirstError wraps a CancelledError.
  4. Same for test_cancelGatherResults.

Please address these comments, and then merge.

comment:27 Changed 4 years ago by Kai Zhang

Resolution: fixed
Status: newclosed

(In [39657]) Merge deferredlist-cancellation-6639: Added cancel() method to DeferredList.

Author: kaizhang Reviewer: itamar Fixes: #6639

Added cancellation support to twisted.internet.defer.DeferredList. When a DeferredList is cancelled by calling the cancel method, cancel every Deferred in the list. If the DeferredList has already fired, including the case where the fireOnOneCallback/fireOnOneErrback flag is set and the DeferredList fires because one Deferred in the list fires with a non-failure/failure result, do nothing in the cancel method.

Note: See TracTickets for help on using tickets.