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
RetroSearch is an open source project built by @garambo | Open a GitHub Issue
Search and Browse the WWW like it's 1997 | Search results from DuckDuckGo
HTML:
3.2
| Encoding:
UTF-8
| Version:
0.7.4