Code review comment for lp:~jameinel/bzr/2.5-unending-sendall-1047309

Revision history for this message
Richard Wilbur (richard-wilbur) wrote :

The documentation for paramiko.Channel.send: "Returns the number of bytes sent, or 0 if the channel stream is closed."[1]

Python 2.7 socket library documentation for socket.send: "Returns the number of bytes sent. Applications are responsible for checking that all data has been sent; if only some of the data was transmitted, the application needs to attempt delivery of the remaining data. For further information on this concept, consult the Socket Programming HOWTO."[2]

Interestingly the Socket Programming HOWTO (referenced above) has an example which suggests you might get a 0 return value[3], but only if the connection is broken:
            sent = self.sock.send(msg[totalsent:])
            if sent == 0:
                raise RuntimeError("socket connection broken")

Linux documentation for socket send[4]:
RETURN VALUE
       On success, these calls return the number of characters sent. On
       error, -1 is returned, and errno is set appropriately.

The consensus seems to be that a socket send should never return 0 unless there is an error. Paramiko looks like it thinks that is a normal way to signal a closed connection. The python socket programming HOWTO implies that the only reason for a 0 return value is also a closed connection.

@John, I agree with your proposition that we should throw an exception immediately when send returns 0.
@Martin, this would alleviate the need for _max_no_content_sends. Hence,
  1. I am in favour of removing _max_no_content_sends
  2. Comment/docstring for osutils.sendall would be fine if it isn't obvious from the exception we throw
  3.,4. Agree

References:
[1] http://www.lag.net/paramiko/docs/paramiko.Channel-class.html#send
[2] http://docs.python.org/library/socket.html
[3] http://docs.python.org/howto/sockets.html#socket-howto
[4] info send

review: Needs Fixing

« Back to merge proposal