Merge lp:~gary/lazr.smtptest/threadsafety into lp:lazr.smtptest

[Gary Poster]

Hi Barry. As we discussed, these changes make lazr.smtptest more thread-safe for other users of asyncore (such as Launchpad). I am hopeful that this will reduce spurious test failures in tests run in Launchpad's appserver layer. It seems to do the trick for the particular problem that I was trying to diagnose and solve (though the intermittent and opaque nature of these sorts of things means I only have statistics, not absolute confidence).

The addition of add_channel for the server and the channel are to work around the fact that the smtpd base classes (in Python 2.6 and 2.7) do not expose the asyncore map argument to the __init__. The options I saw were to copy over the __init__ code of the two classes, which seemed unattractive; or to try and come up with a hack that involved less code copying while trying to be as un-ugly as possible. add_channel is one of the few methods in the asyncore dispatcher that actually use the mapping, and the first one in the dispatcher's lifecycle to do so, so it seems a relatively reasonable an safe place to do the work, given the unfortunate circumstances. Other ideas welcome.

The other change I made was to actually close the sockets when the server is told to stop: before, we simply cleared out the map, without actually closing the sockets.

I still need to run the LP tests with these changes; I'm more suspicious of the second, smaller change causing a problem to LP than the first.

Thank you!

Gary

[Gary Poster (gary)]

All LP tests pass with this branch.

[Barry Warsaw (barry)]

All the Mailman 3 tests passed with this branch.

review: Approve

=== modified file 'src/lazr/smtptest/NEWS.txt'
--- src/lazr/smtptest/NEWS.txt	2009-07-07 22:47:59 +0000
+++ src/lazr/smtptest/NEWS.txt	2011-07-06 21:31:16 +0000
@@ -2,6 +2,17 @@
 NEWS for lazr.smtptest
 ======================
 
+1.3 (2011-06-07)
+================
+
+- Make the test server thread-safe with other code that starts an asyncore
+  loop.  Requires Python 2.6 or 2.7.
+
+- Be cleaner about stopping the server: before, it left sockets running
+  ans simply cleared out the socket map.  In an associated change, the EXIT
+  smtp command sends the reply first, and then shuts down the server, rather
+  than the other way around.
+
 1.2 (2009-07-07)
 ================
 

=== modified file 'src/lazr/smtptest/server.py'
--- src/lazr/smtptest/server.py	2009-06-30 00:40:38 +0000
+++ src/lazr/smtptest/server.py	2011-07-06 21:31:16 +0000
@@ -41,14 +41,14 @@
     """A channel that can reset the mailbox."""
 
     def __init__(self, server, connection, address):
+        self._server = server
         smtpd.SMTPChannel.__init__(self, server, connection, address)
-        self._server = server
 
     # pylint: disable-msg=W0613
     def smtp_EXIT(self, argument):
         """EXIT - a new SMTP command to cleanly stop the server."""
+        self.push('250 Ok')
         self._server.stop()
-        self.push('250 Ok')
 
     def smtp_RSET(self, argument):
         """RSET - hijack this to reset the server instance."""
@@ -66,6 +66,16 @@
             # Nothing here can affect the outcome.
             pass
 
+    def add_channel(self, map=None):
+        # This has an old style base class.  We want to make _map equal to
+        # our server's socket_map, to make this thread safe with other
+        # asyncores.  We do it here, overriding the behavior of the asyncore
+        # __init__ as soon as we can, without having to copy over code from
+        # the rest of smtpd.SMTPChannel.__init__ and modify it.
+        if self._map is not self._server.socket_map:
+            self._map = self._server.socket_map
+        smtpd.SMTPChannel.add_channel(self, map)
+
 
 class Server(smtpd.SMTPServer):
     """An SMTP server."""
@@ -80,10 +90,21 @@
         """
         self.host = host
         self.port = port
+        self.socket_map = {}
         smtpd.SMTPServer.__init__(self, (host, port), None)
         self.set_reuse_addr()
         log.info('[SMTPServer] listening: %s:%s', host, port)
 
+    def add_channel(self, map=None):
+        # This has an old style base class.  We want to make _map equal to
+        # socket_map, to make this thread safe with other asyncores.  We do
+        # it here, overriding the behavior of the SMTPServer __init__ as
+        # soon as we can, without having to copy over code from the rest of
+        # smtpd.SMTPServer.__init__ and modify it.
+        if self._map is not self.socket_map:
+            self._map = self.socket_map
+        smtpd.SMTPServer.add_channel(self, map)
+
     def handle_accept(self):
         """Handle connections by creating our own Channel object."""
         connection, address = self.accept()
@@ -106,12 +127,11 @@
     def start(self):
         """Start the asyncore loop."""
         log.info('[SMTPServer] starting asyncore loop')
-        asyncore.loop()
+        asyncore.loop(map=self.socket_map)
 
     def stop(self):
         """Stop the asyncore loop."""
-        asyncore.socket_map.clear()
-        asyncore.close_all()
+        asyncore.close_all(map=self.socket_map)
         self.close()
 
     # pylint: disable-msg=R0201

=== modified file 'src/lazr/smtptest/version.txt'
--- src/lazr/smtptest/version.txt	2009-09-04 19:06:38 +0000
+++ src/lazr/smtptest/version.txt	2011-07-06 21:31:16 +0000
@@ -1,1 +1,1 @@
-1.2
+1.3