[Twisted-Python] SSL4ClientEndpoint not updating the transport's connected flag on connection lost
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! -- Adi Roiban
On Aug 11, 2016, at 06:59, Adi Roiban <adi@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/6022f65903eda2f8df74b2d284b8ef359df2... <https://github.com/twisted/twisted/blob/6022f65903eda2f8df74b2d284b8ef359df2...> As you can see, neither it, nor the superclass that it upcalls to <https://github.com/twisted/twisted/blob/6022f65903eda2f8df74b2d284b8ef359df2... <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. -glyph
On 20 August 2016 at 08:37, Glyph Lefkowitz <glyph@twistedmatrix.com> wrote:
On Aug 11, 2016, at 06:59, Adi Roiban <adi@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/6022f65903eda2f8df74b2d284b8ef359df2...
As you can see, neither it, nor the superclass that it upcalls to <https://github.com/twisted/twisted/blob/6022f65903eda2f8df74b2d284b8ef359df2...> 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
On Aug 20, 2016, at 2:13 AM, Adi Roiban <adi@roiban.ro> wrote:
On 20 August 2016 at 08:37, Glyph Lefkowitz <glyph@twistedmatrix.com <mailto:glyph@twistedmatrix.com>> wrote:
On Aug 11, 2016, at 06:59, Adi Roiban <adi@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/6022f65903eda2f8df74b2d284b8ef359df2...
As you can see, neither it, nor the superclass that it upcalls to <https://github.com/twisted/twisted/blob/6022f65903eda2f8df74b2d284b8ef359df2...> 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
participants (2)
-
Adi Roiban -
Glyph Lefkowitz