Re: [Python-Dev] Socket timeout patch

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). - 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/)