[Python-Dev] Socket timeout patch

[Michael Gilfix]

  Whew. Lots of stuff. Give me a while to address all these issues,
rework the patch, and then submit a comprehensive answer. Should
have an answer by the end of the week latest.

                 -- Mike

On Thu, May 23 @ 16:13, Guido van Rossum wrote:
> 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/)
`-> (guido)

-- 
Michael Gilfix
mgilfix@eecs.tufts.edu

For my gpg public key:
http://www.eecs.tufts.edu/~mgilfix/contact.html