Opened 7 years ago

Closed 6 years ago

#3055 defect closed fixed (fixed)

iocpreactor calls factory.clientConnectionLost before protocol.connectionLost

Reported by: garthk Owned by: PenguinOfDoom
Priority: highest Milestone:
Component: core Keywords: windows, iocpreactor
Cc: Branch: branches/lose-order-3055
(diff, github, buildbot, log)
Author: pahan Launchpad Bug:

Description

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.

Attachments (1)

iocpdone.tar.gz (3.4 KB) - added by garthk 7 years ago.
Code to reproduce the fault (revised)

Download all attachments as: .zip

Change History (10)

comment:1 Changed 7 years ago by garthk

To reproduce:

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'>>

Changed 7 years ago by garthk

Code to reproduce the fault (revised)

comment:2 follow-up: Changed 7 years ago by 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.

comment:3 in reply to: ↑ 2 Changed 7 years ago by 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.

comment:4 Changed 7 years ago by ghazel

  • Component changed from web to core
  • Keywords iocpreactor added
  • Owner changed from jknight to glyph
  • Summary changed from XML-RPC fails to Django with iocpreactor on Windows to iocpreactor calls factory.clientConnectionLost before protocol.connectionLost

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":

comment:5 Changed 7 years ago by 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.

comment:6 Changed 6 years ago by pahan

  • author set to pahan
  • Branch set to branches/lose-order-3055

(In [22961]) branching!

comment:7 Changed 6 years ago by PenguinOfDoom

  • Keywords review added
  • Priority changed from normal to highest

comment:8 Changed 6 years ago by exarkun

  • Keywords review removed
  • Owner changed from glyph to PenguinOfDoom

Looks good

comment:9 Changed 6 years ago by pahan

  • Resolution set to fixed
  • Status changed from new to closed

(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

Note: See TracTickets for help on using tickets.