[Twisted-Python] ITransport.write after IProtocol.connectionLost -- no failure/exception?

[exvito here]

On Fri, Dec 16, 2016 at 8:27 PM, Glyph Lefkowitz
<glyph at twistedmatrix.com> wrote:
>
>> On Dec 16, 2016, at 5:22 AM, Cory Benfield <cory at lukasa.co.uk> wrote:
>>
>>> On 15 Dec 2016, at 12:07, exvito here <ex.vitorino at gmail.com> wrote:
>>>
>>> Dear all,
>>>
>>> The subject says most of it. While diagnosing a behavior on a somewhat
>>> large codebase, I found out something somewhat surprising -- see
>>> subject -- which is replicated with the minimal code example below.
>>>
>>> It is a LineReceiver client that:
>>> 1. Connects to 127.0.0.1:10000.
>>> 2. Sends a line of text.
>>> 3. Disconnects one second later.
>>> 4. Tries to send another line of text, one second after disconnection.
>>>
>>> I expected step 4 to fail somehow, given that Twisted promptly
>>> detected that the connection closed and IProtocol.connectionLost was
>>> called, as documented. Nevertheless, it does not fail.
>>
>> I can’t speak for the design of Twisted, but this is definitely an intended implementation behaviour: because twisted’s write method is non-blocking, it is generally nicer to avoid write exploding in your face when disconnected.
>
> I can! The main design feature of this approach is that if you have a loop like this:
>
> for x in messages:
>     self.transport.write(self.encode(x))
>
> you should not have to write it to be:
>
> for x in messages:
>     if self._flagISetInConnectionLost:
>         break
>     self.transport.write(self.encode(x))
>
> just to avoid seeing tracebacks in your logs.
>
> If you care about the disconnection event, implement connectionLost to do the thing that needs to be done (in this case, stop your LoopingCall).  That's what that callback is there for!


Thanks for your input Cory and Glyph.

I do agree that a well written protocol should not
self.transport.write after connectionLost -- it does not make any kind
of sense to do that. It's just that the one I was debugging was doing
it in its app-level keep alive messages triggered by
IReactorTime.callLater which wasn't properly cancelled on
connectionLost. This, in turn, lead to unpexpected app-level keep
alive timeouts after disconnection which, while acceptable (depending
on app/protocol design), were having a negative impact on the overall
connection/protocol teardown and cleanup.

Having said this, the fact that self.transport.write does not complain
after connectionLost just made my analysis and debugging more
difficult (yes, I was aware that it is non-blocking -- thanks Cory --
and that when talking to the network the only confirmation of delivery
must be obtained from a received message stating that fact).

All in all, I was just looking for confirmation from a conceptual /
design standpoint. That was my purpose and it is now clear. Thanks.

Going a little bit further, in this context, I wonder if my
understanding of the Protocol / Transport objects lifecycle is 100%
clear. I think there is a one to one relationship in the sense that,
simply put, on an incoming connection the underlying machinery
instantiates a Protocol via the Factory's buildProtocol method,
instantiates a Transport object and then calls Protocol.makeConnection
to "attach" the transport to the protocol; when the connection is
closed, the Transport calls connectionLost on the Protocol and, at
some point, those two objects are no longer "usable" -- they've done
their job and will be destroyed if no app level references are kept to
them.

If this is correct, in particular the fact that a given Transport
instance is not reusable after disconnection (if it is, could you
please point out such a case?), wouldn't it be valuable to log a
warning when write after disconnect is called? (agreed, raising an
Exception would be too much and break compatiblity). I find two
reasons for that: on one hand, given that it would have made my
debugging easier, it may help other developers pin point such cases;
on the other, the fact that Twisted itself logs messages on several
occasions, such as, for example, the default noisy Factory on doStart
/ doStop. Adding to that, the fact that, if I'm reading the code
correctly, ITransport.write just throws away the data for TCP
transports since it falls back to FileDescriptor.write that simply
returns if "not self.connected or self._writeDisconnected" in
src/twisted/internet/abstract.py.

Thoughts? (or corrections, of course?)

Again, thanks for your time and input.
Regards,

PS: I don't mean to start a huge discussion on the topic or question
the current design. It is very nice as it is, thank you! I'm just
looking for improving my understanding and, humbly, contributing back.
--
exvito