[Twisted-Python] Re: [Twisted-commits] r16582 - Remove util.wait and all uses.

[Jean-Paul Calderone]

On Sun, 09 Apr 2006 21:21:34 -0600, Jonathan Lange <jml at wolfwood.twistedmatrix.com> wrote:
>Author: jml
>Date: Sun Apr  9 21:21:33 2006
>New Revision: 16582
>
>Modified:
>   trunk/twisted/conch/test/test_filetransfer.py
>   trunk/twisted/conch/test/test_telnet.py
>   trunk/twisted/lore/nevowlore.py
>   trunk/twisted/mail/test/test_mail.py
>   trunk/twisted/mail/test/test_pop3client.py
>   trunk/twisted/names/test/test_names.py
>   trunk/twisted/protocols/ftp.py
>   trunk/twisted/test/test_adbapi.py
>   trunk/twisted/test/test_defgen.py
>   trunk/twisted/test/test_ftp.py
>   trunk/twisted/test/test_reflector.py
>   trunk/twisted/trial/runner.py
>   trunk/twisted/trial/test/test_util.py
>   trunk/twisted/trial/unittest.py
>   trunk/twisted/trial/util.py
>   trunk/twisted/web2/test/test_stream.py
>Log:
>Remove util.wait and all uses.
>
> [snip]
>
>Modified: trunk/twisted/protocols/ftp.py
>==============================================================================
>--- trunk/twisted/protocols/ftp.py	(original)
>+++ trunk/twisted/protocols/ftp.py	Sun Apr  9 21:21:33 2006
>@@ -660,7 +660,7 @@
>             if cmd == 'PASS':
>                 return self.ftp_PASS(*params)
>             else:
>-                return BAD_CMD_SEQ
>+                return BAD_CMD_SEQ, "PASS required after USER"
>
>         elif self.state == self.AUTHED:
>             method = getattr(self, "ftp_" + cmd, None)
>@@ -672,7 +672,7 @@
>             if cmd == 'RNTO':
>                 return self.ftp_RNTO(*params)
>             else:
>-                return BAD_CMD_SEQ, "Blah"
>+                return BAD_CMD_SEQ, "RNTO required after RNFR"
>
>
>     def ftp_USER(self, username):
>

Hmmm.  I don't see any changes related to wait() in these hunks. ;)

> [snip]
>Modified: trunk/twisted/test/test_ftp.py
>==============================================================================
>--- trunk/twisted/test/test_ftp.py	(original)
>+++ trunk/twisted/test/test_ftp.py	Sun Apr  9 21:21:33 2006
> [snip]
>@@ -86,7 +82,10 @@
>         # Connect a client to it
>         portNum = self.port.getHost().port
>         clientCreator = protocol.ClientCreator(reactor, ftp.FTPClientBasic)
>-        self.client = wait(clientCreator.connectTCP("127.0.0.1", portNum))
>+        d = clientCreator.connectTCP("127.0.0.1", portNum)
>+        def gotClient(client):
>+            self.client = client
>+        return d.addCallback(gotClient)
>
>     def tearDown(self):
>         # Clean up sockets

While it doesn't appear that this change *introduced* a new race condition, it does seem as though it has jostled things so as to cause an existing one to start going the wrong way.

This seems to be a pretty common one in the test suite, so everyone pay attention.

Just because one side of a TCP connection has had its connection-established-successfully callback fired (ie, buildProtocol on the Factory) does not mean that both sides of the TCP connection have had this callback fired.  Quite often, in fact, this is not the case (particularly on platforms you forgot to run the unit tests on ;).

This ftp setUp needs to be made to wait for both the server and the client to become connected before proceeding.  The failure to do so can be seen as a new failing test on the reactors buildslave in the Qt step.

(Of course, the Qt step was already red, because a previous branch uncovered still another race condition.)

Please revert the branch or fix the race post-haste.  If someone would take a look at the failing tcp half-close test on that reactor, I would appreciate it, otherwise I'll try to get to it sometime in the next couple days.

Jean-Paul