On 2016-12-17, at 23:18, Glyph Lefkowitz <glyph@twistedmatrix.com> wrote:
On Dec 17, 2016, at 6:11 AM, exvito here <ex.vitorino@gmail.com> wrote:
[...]
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.
They're not "destroyed", exactly.
The way to think about this is that the reactor, while running, is always on the stack. The stack is the root of what's currently 'active' in the garbage collector. The reactor then has references to various things - in this case, TCP transports and timed DelayedCall objects. The transport has a reference to the protocol, which is what keeps both of them "usable". The DelayedCall has a reference to your function, and your function to anything it needs to do its work.
Things can be removed from and added back to the reactor at any time - for example, a transport may be removed without being closed by calling .stopReading() and .stopWriting() on it. If it gets garbage collected, its socket will be closed, but because the reactor didn't close it, you won't get a normal connectionLost notification.
Sure, I had that clear in my understanding, thanks for the additional details on the stopReading/stopWriting -- I'm not familiar with low lever reactor details and, maybe, who knows, one day I'll put myself to the interesting exercise/challenge of playing around writing a reactor. Just to improve my understanding of these details.
So rather than thinking of a Transport as having a precisely-defined lifecycle - although it does have certain states it goes through; connecting/connected/disconnecting/disconnected - the best way to think about it is as a graph of objects that the reactor is doing stuff to.
The bottom line (in my mind) is: is it correct or not to assume that, by design, once a Transport instance has been disconnected that same instance will never be connected again, regardless of when exactly that object is destroyed or garbage collected?
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.
If you start logging a warning when write is called on a closed transport, then that means any code that wants to be warnings-clean needs to check to see if the transport is writable before writing to it. This adds a lot of complexity to that sort of message-formatting code, and potentially makes it less efficient. After all, if you have to check each write() call, that will strongly encourage you to make your message-formatting happen entirely in memory, so you always have a big string that represents the whole message, just in case a transport.write() in the middle would encounter an error.
I understand that and find it perfectly acceptable. I just tend to err on the extra-cautious side when facing data-loss / data-integrity possibilities and, in the many scenarios I've used Twisted in, performance was never an issue while semantic correctness and implementation resiliency tend to need fixes and improvements. :) One could argue, though, that someone looking for warnings-clean code (who doesn't, even if the real world and interfacing with it most often prevents it?) might like to know that their "perfect" (lots of quotes here!) warnings-clean code writes won't go anywhere after connectionLost which, in some cases, may help pinpoint an underlying design / implementation issue. I'd like to underline, though, that I'm not insisting on the "log warning" aspect I raised before. Like I said, the status quo is very good, thank you. Just sharing a few thoughts.
More importantly, when users start seeing a warning like this, they often develop an incorrect intuition about what's going on with the bytes; one of the _most_ frequently asked questions about how to write Twisted protocols is "how do I tell if my bytes got to the other end without sending an application-level acknowledgement". The answer, of course, is "that's impossible" but if users see that sometimes it warns them when the connection's closed, it reinforces the idea that there must be a way.
If you call .write, there is always the possibility that the connection _looks_ open to Twisted, but in fact is closed on the wire, and those bytes will be lost. So it's important to me that we behave the same way in both cases.
Yes, I can relate to that common misconception regarding delivery confirmation, throughout the years, working with other people. Networking is non-trivival and there are lots of parts out of application control that can behave in unexpected ways and/or go wrong.
That said, it's not like there's no room for improvement here! The solution _I'd_ really like to see for this problem is improved visualization of what the reactor is doing. Something that drew a graph of the reactor, and open connections, and pending timed calls would allow you to instantly see that your timed call was not connected to an active connection, and correct your code.
Well, trial already does that in part, doesn't it? It complains loudly about unclean reactor state after each test. It just doesn't relate timed calls to connections, which sounds like a very non-obvious challenge to handle at the reactor level: how could the reactor know that a given timed call callback will make use of a particular connection? That does sound like a very interesting capability, though.
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.
Don't worry, the design is pretty fixed - and if you managed to provoke such a discussion, then maybe it does need to change! But I doubt it :)
I'm glad it is pretty fixed. That has always given me the confidence to work with it throughout the years. Thanks and congratulations for a very solid piece of code! Having said this, again very humbly, who knows what the future may bring! ;) Thanks for taking the time to respond. Regards, -- exvito