On Fri, Aug 30, 2013 at 6:34 AM, Charles-François Natali
<report@bugs.python.org> wrote:
[Giampaolo Rodola']
> > - I'm not sure why in case of EINTR you retry with a different timeout; can't you just return []?
>
> Because when I do:
> selector.select(10)
>
> I expect the selector to wait 10 seconds if no event occurs: the fact
> that a signal comes in shouldn't break the contract made by the API,
> i.e. that it will return when a FD is ready or the timeout expires.

I disagree. In this case it is almost certain that there is already
some kind of retry loop around the Selector.select() call, so there is
no need for the retry. (Certainly this is how it happens in Tulip.) In
the case of select() the contract is actually quite clear, and there
shouldn't be any problem with returning an empty list early. (This is
quite different from e.g. an interrupted read() or recv() call, where
there is no existing return value that you can return safely, since
their users don't expect None and will usually treat an empty string
as an EOF indicator.)

> Early return can lead to spurious errors: just imagine you send a
> request to a busy server: it would be bad to raise a timeout error
> just because the user put the client in the background with CTRL-Z
> (which results in SIGSTOP).

Yeah, and Tulip's retry handles this just fine.

I also noticed that you are hardcoding a dependency on
time.monotonic() if it exists. If it weren't for your desire to "fix"
this issue you wouldn't have needed a time function at all!

> > - this is probably because I'm paranoid about performances but given that select() method will be called repeatedly I would not use a decorator. Also, similarly to what has been done elsewhere in the stdlib, for "critical" parts I would recommend localizing variable access in order to minimize overhead as in:
> >
> >     def select(self, timeout=None):
> >         ...
> >         key_from_fd = self._key_from_fd
> >         ready_append = ready.append
> >         for fd in r | w:
> >             ...
> >             key = key_from_fd(fd)
> >             if key:
> >                 ready_append((key, events & key.events))
>
> I find that localizing variables leads to unreadable code, and is tied
> to the current CPython interpreter implementation: such optimizations
> belong to the interpreter, not user code.
>
> As for the decorator performance overhead, I don't think it weights
> much compared to the cost of a syscall (+ GIL acquire/release):
> with decorator:
> $ ./python -m timeit -s "from selectors import DefaultSelector,
> EVENT_WRITE; import os; s = DefaultSelector();
> s.register(os.pipe()[1], EVENT_WRITE)" "s.select()"
> 100000 loops, best of 3: 3.69 usec per loop
> without decorator:
> $ ./python -m timeit -s "from selectors import DefaultSelector,
> EVENT_WRITE; import os; s = DefaultSelector();
> s.register(os.pipe()[1], EVENT_WRITE)" "s.select()"
> 100000 loops, best of 3: 3.52 usec per loop
>
> That's a 4% overhead, with a single FD that's always ready (and I
> suspect that most of the overhead is due to the call to time(), not
> the decorator per se).

But still I agree with Giampaolo that the decorator doesn't feel
right. It's like a try/except around the entire body of your function,
which is usually a code smell (even though I realize the exception it
catches can only be raised by the syscall). Please go back to the
explicit try/except returning [].

> Also, I'll shortly propose a patch to handle EINTR within C code, so
> those EINTR wrappers won't  be needed anymore.

Hm, it may be a while before everyone is comfortable with that change.
Who knows what might break?