Changeset 25928

Timestamp:

01/09/2009 01:18:47 PM (6 months ago)

Author:

exarkun

Message:

Merge dtpfactory-timeouts-3596 Author: exarkun Reviewer: therve Fixes: #3596 Fix the interaction between DTP connection setup success, timeout, and failure events. In particular, fix the behavior when a connection fails after the specified timeout has elapsed. Add direct unit tests for DTPFactory as well.

Files:

• trunk/twisted/protocols/ftp.py (modified) (4 diffs)
• trunk/twisted/test/test_ftp.py (modified) (2 diffs)

trunk/twisted/protocols/ftp.py

@@ -485 +485 @@
 class DTPFactory(protocol.ClientFactory):
     """
-    DTP protocol factory.
+    Client factory for I{data transfer process} protocols.
 
     @ivar peerCheck: perform checks to make sure the ftp-pi's peer is the same
         as the dtp's
     @ivar pi: a reference to this factory's protocol interpreter
-    """
+
+    @ivar _state: Indicates the current state of the DTPFactory.  Initially,
+        this is L{_IN_PROGRESS}.  If the connection fails or times out, it is
+        L{_FAILED}.  If the connection succeeds before the timeout, it is
+        L{_FINISHED}.
+    """
+
+    _IN_PROGRESS = object()
+    _FAILED = object()
+    _FINISHED = object()
+
+    _state = _IN_PROGRESS
 
     # -- configuration variables --
@@ -496 +507 @@
 
     # -- class variables --
-    def __init__(self, pi, peerHost=None):
+    def __init__(self, pi, peerHost=None, reactor=None):
         """Constructor
         @param pi: this factory's protocol interpreter
@@ -505 +516 @@
         self.peerHost = peerHost            # the from FTP.transport.peerHost()
         self.deferred = defer.Deferred()    # deferred will fire when instance is connected
-        self.deferred.addBoth(lambda ign: (delattr(self, 'deferred'), ign)[1])
         self.delayedCall = None
+        if reactor is None:
+            from twisted.internet import reactor
+        self._reactor = reactor
+
 
     def buildProtocol(self, addr):
         log.msg('DTPFactory.buildProtocol', debug=True)
+
+        if self._state is not self._IN_PROGRESS:
+            return None
+        self._state = self._FINISHED
+
         self.cancelTimeout()
-        if self.pi.dtpInstance:   # only create one instance
-            return
         p = DTP()
         p.factory = self
@@ -519 +536 @@
         return p
 
+
     def stopFactory(self):
         log.msg('dtpFactory.stopFactory', debug=True)
         self.cancelTimeout()
 
+
     def timeoutFactory(self):
         log.msg('timed out waiting for DTP connection')
-        if self.deferred:
-            d, self.deferred = self.deferred, None
-
-            # TODO: LEFT OFF HERE!
-
-            d.addErrback(debugDeferred, 'timeoutFactory firing errback')
-            d.errback(defer.TimeoutError())
-        self.stopFactory()
+        if self._state is not self._IN_PROGRESS:
+            return
+        self._state = self._FAILED
+
+        d = self.deferred
+        self.deferred = None
+        d.errback(
+            PortConnectionError(defer.TimeoutError("DTPFactory timeout")))
+
 
     def cancelTimeout(self):
-        if not self.delayedCall.called and not self.delayedCall.cancelled:
+        if self.delayedCall is not None and self.delayedCall.active():
             log.msg('cancelling DTP timeout', debug=True)
             self.delayedCall.cancel()
-            assert self.delayedCall.cancelled
-            log.msg('timeout has been cancelled', debug=True)
+
 
     def setTimeout(self, seconds):
         log.msg('DTPFactory.setTimeout set to %s seconds' % seconds)
-        self.delayedCall = reactor.callLater(seconds, self.timeoutFactory)
+        self.delayedCall = self._reactor.callLater(seconds, self.timeoutFactory)
+
 
     def clientConnectionFailed(self, connector, reason):
-        self.deferred.errback(PortConnectionError(reason))
+        if self._state is not self._IN_PROGRESS:
+            return
+        self._state = self._FAILED
+        d = self.deferred
+        self.deferred = None
+        d.errback(PortConnectionError(reason))
+
 
 # -- FTP-PI (Protocol Interpreter) --

trunk/twisted/test/test_ftp.py

@@ -16 +16 @@
 from twisted.trial import unittest, util
 from twisted.protocols import basic
-from twisted.internet import reactor, protocol, defer, error
+from twisted.internet import reactor, task, protocol, defer, error
 from twisted.internet.interfaces import IConsumer
 from twisted.cred import portal, checkers, credentials
@@ -536 +536 @@
                 ["425 Can't open data connection."])
         return d.addCallback(gotPortNum)
+
+
+
+class DTPFactoryTests(unittest.TestCase):
+    """
+    Tests for L{ftp.DTPFactory}.
+    """
+    def setUp(self):
+        """
+        Create a fake protocol interpreter and a L{ftp.DTPFactory} instance to
+        test.
+        """
+        self.reactor = task.Clock()
+
+        class ProtocolInterpreter(object):
+            dtpInstance = None
+
+        self.protocolInterpreter = ProtocolInterpreter()
+        self.factory = ftp.DTPFactory(
+            self.protocolInterpreter, None, self.reactor)
+
+
+    def test_setTimeout(self):
+        """
+        L{ftp.DTPFactory.setTimeout} uses the reactor passed to its initializer
+        to set up a timed event to time out the DTP setup after the specified
+        number of seconds.
+        """
+        # Make sure the factory's deferred fails with the right exception, and
+        # make it so we can tell exactly when it fires.
+        finished = []
+        d = self.assertFailure(self.factory.deferred, ftp.PortConnectionError)
+        d.addCallback(finished.append)
+
+        self.factory.setTimeout(6)
+
+        # Advance the clock almost to the timeout
+        self.reactor.advance(5)
+
+        # Nothing should have happened yet.
+        self.assertFalse(finished)
+
+        # Advance it to the configured timeout.
+        self.reactor.advance(1)
+
+        # Now the Deferred should have failed with TimeoutError.
+        self.assertTrue(finished)
+
+        # There should also be no calls left in the reactor.
+        self.assertFalse(self.reactor.calls)
+
+
+    def test_buildProtocolOnce(self):
+        """
+        A L{ftp.DTPFactory} instance's C{buildProtocol} method can be used once
+        to create a L{ftp.DTP} instance.
+        """
+        protocol = self.factory.buildProtocol(None)
+        self.assertIsInstance(protocol, ftp.DTP)
+
+        # A subsequent call returns None.
+        self.assertIdentical(self.factory.buildProtocol(None), None)
+
+
+    def test_timeoutAfterConnection(self):
+        """
+        If a timeout has been set up using L{ftp.DTPFactory.setTimeout}, it is
+        cancelled by L{ftp.DTPFactory.buildProtocol}.
+        """
+        self.factory.setTimeout(10)
+        protocol = self.factory.buildProtocol(None)
+        # Make sure the call is no longer active.
+        self.assertFalse(self.reactor.calls)
+
+
+    def test_connectionAfterTimeout(self):
+        """
+        If L{ftp.DTPFactory.buildProtocol} is called after the timeout
+        specified by L{ftp.DTPFactory.setTimeout} has elapsed, C{None} is
+        returned.
+        """
+        # Handle the error so it doesn't get logged.
+        d = self.assertFailure(self.factory.deferred, ftp.PortConnectionError)
+
+        # Set up the timeout and then cause it to elapse so the Deferred does
+        # fail.
+        self.factory.setTimeout(10)
+        self.reactor.advance(10)
+
+        # Try to get a protocol - we should not be able to.
+        self.assertIdentical(self.factory.buildProtocol(None), None)
+
+        # Make sure the Deferred is doing the right thing.
+        return d
+
+
+    def test_timeoutAfterConnectionFailed(self):
+        """
+        L{ftp.DTPFactory.deferred} fails with L{PortConnectionError} when
+        L{ftp.DTPFactory.clientConnectionFailed} is called.  If the timeout
+        specified with L{ftp.DTPFactory.setTimeout} expires after that, nothing
+        additional happens.
+        """
+        finished = []
+        d = self.assertFailure(self.factory.deferred, ftp.PortConnectionError)
+        d.addCallback(finished.append)
+
+        self.factory.setTimeout(10)
+        self.assertFalse(finished)
+        self.factory.clientConnectionFailed(None, None)
+        self.assertTrue(finished)
+        self.reactor.advance(10)
+        return d
+
+
+    def test_connectionFailedAfterTimeout(self):
+        """
+        If L{ftp.DTPFactory.clientConnectionFailed} is called after the timeout
+        specified by L{ftp.DTPFactory.setTimeout} has elapsed, nothing beyond
+        the normal timeout before happens.
+        """
+        # Handle the error so it doesn't get logged.
+        d = self.assertFailure(self.factory.deferred, ftp.PortConnectionError)
+
+        # Set up the timeout and then cause it to elapse so the Deferred does
+        # fail.
+        self.factory.setTimeout(10)
+        self.reactor.advance(10)
+
+        # Now fail the connection attempt.  This should do nothing.  In
+        # particular, it should not raise an exception.
+        self.factory.clientConnectionFailed(None, defer.TimeoutError("foo"))
+
+        # Give the Deferred to trial so it can make sure it did what we
+        # expected.
+        return d
+