iocpreactor calls factory.clientConnectionLost before protocol.connectionLost

[garthk]

I get twisted.internet.error.ConnectionDone instead of a response when I hit a Django based XML-RPC server using Twisted.Web 2.5 on Windows, but only when using iocpreactor.

Workaround: don't use iocpreactor.

[garthk]

To reproduce:

• Install Django from SVN
• Install Twisted 2.50
• Extract iocpdone.tar.gz
• Run done.py

Both Twisted and xmlrpclib calls to the SimpleXMLRPCServer will succeed.

The xmlrpclib call to Django will succeed. The Twisted call will fail. On my machine, the reactor also fails to stop -- the final print won't execute.

C:\dev\workspace\MDS>done
Sleeping 3s to give Django time to start...
Validating models...
0 errors found

Django version 0.97-pre-SVN-140, using settings 'done.settings'
Development server is running at http://127.0.0.1:8023/
Quit the server with CTRL-BREAK.
---------------------------------------------------------------------------
Testing Python client to SimpleXMLRPCServer (http://127.0.0.1:8024/)...
.psf - - [25/Feb/2008 15:38:46] "POST / HTTP/1.0" 200 -
=> 'kjhds'
---------------------------------------------------------------------------
Testing Twisted client to SimpleXMLRPCServer (http://127.0.0.1:8024/)...
.psf - - [25/Feb/2008 15:38:46] "POST / HTTP/1.0" 200 -
=> 'kjhds'
---------------------------------------------------------------------------
Testing Python client to Django (http://127.0.0.1:8023/RPC2/)...
"<?xml version='1.0'?>\n<methodCall>\n<methodName>Echo</methodName>\n<params>\n<param>\n<value><string>kjhds</string></value>\n</param>\n</params>\n</methodCall>\n"
Content-Type: text/html; charset=utf-8

<?xml version='1.0'?>
<methodResponse>
<params>
<param>
<value><string>kjhds</string></value>
</param>
</params>
</methodResponse>

[25/Feb/2008 15:38:46] "POST /RPC2/ HTTP/1.0" 200 131
=> 'kjhds'
---------------------------------------------------------------------------
Testing Twisted client to Django (http://127.0.0.1:8023/RPC2/)...
'<?xml version="1.0"?>\n<methodCall>\n<methodName>Echo</methodName>\n<params>\n<param>\n<value><string>kjhds</string></value>\n</param>\n</params>\n\n</methodCall>\n'
Content-Type: text/html; charset=utf-8

<?xml version='1.0'?>
<methodResponse>
<params>
<param>
<value><string>kjhds</string></value>
</param>
</params>
</methodResponse>

[25/Feb/2008 15:38:46] "POST /RPC2/ HTTP/1.0" 200 131
=> <twisted.python.failure.Failure <class 'twisted.internet.error.ConnectionDone'>>

[garthk]

Code to reproduce the fault (revised)

[therve]

You can't restart the reactor. I suspect that if you change the order of the runtest calls, the first will succeed while the second will not.

[ghazel]

Replying to therve:

You can't restart the reactor. I suspect that if you change the order of the runtest calls, the first will succeed while the second will not.

Not true. Reversed, the failure is still in the Twisted client Django server test.

[ghazel]

The difference is that t.i.iocpreactor.abstract.ConnectedSocket.connectionLost causes clientConnectionLost to be called on the factory before connectionLost is called on the protocol. This is the reverse of t.i.tcp.BaseClient.connectionLost, and t.w.xmlrpc.QueryFactory treats that as an error before the protocol can process the data.

The fix might be as simple as:

Index: twisted/internet/iocpreactor/abstract.py
===================================================================
--- twisted/internet/iocpreactor/abstract.py    (revision 22696)
+++ twisted/internet/iocpreactor/abstract.py    (working copy)
@@ -122,7 +122,6 @@
         # this should call closesocket() and kill it dead!
         self.socket.close()
         del self.socket
-        self.sf.connectionLost(reason)
         try:
             protocol.connectionLost(reason)
         except TypeError, e:
@@ -135,6 +134,7 @@
                 protocol.connectionLost()
             else:
                 raise
+        self.sf.connectionLost(reason)

     def startReading(self):
         if self.state != "connected":

[exarkun]

It would be good if the relative ordering of these two events were defined, documented, and tested.

It's probably best that it be the same across all reactors, but I'm not sure it's actually a good idea to write code that depends on the relative ordering.

Another fix for this problem is probably to get rid of the clientConnectionLost method on _QueryFactory. Then it won't get in the way of the successful code path, regardless of the order the methods are called in.

[pahan]

(In [22961]) branching!

[exarkun]

Looks good

[pahan]

(In [22972]) Merge lose-order-3055: add test to check order of reactor callbacks

Checks that protocol.connectionLost and factory.clientConnectionLost are called in the right order.

Author: PenguinOfDoom Reviewer: exarkun Fixes #3055