[Twisted-Python] SSL4ClientEndpoint not updating the transport's connected flag on connection lost

[Glyph Lefkowitz]

> On Aug 20, 2016, at 2:13 AM, Adi Roiban <adi at roiban.ro> wrote:
> 
> On 20 August 2016 at 08:37, Glyph Lefkowitz <glyph at twistedmatrix.com <mailto:glyph at twistedmatrix.com>> wrote:
>> 
>> On Aug 11, 2016, at 06:59, Adi Roiban <adi at roiban.ro> wrote:
>> 
>> Hi,
>> 
>> I have observed that when a SSL4ClientEndpoint was used, the
>> protocol's transport `connected` attribute is not updated by the
>> wrapper when the connection is lost.
>> 
>> 
>> Hey Adi,
>> 
>> I took a look through the code, and I think it's a lot simpler than you
>> realize :-).
>> 
>> My current finding is that the reactor will call connection lost on
>> t.i.abstract.FileDescriptor.connectionLost and not on
>> t.protocols.policies.ProtocolWrapper
>> 
>> 
>> This is definitely not true, or dropping the connection just wouldn't work
>> at all, and you wouldn't get connectionLost on your protocol.  The call path
>> is FileDescriptor.connectionLost -> endpoint wrapper protocol.connectionLost
>> -> *TLS wrapper protocol.connectionLost* -> your protocol connectionLost.
>> 
>> The bold-faced protocol is the interesting one, because that is the
>> 'transport' object that your protocol sees.  Here's that object's
>> connectionLost method:
>> 
>> https://github.com/twisted/twisted/blob/6022f65903eda2f8df74b2d284b8ef359df24904/src/twisted/protocols/tls.py#L488
>> 
>> As you can see, neither it, nor the superclass that it upcalls to
>> <https://github.com/twisted/twisted/blob/6022f65903eda2f8df74b2d284b8ef359df24904/src/twisted/protocols/policies.py#L123>
>> sets the "connected" attribute.  So the method is totally called, it just
>> doesn't do what you expect.
>> 
>> so it seems that when SSL is used the wrapped protocol is registered
>> instead of the wrapping protocol.
>> 
>> 
>> I'm not sure what you mean here.
>> 
>> I am doing something wrong here?
>> 
>> 
>> Is this the expected behavior or this is a bug?
>> 
>> 
>> Probably both, kinda?  This attribute is undocumented; probably an old
>> implementation accident.  Technically it's public, but you probably
>> shouldn't be depending on it.  However, it's silly that this is inconsistent
>> between TLS transports and raw TCP transports, so fixing up
>> ProtocolWrapper.connectionLost to set 'connected' to False is probably not a
>> bad change.
> 
> Thanks for your comments.
> 
> I asked the question about SSL4ClientEndpoint in parallel with the
> question about twisted.internet.protocol.Protocol.connected ... so by
> that time I was very confused about all this.
> 
> I have created a ticket for this
> https://twistedmatrix.com/trac/ticket/8774 <https://twistedmatrix.com/trac/ticket/8774> and will try to work on it.
> 
> I have searched to code for `self.connected =` and there are a few of
> places where it is not set at connection lost:
> 
> INotify, PTYProcess, udp.Port (not sure why we have this state for
> UDP) , unix.Port (but is set to 0 for unix.DatagramPort)
> iocpreactor.udp.Port (but is set to False for iocpreactor.tcp.Port)
> 
> I will try to follow up with a ticket+branch.
> 
> Thanks!

No problem!  Glad we could clear that up - it was definitely a bit confusing at first for me as well, which is why I wanted to investigate to the bottom of it :).

Another option here, though, is to simply deprecate the attribute; like I said, it's not documented and if you're properly implementing everything by reacting to events, it's not very useful.  Why do you need it?

-glyph

-------------- next part --------------
An HTML attachment was scrubbed...
URL: </pipermail/twisted-python/attachments/20160820/55ffa95a/attachment-0002.html>