[Python-Dev] Socket timeout patch

[Guido van Rossum]

>    The socket timeout patch is finally available with doc
> updates n' unit test as patch #555085 in the SF tracker. This
> implementation provides timeout functionality at the C
> level. A patch to socket.py also fixes the problem of losing
> data while an exception is thrown to the underlying socket.

Hi Michael,

Sorry for taking such a long time to review this.  (Apparently nobody
else had time either.)  And thanks for doing this!  I think this is a
great start.  Rather than typing them in the SF bug manager issue I am
mailing them (I'll add a link to the archived version of this message
to the SF bug manager to track the activity).


General style nits:

- You submitted a reverse diff!  Customary is diff(old, new).

- Please don't put a space between the function name and the open
  parenthesis.  (You do this both in C and in Python code.)

- Also please don't put a space between the open parenthesis and the
  first argument (you do this almost consistently in test_timeout.py).

- Please don't introduce lines longer than 78 columns.


Feedback on the patch to socket.py:

- I think you're changing the semantics of unbuffered and
  line-buffered reads/writes on Windows.  For one thing, you no longer
  implement line-buffered writes correctly.  The idea is that if the
  buffer size is set to 1, data is flushed at \n only, so that if
  the code builds up the line using many small writes, this doesn't
  result in many small sends. There was code for this in write() --
  why did you delete it?

- It also looks like you've broken the semantics of size<0 in read().

- Maybe changing the write buffer to a list makes sense too?

- Since this code appears independent from the timeoutsocket code,
  maybe we can discuss this separately?


Feedback on the documentation:

- I would document that the argument to settimeout() should be int or
  float (hm, can it be a long?  that should be acceptable even if it's
  strange), and that the return value of gettimeout() is None or a
  float.

- You may want to document the interaction with blocking mode.


Feedback on the C socket module changes:

- Why import the select module?  It's much more efficient to use the
  select system call directly.  You don't need all the overhead and
  generality that the select module adds on top of it, and it costs a
  lot (select allocates lots of objects and lots of memory and hence
  is very expensive).

- <errno.h> is already included by Python.h.

- Please don't introduce more static functions with a 'Py' name
  prefix.

- You should raise TypeError if the type of the argument is wrong, and
  ValueError if the value is wrong (out of range).  Not SocketError.

- I believe that you can't reliably maintain a "sock_blocking" flag;
  there are setsockopt() or ioctl() calls that can make a socket
  blocking or non-blocking.  Also, there's no way to know whether a
  socket passed to fromfd() is in blocking mode or not.

- There are refcount bugs.  I didn't do a detailed review of these,
  but I note that the return value from PyFloat_FromDouble() in
  PySocketSock_settimeout() is leaked.  (There's an INCREF that's only
  needed for the None case.)

- The function internal_select() is *always* used like this:

        count = internal_select (s, 1);
        if (count < 0)
                return NULL;
        else if (count ==  0) /* Timeout elapsed */
                return PyTimeout_Err ();

  If internal_select() called PyTimeout_Err() itself, all call sites
  could be simplified to this:

        count = internal_select (s, 1);
        if (count < 0)
                return NULL;

  or even (now that the count variable is no longer needed) to this:

        if(internal_select (s, 1) < 0)
                return NULL;

- The accept() wrapper contains this bit of code (only showing the
  Unix variant):

	if (newfd < 0)
		if (!s->sock_blocking || (errno != EAGAIN && errno != EWOULDBLOCK))
			return s->errorhandler ();

  Isn't the sense of testing s->sock_blocking wrong?  I would think
  that if we're in blocking mode we'd want to return immediately
  without even checking the errno.  I recommend writing this out more
  clearly, e.g. like this:

    if (s->sock_blocking)
       return s->errorhandler();
    /* A non-blocking accept() failed */
    if (errno != EAGAIN && errno != EWOULDBLOCK)
       return s->errorhandler();

- What is s->errorhandler() for?  AFAICT, this is always equal to
  PySocket_Err!

- The whole interaction between non-blocking mode and timeout mode is
  confusing to me.  Are you sure that this always does the right
  thing?  Have you even thought about what "the right thing" is in all
  4 combinations?

--Guido van Rossum (home page: http://www.python.org/~guido/)