Ticket #990: ddiff

=== modified file 'twisted/internet/defer.py'
--- twisted/internet/defer.py   2010-02-21 23:19:24 +0000
+++ twisted/internet/defer.py   2010-02-22 20:23:28 +0000
@@ -180,8 +180,8 @@
     For more information about Deferreds, see doc/howto/defer.html or
     U{http://twistedmatrix.com/projects/core/documentation/howto/defer.html}
 
-    When creating a Deferred, you should provide a canceller function, which
-    will be called by d.cancel() to let you do any cleanup necessary if the
+    When creating a Deferred, you may provide a canceller function, which
+    will be called by d.cancel() to let you do any clean-up necessary if the
     user decides not to wait for the deferred to complete.
     """
 
@@ -204,20 +204,25 @@
         """
         Initialize a L{Deferred}.
 
-        @param canceller: an object to call in order to stop the pending
-            operation scheduled by this L{Deferred} when L{Deferred.cancel} is
-            invoked.  If this callable does not invoke its argument's
-            C{callback} or C{errback} method when it is called,
-            L{Deferred.cancel} will invoke L{Deferred.errback} on its behalf.
-            Note that if this is not supplied, C{callback} or C{errback} may
-            still be invoked once, even if the default behavior of C{cancel}
-            invokes C{errback} on your behalf.  This is to allow the clients of
-            code which returns a Deferred to to cancel it without the
-            instantiator providing any specific implementation support for
-            cancellation.  New in 10.0.
-
-        @type canceller: a 1-argument callable which takes a L{Deferred} and
-            returns C{None}
+        @param canceller: a callable used to stop the pending operation
+            scheduled by this L{Deferred} when L{Deferred.cancel} is
+            invoked. The canceller will be passed the deferred whose
+            cancelation is requested (i.e., self).
+            
+            If a canceller is not given, or does not invoke its argument's
+            C{callback} or C{errback} method, L{Deferred.cancel} will
+            invoke L{Deferred.errback} with a L{CancelledError}.
+
+            Note that if a canceller is not given, C{callback} or
+            C{errback} may still be invoked exactly once, even though
+            defer.py will have already invoked C{errback}, as described
+            above.  This allows clients of code which returns a L{Deferred}
+            to cancel it without requiring the L{Deferred} instantiator to
+            provide any specific implementation support for cancellation.
+            New in 10.0.
+
+        @type canceller: a 1-argument callable which takes a L{Deferred}. The
+            return result is ignored.
         """
         self.callbacks = []
         self._canceller = canceller
@@ -360,38 +365,29 @@
         """
         Cancel this L{Deferred}.
 
-        If this L{Deferred} is waiting on another L{Deferred}, forward the
-        cancellation to the other L{Deferred}.
-
         If the L{Deferred} has not yet had its C{errback} or C{callback} method
         invoked, call the canceller function provided to the constructor. If
         that function does not invoke C{callback} or C{errback}, or if no
         canceller function was provided, errback with L{CancelledError}.
 
-        @raise AlreadyCalledError: if the L{Deferred} has previously had its
-            C{callback} or C{errback} method invoked.
+        If this L{Deferred} is waiting on another L{Deferred}, forward the
+        cancellation to the other L{Deferred}.
         """
-        canceller = self._canceller
         if not self.called:
+            canceller = self._canceller
             if canceller:
                 canceller(self)
             else:
-                # Eat the callback that will eventually be fired
+                # Arrange to eat the callback that will eventually be fired
                 # since there was no real canceller.
                 self._suppressAlreadyCalled = 1
-
             if not self.called:
-                # The canceller didn't do an errback of its own
-                try:
-                    raise CancelledError()
-                except:
-                    self.errback(failure.Failure())
+                # There was no canceller, or the canceller didn't call
+                # callback or errback.
+                self.errback(failure.Failure(CancelledError()))
         elif isinstance(self.result, Deferred):
-            # Waiting for another deferred -- cancel it instead
+            # Waiting for another deferred -- cancel it instead.
             self.result.cancel()
-        else:
-            # Called and not waiting for another deferred
-            raise AlreadyCalledError()
 
 
     def _continue(self, result):
@@ -400,9 +396,6 @@
 
 
     def _startRunCallbacks(self, result):
-        # Canceller is no longer relevant
-        self.canceller = None
-
         if self.called:
             if self._suppressAlreadyCalled:
                 self._suppressAlreadyCalled = 0
@@ -1078,6 +1071,17 @@
 
 
     def _cancelAcquire(self, d):
+        """
+        Remove a deferred d from our waiting list, as the deferred has been
+        canceled.
+        
+        Note: We do not need to wrap this in a try/except to catch d not
+        being in self.waiting because this canceller will not be called if
+        d has fired. release() pops a deferred out of self.waiting and
+        calls it, so the canceller will no longer be called.
+
+        @param d: The deferred that has been canceled.
+        """
         self.waiting.remove(d)
 
 
@@ -1130,6 +1134,17 @@
 
 
     def _cancelAcquire(self, d):
+        """
+        Remove a deferred d from our waiting list, as the deferred has been
+        canceled.
+        
+        Note: We do not need to wrap this in a try/except to catch d not
+        being in self.waiting because this canceller will not be called if
+        d has fired. release() pops a deferred out of self.waiting and
+        calls it, so the canceller will no longer be called.
+
+        @param d: The deferred that has been canceled.
+        """
         self.waiting.remove(d)
 
 
@@ -1202,6 +1217,17 @@
 
 
     def _cancelGet(self, d):
+        """
+        Remove a deferred d from our waiting list, as the deferred has been
+        canceled.
+        
+        Note: We do not need to wrap this in a try/except to catch d not
+        being in self.waiting because this canceller will not be called if
+        d has fired. put() pops a deferred out of self.waiting and calls
+        it, so the canceller will no longer be called.
+
+        @param d: The deferred that has been canceled.
+        """
         self.waiting.remove(d)
 
 

=== modified file 'twisted/test/test_defer.py'
--- twisted/test/test_defer.py  2010-02-21 21:13:51 +0000
+++ twisted/test/test_defer.py  2010-02-22 20:28:00 +0000
@@ -28,40 +28,40 @@
 class DeferredTestCase(unittest.TestCase):
 
     def setUp(self):
-        self.callback_results = None
-        self.errback_results = None
-        self.callback2_results = None
+        self.callbackResults = None
+        self.errbackResults = None
+        self.callback2Results = None
 
     def _callback(self, *args, **kw):
-        self.callback_results = args, kw
+        self.callbackResults = args, kw
         return args[0]
 
     def _callback2(self, *args, **kw):
-        self.callback2_results = args, kw
+        self.callback2Results = args, kw
 
     def _errback(self, *args, **kw):
-        self.errback_results = args, kw
+        self.errbackResults = args, kw
 
     def testCallbackWithoutArgs(self):
         deferred = defer.Deferred()
         deferred.addCallback(self._callback)
         deferred.callback("hello")
-        self.failUnlessEqual(self.errback_results, None)
-        self.failUnlessEqual(self.callback_results, (('hello',), {}))
+        self.failUnlessEqual(self.errbackResults, None)
+        self.failUnlessEqual(self.callbackResults, (('hello',), {}))
 
     def testCallbackWithArgs(self):
         deferred = defer.Deferred()
         deferred.addCallback(self._callback, "world")
         deferred.callback("hello")
-        self.failUnlessEqual(self.errback_results, None)
-        self.failUnlessEqual(self.callback_results, (('hello', 'world'), {}))
+        self.failUnlessEqual(self.errbackResults, None)
+        self.failUnlessEqual(self.callbackResults, (('hello', 'world'), {}))
 
     def testCallbackWithKwArgs(self):
         deferred = defer.Deferred()
         deferred.addCallback(self._callback, world="world")
         deferred.callback("hello")
-        self.failUnlessEqual(self.errback_results, None)
-        self.failUnlessEqual(self.callback_results,
+        self.failUnlessEqual(self.errbackResults, None)
+        self.failUnlessEqual(self.callbackResults,
                              (('hello',), {'world': 'world'}))
 
     def testTwoCallbacks(self):
@@ -69,10 +69,10 @@
         deferred.addCallback(self._callback)
         deferred.addCallback(self._callback2)
         deferred.callback("hello")
-        self.failUnlessEqual(self.errback_results, None)
-        self.failUnlessEqual(self.callback_results,
+        self.failUnlessEqual(self.errbackResults, None)
+        self.failUnlessEqual(self.callbackResults,
                              (('hello',), {}))
-        self.failUnlessEqual(self.callback2_results,
+        self.failUnlessEqual(self.callback2Results,
                              (('hello',), {}))
 
     def testDeferredList(self):
@@ -295,11 +295,11 @@
         d.addCallback(lambda r, d2=d2: d2)
         d.addCallback(self._callback)
         d.callback(1)
-        assert self.callback_results is None, "Should not have been called yet."
+        assert self.callbackResults is None, "Should not have been called yet."
         d2.callback(2)
-        assert self.callback_results is None, "Still should not have been called yet."
+        assert self.callbackResults is None, "Still should not have been called yet."
         d2.unpause()
-        assert self.callback_results[0][0] == 2, "Result should have been from second deferred:%s"% (self.callback_results,)
+        assert self.callbackResults[0][0] == 2, "Result should have been from second deferred:%s"% (self.callbackResults,)
 
     def testGatherResults(self):
         # test successful list of deferreds
@@ -640,98 +640,253 @@
         d.addBoth(lambda ign: None)
 
 
-class FooError(Exception):
-    pass
 
 class DeferredCancellerTest(unittest.TestCase):
     def setUp(self):
-        self.callback_results = None
-        self.errback_results = None
-        self.callback2_results = None
-        self.cancellerCalled = False
+        self.callbackResults = None
+        self.errbackResults = None
+        self.callback2Results = None
+        self.cancellerCallCount = 0
+
+
+    def tearDown(self):
+        # Sanity check that the canceller was called at most once.
+        self.assertTrue(self.cancellerCallCount in (0, 1))
+
 
     def _callback(self, data):
-        self.callback_results = data
+        self.callbackResults = data
         return data
 
+
     def _callback2(self, data):
-        self.callback2_results = data
+        self.callback2Results = data
+
 
     def _errback(self, data):
-        self.errback_results = data
+        self.errbackResults = data
 
 
     def test_noCanceller(self):
         """
-        Verify that a Deferred without a canceller errbacks with defer.CancelledError.
-        """
-        d = defer.Deferred()
-        d.addCallbacks(self._callback, self._errback)
-        d.cancel()
-        self.assertEquals(self.errback_results.type, defer.CancelledError)
-
-        # Test that further callbacks *are* swallowed
-        d.callback(None)
-
-        # But that a second is not
-        self.assertRaises(defer.AlreadyCalledError, d.callback, None)
-
-
-    def test_canceller(self):
-        """
-        Verify that a Deferred calls its specified canceller when it is
-        cancelled.
-        """
-        def cancel(d):
-            self.cancellerCalled=True
-
-        d = defer.Deferred(canceller=cancel)
-        d.addCallbacks(self._callback, self._errback)
-        d.cancel()
-        self.assertEquals(self.cancellerCalled, True)
-        self.assertEquals(self.errback_results.type, defer.CancelledError)
-
-        # Test that further callbacks are *not* swallowed
-        self.assertRaises(defer.AlreadyCalledError, d.callback, None)
-
-
-    def test_cancellerWithCallback(self):
-        """
-        Verify that a canceller which invokes a (non-errback) callback will not
-        get errbacked with L{CancelledError}
-        """
-        def cancel(d):
-            self.cancellerCalled=True
-            d.errback(FooError())
-        d = defer.Deferred(canceller=cancel)
-        d.addCallbacks(self._callback, self._errback)
-        d.cancel()
-        self.assertEquals(self.cancellerCalled, True)
-        self.assertEquals(self.errback_results.type, FooError)
-
-
-    def test_cancelAlreadyCalled(self):
-        """
-        Verify that cancelling a L{Deferred} twice will result in an
-        L{AlreadyCalledError}.
-        """
-        def cancel(d):
-            self.cancellerCalled=True
-        d = defer.Deferred(canceller=cancel)
-        d.callback(None)
-        self.assertRaises(defer.AlreadyCalledError, d.cancel)
-        self.assertEquals(self.cancellerCalled, False)
+        A L{defer.Deferred} without a canceller must errback with a
+        L{defer.CancelledError} and not callback.
+        """
+        d = defer.Deferred()
+        d.addCallbacks(self._callback, self._errback)
+        d.cancel()
+        self.assertEquals(self.errbackResults.type, defer.CancelledError)
+        self.assertEquals(self.callbackResults, None)
+
+
+    def test_raisesAfterCancelAndCallback(self):
+        """
+        A L{defer.Deferred} without a canceller, when cancelled must allow
+        a single extra call to callback, and raise
+        L{defer.AlreadyCalledError} if callbacked or errbacked thereafter.
+        """
+        d = defer.Deferred()
+        d.addCallbacks(self._callback, self._errback)
+        d.cancel()
+
+        # A single extra callback should be swallowed.
+        d.callback(None)
+
+        # But a second call to callback or errback is not.
+        self.assertRaises(defer.AlreadyCalledError, d.callback, None)
+        self.assertRaises(defer.AlreadyCalledError, d.errback, Exception())
+
+
+    def test_raisesAfterCancelAndErrback(self):
+        """
+        A L{defer.Deferred} without a canceller, when cancelled must allow
+        a single extra call to errback, and raise
+        L{defer.AlreadyCalledError} if callbacked or errbacked thereafter.
+        """
+        d = defer.Deferred()
+        d.addCallbacks(self._callback, self._errback)
+        d.cancel()
+
+        # A single extra errback should be swallowed.
+        d.errback(Exception())
+
+        # But a second call to callback or errback is not.
+        self.assertRaises(defer.AlreadyCalledError, d.callback, None)
+        self.assertRaises(defer.AlreadyCalledError, d.errback, Exception())
+
+
+    def test_noCancellerMultipleCancelsAfterCancelAndCallback(self):
+        """
+        A L{Deferred} without a canceller, when cancelled and then
+        callbacked, ignores multiple cancels thereafter.
+        """
+        d = defer.Deferred()
+        d.addCallbacks(self._callback, self._errback)
+        d.cancel()
+        currentFailure = self.errbackResults
+        # One callback will be ignored
+        d.callback(None)
+        # Cancel should have no effect.
+        d.cancel()
+        self.assertIdentical(currentFailure, self.errbackResults)
+
+
+    def test_noCancellerMultipleCancelsAfterCancelAndErrback(self):
+        """
+        A L{defer.Deferred} without a canceller, when cancelled and then
+        errbacked, ignores multiple cancels thereafter.
+        """
+        d = defer.Deferred()
+        d.addCallbacks(self._callback, self._errback)
+        d.cancel()
+        self.assertEquals(self.errbackResults.type, defer.CancelledError)
+        currentFailure = self.errbackResults
+        # One errback will be ignored
+        d.errback(GenericError())
+        # I.e., we should still have a CancelledError.
+        self.assertEquals(self.errbackResults.type, defer.CancelledError)
+        d.cancel()
+        self.assertIdentical(currentFailure, self.errbackResults)
+
+
+    def test_noCancellerMultipleCancel(self):
+        """
+        Calling cancel multiple times on a deferred with no canceller
+        results in a L{defer.CancelledError}. Subsequent calls to cancel
+        do not cause an error.
+        """
+        d = defer.Deferred()
+        d.addCallbacks(self._callback, self._errback)
+        d.cancel()
+        self.assertEquals(self.errbackResults.type, defer.CancelledError)
+        currentFailure = self.errbackResults
+        d.cancel()
+        self.assertIdentical(currentFailure, self.errbackResults)
+
+
+    def test_cancellerMultipleCancel(self):
+        """
+        Verify that calling cancel multiple times on a deferred with a
+        canceller that does not errback results in a
+        L{defer.CancelledError} and that subsequent calls to cancel do not
+        cause an error and that after all that, the canceller was only
+        called once.
+        """
+        def cancel(d):
+            self.cancellerCallCount += 1
+            
+        d = defer.Deferred(canceller=cancel)
+        d.addCallbacks(self._callback, self._errback)
+        d.cancel()
+        self.assertEquals(self.errbackResults.type, defer.CancelledError)
+        currentFailure = self.errbackResults
+        d.cancel()
+        self.assertIdentical(currentFailure, self.errbackResults)
+        self.assertEquals(self.cancellerCallCount, 1)
+
+
+    def test_simpleCanceller(self):
+        """
+        Verify that a L{defer.Deferred} calls its specified canceller when
+        it is cancelled, and that further call/errbacks raise
+        L{defer.AlreadyCalledError}.
+        """
+        def cancel(d):
+            self.cancellerCallCount += 1
+
+        d = defer.Deferred(canceller=cancel)
+        d.addCallbacks(self._callback, self._errback)
+        d.cancel()
+        self.assertEquals(self.cancellerCallCount, 1)
+        self.assertEquals(self.errbackResults.type, defer.CancelledError)
+
+        # Test that further call/errbacks are *not* swallowed
+        self.assertRaises(defer.AlreadyCalledError, d.callback, None)
+        self.assertRaises(defer.AlreadyCalledError, d.errback, Exception())
+
+
+    def test_cancellerArg(self):
+        """
+        Verify that a canceller is given the correct deferred argument.
+        """
+        def cancel(d1):
+            self.assertIdentical(d1, d)
+        d = defer.Deferred(canceller=cancel)
+        d.addCallbacks(self._callback, self._errback)
+        d.cancel()
+
+
+    def test_cancelAfterCallback(self):
+        """
+        Test that cancelling a deferred after it has been callbacked does
+        not cause an error.
+        """
+        def cancel(d):
+            self.cancellerCallCount += 1
+            d.errback(GenericError())
+        d = defer.Deferred(canceller=cancel)
+        d.addCallbacks(self._callback, self._errback)
+        d.callback('biff!')
+        d.cancel()
+        self.assertEquals(self.cancellerCallCount, 0)
+        self.assertEquals(self.errbackResults, None)
+        self.assertEquals(self.callbackResults, 'biff!')
+
+
+    def test_cancelAfterErrback(self):
+        """
+        Test that cancelling a L{Deferred} after it has been errbacked does
+        not result in a L{defer.CancelledError}.
+        """
+        def cancel(d):
+            self.cancellerCallCount += 1
+            d.errback(GenericError())
+        d = defer.Deferred(canceller=cancel)
+        d.addCallbacks(self._callback, self._errback)
+        d.errback(GenericError())
+        d.cancel()
+        self.assertEquals(self.cancellerCallCount, 0)
+        self.assertEquals(self.errbackResults.type, GenericError)
+        self.assertEquals(self.callbackResults, None)
+
+
+    def test_cancellerThatErrbacks(self):
+        """
+        Test a canceller which errbacks its deferred.
+        """
+        def cancel(d):
+            self.cancellerCallCount += 1
+            d.errback(GenericError())
+        d = defer.Deferred(canceller=cancel)
+        d.addCallbacks(self._callback, self._errback)
+        d.cancel()
+        self.assertEquals(self.cancellerCallCount, 1)
+        self.assertEquals(self.errbackResults.type, GenericError)
+
+
+    def test_cancellerThatCallbacks(self):
+        """
+        Test a canceller which calls its deferred.
+        """
+        def cancel(d):
+            self.cancellerCallCount += 1
+            d.callback('hello!')
+        d = defer.Deferred(canceller=cancel)
+        d.addCallbacks(self._callback, self._errback)
+        d.cancel()
+        self.assertEquals(self.cancellerCallCount, 1)
+        self.assertEquals(self.callbackResults, 'hello!')
+        self.assertEquals(self.errbackResults, None)
 
 
     def test_cancelNestedDeferred(self):
         """
         Verify that a Deferred, A, which is waiting on another Deferred, B,
-        returned from one of its callbacks, will propagate L{CancelledError}
-        when B is cancelled.
+        returned from one of its callbacks, will propagate
+        L{defer.CancelledError} when A is cancelled.
         """
         def innerCancel(d):
-            self.assertIdentical(d, B)
-            self.cancellerCalled = True
+            self.cancellerCallCount += 1
         def cancel(d):
             self.assert_(False)
 
@@ -741,8 +896,11 @@
         A.addCallback(lambda data: B)
         A.cancel()
         A.addCallbacks(self._callback, self._errback)
-        self.assertEquals(self.cancellerCalled, True)
-        self.assertEquals(self.errback_results.type, defer.CancelledError)
+        # The cancel count should be one (the cancellation done by B)
+        self.assertEquals(self.cancellerCallCount, 1)
+        # B's canceller didn't errback, so defer.py will have called errback
+        # with a CancelledError.
+        self.assertEquals(self.errbackResults.type, defer.CancelledError)
 
 
 
@@ -775,9 +933,9 @@
 
     def test_errorLog(self):
         """
-        Verify that when a Deferred with no references to it is fired, and its
-        final result (the one not handled by any callback) is an exception,
-        that exception will be logged immediately.
+        Verify that when a L{Deferred} with no references to it is fired,
+        and its final result (the one not handled by any callback) is an
+        exception, that exception will be logged immediately.
         """
         defer.Deferred().addCallback(lambda x: 1/0).callback(1)
         gc.collect()
@@ -884,6 +1042,33 @@
         lock.release()
         self.failIf(lock.locked)
 
+
+    def test_cancelLockAfterAcquired(self):
+        """
+        When canceling a L{Deferred] from a L{DeferredLock} that already
+        has the lock, the cancel should have no effect.
+        """
+        def _failOnErrback(_):
+            self.fail("Unexpected errback call!")
+        lock = defer.DeferredLock()
+        d = lock.acquire()
+        d.addErrback(_failOnErrback)
+        d.cancel()
+
+
+    def test_cancelLockBeforeAcquired(self):
+        """
+        When canceling a L{Deferred] from a L{DeferredLock} that does not
+        yet have the lock (i.e., the L{Deferred} has not fired), the cancel
+        should cause a L{defer.CancelledError} failure.
+        """
+        lock = defer.DeferredLock()
+        _ign = lock.acquire()
+        d = lock.acquire()
+        self.failUnlessFailure(d, defer.CancelledError)
+        d.cancel()
+
+
     def testSemaphore(self):
         N = 13
         sem = defer.DeferredSemaphore(N)
@@ -929,6 +1114,35 @@
             sem.release()
             self.assertEquals(self.counter, N + 1)
 
+
+    def test_cancelSemaphoreAfterAcquired(self):
+        """
+        When canceling a L{Deferred] from a L{DeferredSemaphore} that
+        already has the semaphore, the cancel should have no effect.
+        """
+        def _failOnErrback(_):
+            self.fail("Unexpected errback call!")
+            
+        sem = defer.DeferredSemaphore(1)
+        d = sem.acquire()
+        d.addErrback(_failOnErrback)
+        d.cancel()
+
+
+    def test_cancelSemaphoreBeforeAcquired(self):
+        """
+        When canceling a L{Deferred] from a L{DeferredSemaphore} that does
+        not yet have the semaphore (i.e., the L{Deferred} has not fired),
+        the cancel should cause a L{defer.CancelledError} failure.
+        """
+        sem = defer.DeferredSemaphore(1)
+        _ign = sem.acquire()
+        d = sem.acquire()
+        self.failUnlessFailure(d, defer.CancelledError)
+        d.cancel()
+        return d
+
+
     def testQueue(self):
         N, M = 2, 2
         queue = defer.DeferredQueue(N, M)
@@ -967,6 +1181,34 @@
         self.assertRaises(defer.QueueUnderflow, queue.get)
 
 
+    def test_cancelQueueAfterSynchronousGet(self):
+        """
+        When canceling a L{Deferred] from a L{DeferredQueue} that already has
+        a result, the cancel should have no effect.
+        """
+        def _failOnErrback(_):
+            self.fail("Unexpected errback call!")
+            
+        queue = defer.DeferredQueue()
+        d = queue.get()
+        d.addErrback(_failOnErrback)
+        queue.put(None)
+        d.cancel()
+    
+
+    def test_cancelQueueAfterGet(self):
+        """
+        When canceling a L{Deferred] from a L{DeferredQueue} that does not
+        have a result (i.e., the L{Deferred} has not fired), the cancel
+        should cause a L{defer.CancelledError} failure.
+        """
+        queue = defer.DeferredQueue()
+        d = queue.get()
+        self.failUnlessFailure(d, defer.CancelledError)
+        d.cancel()
+        return d
+
+
 
 class DeferredFilesystemLockTestCase(unittest.TestCase):
     """