[Addressing only points that need attention]
- Also please don't put a space between the open parenthesis and the first argument (you do this almost consistently in test_timeout.py).
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.
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]
I screwed up the write. New the write is:
[...]
which is pretty much the same as the old. The read should be ok though. I could really use someone with a win compiler to test this for me.
I'll review it more when you next upload it.
- It also looks like you've broken the semantics of size<0 in read().
Maybe I'm misunderstanding the code, but I thought that a size < 0 meant to read until there are no more? The statement:
while size < 0 or buf_len < size:
accomplishes the same thing as what's in the current socket.py implementation. If you look closely, the 'if >= 0' branch *always* returns, meaning that the < 0 is equiv to while 1. Due to shortcutting, the same thing happens in the above statement. Maybe a comment would make it clearer?
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.
- 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).
- Since this code appears independent from the timeoutsocket code, maybe we can discuss this separately?
The point of this code was to keep from losing data when an exception occurs (as timothy, if I remember correctly, pointed out). Hence the reason for keeping a lot more data around in instance variables instead of local variables. So the windows version might (in obscure cases) be affected by the timeout changes. That's what this patch was addressing.
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.
- 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.
- 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.
Well, upon socket creation (in init_sockobject), the socket is set to blocking mode. I think that handles fromfd, right? Doesn't every initialization means have to call that function?
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.
The real problem would be someone using an ioctl or setsockopt (Can you even do blocking stuff through setsockopt?).
Yes, setblocking() makes a call to setsockopt(). :-)
Ugh. The original timeoutsocket didn't really deal with anything like that. Erm, seems like an interface problem here - using ioctl kinda breaks the socket object interface. Perhaps we should be doing some sort of getsockopt to figure out the blocking mode and update our state accordingly? That would be an extra call for each thing to the interface though.
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.
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.
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.
Hmm.. Will need to think about this more. Any insight would be helpful or some wisdom about how you usually handle this sort of thing.
See above. Since we don't know what people out there are doing, I don't want to break existing code. We do know that existing code doesn't use (this form of) timeout, so we can exploit that knowledge.
- What is s->errorhandler() for? AFAICT, this is always equal to PySocket_Err!
This was always in the module. Not sure why it was put there intially. I used it to be consistent.
Argh, you're right. MAL added this; I'll ask him why.
If you made it this far, it's time for coffee.
When can I expect a new version? --Guido van Rossum (home page: http://www.python.org/~guido/)