#1662 enhancement new
pauseProducing() should not block POLL_DISCONNECTED
|Reported by:||andrea||Owned by:|
|Cc:||andrea, jknight, glyph, exarkun||Branch:|
This is an enhancement to consider for the mid/long term.
The basic problem is that I can't throttle the input/output from the network using the registerProducer (that calls pauseProducing()) without risking to hang forever. Hanging forever means pratically disabling the reconnectingclient behavior.
It's a significant problem because it's in the twisted core, in pollreactor and in general in the reactor API (I doubt it can be fixed without some changes to the reactor API).
In other words the deficiency is that writing a network app with twisted is like writing a network app in a OS that only supports POLLIN and POLLOUT. All other flags are unusable with twisted:
POLLPRI POLLERR POLLHUP POLLNVAL
I need to poll the fds in pausedProducer state only for POLLERR|POLLHUP|POLLNVAL. Currently either I poll them for POLLIN or I don't poll them at all, this is a big limitation if you can't trust the producer not being maliciously written.
Currently if the producer is malicious, he can either hang the tcp connection permanently to the point that it won't even notice a close() on the other side, or without registering a producer twisted can leak an infinite amount of memory leading either ways to a local DoS. Not even the keepalive seem to wake it up, after all if it could notice it, he would have noticed it before the keepalive timer triggers.
I think the fix is to try by default to make registerProducer not to remove the file descriptor from the poll syscall, but to change it from POLLIN/POLLOUT to pollreactor.POLL_DISCONNECT.
pauseProducing doesn't mean "hang indefinitely", one should still be notified if the other side of the tcp closes the socket. So a new protocol.connectionLostAsync API could be added, to notify that there is still incoming data to read, but the other side has already closed the tcp connection. This way the connectionLostAsync could in turn call transport.loseConnection() or even transport.connectionLost(). The latter is needed to close the socket hard even if there's still data in the send queue, right?
Currently I don't strictly need this, because I've always "trusted" ssl connection, that I can use to call protocol.transport.loseConnection() on the "untrusted" protocol that is hanging in paused state. That is guaranteed to unblock it and make it notice that the other side disconnected, right? Or do I need to call protocol.transport.connectionLost(closeconnection) instead to be sure it unblocks?