Twisted r31537 and Foolscap incompatibility

[billnoon]

The new ssl implementation added in [31537] #4854 is incompatible with Foolscap (v0.6.1).

The ConnectionMixin is treating the self.protocol object as a transport and calls write() and loseConnection() on it.

Here is a traceback:

	Traceback (most recent call last):
	  File "/usr/lib64/python2.6/site-packages/foolscap-0.6.1_-py2.6.egg/foolscap/slicers/root.py", line 107, in send
	    d.callback(None)
	  File "/usr/lib64/python2.6/site-packages/Twisted-11.0.0_r31548-py2.6-linux-x86_64.egg/twisted/internet/defer.py", line 361, in callback
	    self._startRunCallbacks(result)
	  File "/usr/lib64/python2.6/site-packages/Twisted-11.0.0_r31548-py2.6-linux-x86_64.egg/twisted/internet/defer.py", line 455, in _startRunCallbacks
	    self._runCallbacks()
	  File "/usr/lib64/python2.6/site-packages/Twisted-11.0.0_r31548-py2.6-linux-x86_64.egg/twisted/internet/defer.py", line 542, in _runCallbacks
	    current.result = callback(current.result, *args, **kw)
	--- <exception caught here> ---
	  File "/usr/lib64/python2.6/site-packages/foolscap-0.6.1_-py2.6.egg/foolscap/banana.py", line 216, in produce
	    self.pushSlicer(slicer, obj)
	  File "/usr/lib64/python2.6/site-packages/foolscap-0.6.1_-py2.6.egg/foolscap/banana.py", line 352, in pushSlicer
	    openID = self.sendOpen()
	  File "/usr/lib64/python2.6/site-packages/foolscap-0.6.1_-py2.6.egg/foolscap/banana.py", line 481, in sendOpen
	    int2b128(openID, self.transport.write)
	  File "/usr/lib64/python2.6/site-packages/foolscap-0.6.1_-py2.6.egg/foolscap/banana.py", line 24, in int2b128
	    stream(chr(0))
	  File "/usr/lib64/python2.6/site-packages/Twisted-11.0.0_r31548-py2.6-linux-x86_64.egg/twisted/internet/_newtls.py", line 176, in write
	    self.protocol.write(bytes)
	exceptions.AttributeError: 'Broker' object has no attribute 'write'

This could be caused by foolscap setting the protocol attribute on the transport in negotiate.py  https://github.com/warner/foolscap/blob/master/foolscap/negotiate.py#L1162

[exarkun]

Fixing description markup

[exarkun]

Setting an attribute on a transport object is pretty sketchy. I am sympathetic to breakage, but not as sympathetic to breakage caused by messing around with attributes that don't belong to you.

Someone should file a bug against Foolscap to have it stop doing this.

It's not clear how we might fix this in Twisted (aside from reverting the change to the SSL implementation).

[zooko]

In  this tpml message, Glyph wrote:

If foolscap needs to switch protocols mid-stream, it either needs to do what AMP does, or contribute a real fix for that ticket. (And then require a new version of Twisted :).)

[davidsarah]

foolscap ticket is  http://foolscap.lothar.com/trac/ticket/173 .

What does AMP do exactly?

[exarkun]

AMP does this exactly: http://twistedmatrix.com/trac/browser/trunk/twisted/protocols/amp.py#L1983

Combined with:

http://twistedmatrix.com/trac/browser/trunk/twisted/protocols/amp.py#L2045

One could imagine an even cleaner approach than this, I expect, but the idea would be the same: don't mess with the transport, handle all the book-keeping at the protocol level.

[exarkun]

Some follow-up discussion happened on IRC. Despite the fact that protocol is a public attribute on the transport (since it doesn't start with an underscore), setting it to something new still isn't a supported way to get any particular behavior. It's not part of the transport interface (which is well defined) and it's not documented on its own.

Since the guarantee of connectTCP and listenTCP is just that the transport will provide ITCPTransport, it's already possible that different reactor implementations might already differ in their handling of this attribute. In fact, IOCP reactor already had its startTLS supported implemented the way #4854 changed the rest of the reactors to work (and IOCP reactor never had another startTLS implementation that would have worked with Foolscap). So in some cases, it was already broken to rely on setting the protocol attribute to do this. I'm not saying that's a sufficient reason to break cases where it was working, but it's part of the argument.

Of course, the main reason to allow the change that #4854 introduced is that the change did something useful - it fixed an SSL bug, it simplified the SSL implementation, and it improved SSL performance. In order to be able to do things like that, we need to be able to change the implementation details of interfaces. However, we should be better about keeping non-interface stuff private, and I think we have been in more recent code (but transport.protocol is oooooooold).

So, the conclusion is that I don't think we're going to try to preserve the old protocol setting behavior. Foolscap (and any other users doing something similar, though I don't know of any yet) will need to deal with this in order to work with Twisted 11.1.