Merge lp:~exarkun/divmod.org/pop3-timeouts into lp:divmod.org

[Jean-Paul Calderone]

Branch does a couple things:

  - Adds a check to `setStatus` to not bother doing anything if `grabber` has already been discarded
  - Shuffles things around a bit in `timeoutConnection` so the timeout is actually reflected in the status.

[Tristan Seligmann (mithrandi)]

I haven't actually tested the code, but the diff looks reasonable.

review: Approve

=== modified file 'Quotient/xquotient/grabber.py'
--- Quotient/xquotient/grabber.py	2012-04-28 14:23:40 +0000
+++ Quotient/xquotient/grabber.py	2012-05-11 14:12:27 +0000
@@ -409,10 +409,11 @@
         Disassociate the protocol object from the POP3Grabber and drop the
         connection.
         """
+        msg = u"Timed out waiting for server response."
         addr, peer = self.transport.getHost(), self.transport.getPeer()
         log.msg("POP3GrabberProtocol/%s->%s timed out" % (addr, peer))
-        self.transientFailure(failure.Failure(
-            error.TimeoutError("Timed out waiting for server response.")))
+        self.setStatus(msg)
+        self.transientFailure(failure.Failure(error.TimeoutError(msg)))
         self.stoppedRunning()
         self.transport.loseConnection()
 
@@ -593,7 +594,8 @@
 
 
     def setStatus(self, msg, success=True):
-        self._transact(self.grabber.status.setStatus, msg, success)
+        if self.grabber is not None:
+            self._transact(self.grabber.status.setStatus, msg, success)
 
 
     def shouldRetrieve(self, uidList):

=== modified file 'Quotient/xquotient/test/test_grabber.py'
--- Quotient/xquotient/test/test_grabber.py	2012-04-28 14:23:40 +0000
+++ Quotient/xquotient/test/test_grabber.py	2012-05-11 14:12:27 +0000
@@ -7,6 +7,8 @@
 from twisted.internet import defer, error
 from twisted.mail import pop3
 from twisted.cred import error as ecred
+from twisted.test.proto_helpers import StringTransport
+from twisted.python.failure import Failure
 
 from epsilon import structlike, extime
 
@@ -364,35 +366,88 @@
     """
     Tests for L{xquotient.grabber.ControlledPOP3GrabberProtocol}.
     """
+    def setUp(self):
+        """
+        Create a grabber in a user store.
+        """
+        self.siteStore = store.Store()
+        self.subStore = substore.SubStore.createNew(self.siteStore, ['grabber'])
+        self.userStore = self.subStore.open()
+        self.scheduler = iaxiom.IScheduler(self.userStore)
+
+        self.grabberItem = grabber.POP3Grabber(
+            store=self.userStore, username=u"alice", domain=u"example.com",
+            password=u"secret", running=True,
+            config=grabber.GrabberConfiguration(store=self.userStore))
+        self.grabberItem.scheduled = extime.Time()
+        self.scheduler.schedule(self.grabberItem, self.grabberItem.scheduled)
+
+
     def test_stoppedRunningWithGrabber(self):
         """
         When L{ControlledPOP3GrabberProtocol.stoppedRunning} is called after a
         transient failure, and the protocol instance has an associated grabber,
         that grabber is rescheduled to run immediately.
         """
-        siteStore = store.Store()
-        subStore = substore.SubStore.createNew(siteStore, ['grabber'])
-        userStore = subStore.open()
-        scheduler = iaxiom.IScheduler(userStore)
-
-        grabberItem = grabber.POP3Grabber(
-            store=userStore, username=u"alice", domain=u"example.com",
-            password=u"secret", running=True,
-            config=grabber.GrabberConfiguration(store=userStore))
-        grabberItem.scheduled = extime.Time()
-        scheduler.schedule(grabberItem, grabberItem.scheduled)
-
-        factory = grabber.POP3GrabberFactory(grabberItem, False)
+        factory = grabber.POP3GrabberFactory(self.grabberItem, False)
         protocol = factory.buildProtocol(None)
         protocol.transientFailure(None)
         protocol.stoppedRunning()
-        self.assertEqual(False, grabberItem.running)
+        self.assertEqual(False, self.grabberItem.running)
 
-        scheduled = list(scheduler.scheduledTimes(grabberItem))
+        scheduled = list(self.scheduler.scheduledTimes(self.grabberItem))
         self.assertEqual(1, len(scheduled))
         self.assertTrue(scheduled[0] <= extime.Time())
 
 
+    def test_stoppedRunningAfterTimeout(self):
+        """
+        When L{ControlledPOP3GrabberProtocol} times out the connection
+        due to inactivity, the controlling grabber's status is set to
+        reflect this.
+        """
+        factory = grabber.POP3GrabberFactory(self.grabberItem, False)
+        protocol = factory.buildProtocol(None)
+        protocol.makeConnection(StringTransport())
+        protocol.timeoutConnection()
+        protocol.connectionLost(Failure(error.ConnectionLost("Simulated")))
+
+        self.assertEqual(
+            self.grabberItem.status.message,
+            u"Timed out waiting for server response.")
+
+
+    def test_stoppedRunningAfterListTimeout(self):
+        """
+        When L{ControlledPOP3GrabberProtocol} times out the connection
+        due to inactivity while waiting for a response to a I{UIDL}
+        (list UIDs) command, the controlling grabber's status is set
+        to reflect this.
+        """
+        factory = grabber.POP3GrabberFactory(self.grabberItem, False)
+        protocol = factory.buildProtocol(None)
+        protocol.allowInsecureLogin = True
+        protocol.makeConnection(StringTransport())
+        # Server greeting
+        protocol.dataReceived("+OK Hello\r\n") 
+        # CAPA response
+        protocol.dataReceived("+OK\r\nUSER\r\nUIDL\r\n.\r\n")
+        # USER response
+        protocol.dataReceived("+OK\r\n")
+        # PASS response
+        protocol.dataReceived("+OK\r\n")
+        # Sanity check, we should have gotten to sending the UIDL
+        self.assertTrue(
+            protocol.transport.value().endswith("\r\nUIDL\r\n"),
+            "Failed to get to UIDL: %r" % (protocol.transport.value(),))
+
+        protocol.timeoutConnection()
+        protocol.connectionLost(Failure(error.ConnectionLost("Simulated")))
+        self.assertEqual(
+            self.grabberItem.status.message,
+            u"Timed out waiting for server response.")
+
+
 
 class GrabberConfigurationTestCase(unittest.TestCase):
     """