[Python-Dev] Socket timeout patch

[Michael Gilfix]

On Mon, Jun 03 @ 13:22, Guido van Rossum wrote:
> >   Couldn't really figure out what you were seeing here. I read that
> > you saw something like func( a, b), which I don't see in my local
> > copy.
> 
> test_timeout.py from the SF page has this.  I'm glad you fixed this
> already in your own copy.

  Weird. I didn't change anything. Oh well. We'll see if it shows up in
the new patch this time round.

> > I do have something like this for list comprehension:
> > 
> >     [ x.find('\n') for x in self._rbuf ]
> > 
> >   Er, but I though there were supposed to be surrounding spaces at the
> > edges...
> 
> I prefer to see that as
> 
>     [x.find('\n') for x in self._rbuf]

  Ok. Done. One day, you can explain to me why you despise whitespace
so. Perhaps she was mean to you or something. She's always hanging around
with that tab guy at any rate and they make a bad mix.

> > > - It also looks like you've broken the semantics of size<0 in read().
>
> I was referring to this piece of code:
> 
> !         if buf_len > size:
> !             self._rbuf.append (data[size:])
> !             data = data[:size]
> 
> Here data[size:] gives you the last byte of the data and data[:size]
> chops off the last byte.

  Ok. This has been fixed. All read sizes now work and have been tested
by me.

> > > - Maybe changing the write buffer to a list makes sense too?
> > 
> >   I could do this. Then just do a join before the flush. Is the append
> > /that/ much faster?
> 
> Depends on how small the chunks are you write.  Roughly, repeated list
> append is O(N log N), while repeated string append is O(N**2).

  Done. The write buffer now uses a list, so it should be faster than
the initial version and the one currently in use.

> OK, but given the issues the first version had, I recommand that the
> code gets more review and that you write unit tests for all cases.

  I agree. I wasn't through enough in my checking. I'm going to see if
I can include a test-case specifically to test the windows file
class directly.

> > > - Please don't introduce more static functions with a 'Py' name
> > >   prefix.
> > 
> >   Only did this in one place, with PyTimeout_Err. The reason was that the
> > other Error functions used the Py prefix, so it was done for consistency. I
> > can change that.. or change the naming scheme with the others if you like.
> 
> I like to do code cleanup that doesn't change semantics (like
> renamings) as a separate patch and checkin.  You can do this before or
> after the timeout changes, but don't merge it into the timeout
> changes.  I still like the static names that you introduce not to
> start with Py.

  Ok. I'll change the PyTimeout_Err to just timeout_err. We can do some
other cleanup after the patch has been accepted. It's big enough as is and
no need to add more complication.

> OK, it looks like you call internal_setblocking(s, 0) to set the
> socket in nonblocking mode.  (Hm, I don't see any calls to set the
> socket in blocking mode!)
> 
> So do I understand that you are now always setting the socket in
> non-blocking mode, even when there is no timeout specified, and that
> you look at the sock_blocking flag to decide whether to do timeouts or
> just pass the nonblocking behavior to the user?
> 
> This is a change in semantics, and could interfere with existing
> applications that pass the socket's file descriptor off to other
> code.  I think I'd be happier if the behavior wasn't changed at all
> until a timeout is set for a socket -- then existing code won't
> break.

  So, the best way to proceed seems to be:

    if (s->sock_timeout == Py_None)
       /* Perhaps do nothing, or just do original behavior */
    else
       /* Get funky. Do one of the solutions discussed below */

> I only really care for sockets passed in to fromfd().  E.g. someone
> can currently do:
> 
>   s1 = socket(AF_INET, SOCK_STREAM)
>   s1.setblocking(0)
> 
>   s2 = fromfd(s1.fileno())
>   # Now s2 is non-blocking too
> 
> I'd like this to continue to work as long as s1 doesn't set a timeout.

  I see the issue. We'll worry about this and not ioctl. So let's look
at solutions:

> >   One solution is to set/unset blocking mode right before doing each
> > call to be sure of the state and based on the internally stored value
> > of the blocking attribute... but... then that kind of renders ioctl
> > useless.
> 
> Don't worry so much about ioctl, but do worry about fromfd.

  Not so popular.

> >   Another solution might be to set the blocking mode to on everytime
> > someone sets a timeout. That would change the blocking/socket
> > interaction already described a bit but not drastically. Also easy
> > to implement.  That sends the message: Don't use ioctls when using
> > timeouts.
> 
> I like this.

  Alright. Well, using the above pseudo-code scheme, we should be alright.
So here are the new semantics:

  If you set_timeout(int/float/long != None):
    The actual socket gets put in non-blocking mode and the usual select
    stuff is done.
  If you set_timeout(None):
    The old behavior is used AND automatically, the socket is set
    to blocking mode. That means that someone who was doing non-blocking
    stuff before, sets a timeout, and then unsets one, will have to do
    a set_blocking call again if he wants non-blocking stuff. This makes
    sense 'cause timeout stuff is blocking by nature.

  That seems fairest and we always have an idea of what state we're in.

                    -- Mike

-- 
Michael Gilfix
mgilfix@eecs.tufts.edu

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