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

[Adi Roiban]

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.

I have this code which will need your own server cert+key file:

https://gist.github.com/adiroiban/8e0be840f81750f88a587e634ddfb194

I tried it on twisted/trunk and when SSL is used the output is

server:client connected
client:connected
client:send-message
server:data received
client:connection-lost
client:transport-status:1:wrapped-transport->0
server:client disconnected
client:transport-status:1:wrapped-transport->0

while without SSL the output is:

server:client connected
client:connected
client:send-message
server:data received
server:client disconnected
client:connection-lost
client:transport-status:0:wrapped-transport->0
client:transport-status:0:wrapped-transport->0

------------

My current finding is that the reactor will call connection lost on
t.i.abstract.FileDescriptor.connectionLost and not on
t.protocols.policies.ProtocolWrapper

so it seems that when SSL is used the wrapped protocol is registered
instead of the wrapping protocol.

------

I am doing something wrong here?

Is this the expected behavior or this is a bug?

Thanks!

[Glyph Lefkowitz]

Post by Adi Roiban
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 :-).

Post by Adi Roiban
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 <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 <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.

Post by Adi Roiban
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.

Post by Adi Roiban
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.

-glyph

[Adi Roiban]

Post by Adi Roiban
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
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!

[Glyph Lefkowitz]

Post by Adi Roiban

Post by Adi Roiban
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
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
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