Opened 3 years ago

Closed 3 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 3 years ago by zooko

  • Cc zooko added

comment:2 Changed 3 years ago by davidsarah

  • Cc davidsarah added
  • Keywords twisted added

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

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 3 years ago by zooko

  • Keywords test-needed added

comment:7 Changed 3 years ago by warner

  • Milestone changed from undecided to 0.6.2

comment:8 Changed 3 years ago by 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 3 years ago by warner

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

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.