Opened 13 years ago

Closed 13 years ago

#173 closed defect (fixed)

incompatibility with Twisted trunk due to setting self.transport.protocol

Reported by: Zooko Owned by:
Priority: major Milestone: 0.6.2
Component: banana Version: 0.6.1
Keywords: twisted test-needed Cc: Zooko, davidsarah

Change History (9)

comment:1 Changed 13 years ago by Zooko

Cc: Zooko added

comment:2 Changed 13 years ago by davidsarah

Cc: davidsarah added
Keywords: twisted added

comment:3 Changed 13 years ago by davidsarah

Twisted mailing list thread: http://twistedmatrix.com/pipermail/twisted-python/2011-April/023823.html

This should preferably be fixed before Twisted 11.1 is released, to stop it from affecting more users. Is there any chance that http://twistedmatrix.com/trac/ticket/3204 (supported API for changing the protocol) will be implemented by 11.1?

comment:4 in reply to:  3 Changed 13 years ago by Zooko

Replying to davidsarah:

This should preferably be fixed before Twisted 11.1 is released, to stop it from affecting more users. Is there any chance that http://twistedmatrix.com/trac/ticket/3204 (supported API for changing the protocol) will be implemented by 11.1?

Glyph's mailing list message made it sound like it foolscap lovers would be the most likely implementors.

comment:5 Changed 13 years ago by Jean-Paul Calderone

I think the fix will be quite simple (at least, one possible fix should be). However, there seem to be no Foolscap unit tests which fail against Twisted trunk@HEAD, so I can't easily test what I have in mind.

Here's a patch, though:

diff --git a/foolscap/negotiate.py b/foolscap/negotiate.py
index f74daba..13a21d8 100644
--- a/foolscap/negotiate.py
+++ b/foolscap/negotiate.py
@@ -1159,7 +1159,8 @@ class Negotiation(protocol.Protocol):
                              )
         b.factory = self.factory # not used for PB code
         b.setTub(self.tub)
-        self.transport.protocol = b
+        self.dataReceived = b.dataReceived
+        self.connectionLost = b.connectionLost
         b.makeConnection(self.transport)
         buf, self.buffer = self.buffer, "" # empty our buffer, just in case
         b.dataReceived(buf) # and hand it to the new protocol

comment:6 Changed 13 years ago by Zooko

Keywords: test-needed added

comment:7 Changed 13 years ago by Brian Warner

Milestone: undecided0.6.2

comment:8 Changed 13 years ago by Brian Warner

I'm trying to trigger the failure. Step one: you probably need pyOpenSSL-0.10 or newer installed, and OS-X 10.6 comes with 0.7 .

My hunch is that the root problem is that the new TLS code (which isn't always used, and it falls back to old code which works fine) is using a new protocol-wrapping mechanism. I'm guessing that the wrapper copies some attributes from .protocol early, such that it doesn't handle later changes correctly.

The patch above might be ok, but one feature I'd like to retain is that the Negotiation object should go away (be GCed) after the protocol is switched. I don't see how to do that with either this self.dataReceived = newprotocol.dataReceived approach, or the less-direct delegate-elsewhere approach used by AMP. It'd probably require a small intermediate Protocol on which intermediate.dataReceived = negotiator.dataReceived, then later gets set to =b.dataReceived. That would allow the (larger) Negotiate object to go away, retaining only the smaller intermediate object.

comment:9 Changed 13 years ago by Brian Warner

Resolution: fixed
Status: newclosed

Ok, fixed by [46e3a04]. I just applied exarkun's patch, plus a change to stop passing an argument into loseConnection (sometimes it works, but sometimes not). I'll defer the release-the-Negotiation-object change for a separate ticket (#183).

Note: See TracTickets for help on using tickets.