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

[Adi Roiban]

On 20 August 2016 at 08:37, Glyph Lefkowitz <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 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!
-- 
Adi Roiban