Issue 131370043: BaseSelectorEventLoop.sock_connect() unregisters the socket on timeout
|
Created:
9 years, 8 months ago by haypo_gmail Modified:
9 years, 8 months ago Reviewers:
GvR Visibility:
Public. |
DescriptionBaseSelectorEventLoop.sock_connect() unregisters the socket on timeout
Patch Set 1 #
Total comments: 6
Patch Set 2 : Split _sock_connect() #
Total comments: 11
Patch Set 3 : Simplify _sock_connect_cb() #
MessagesTotal messages: 11
https://codereview.appspot.com/131370043/diff/1/asyncio/selector_events.py File asyncio/selector_events.py (left): https://codereview.appspot.com/131370043/diff/1/asyncio/selector_events.py#ol... asyncio/selector_events.py:356: return Wouldn't it be much simpler to just unregister the callback here (if registered) and not make any other changes? https://codereview.appspot.com/131370043/diff/1/asyncio/selector_events.py File asyncio/selector_events.py (right): https://codereview.appspot.com/131370043/diff/1/asyncio/selector_events.py#ne... asyncio/selector_events.py:11: import functools I'd rather use lambda. :-)
Sign in to reply to this message.
https://codereview.appspot.com/131370043/diff/1/asyncio/selector_events.py File asyncio/selector_events.py (left): https://codereview.appspot.com/131370043/diff/1/asyncio/selector_events.py#ol... asyncio/selector_events.py:356: return On 2014/08/27 14:18:23, GvR wrote: > Wouldn't it be much simpler to just unregister the callback here (if registered) > and not make any other changes? You cannot use _sock_connect() to remove the write because _sock_connect() is not called back on timeout. The race condition: - sock_connect() -> add a writer and returns a future - on timeout, sock_connect() and _sock_connect() are not called back and the writer remains - in the case of create_connection(), the socket is closed (and the writer is not removed) Or maybe I misunderstood something? https://codereview.appspot.com/131370043/diff/1/asyncio/selector_events.py File asyncio/selector_events.py (right): https://codereview.appspot.com/131370043/diff/1/asyncio/selector_events.py#ne... asyncio/selector_events.py:11: import functools On 2014/08/27 14:18:23, GvR wrote: > I'd rather use lambda. :-) functools.partial() has a better representation than lambda: not only it shows the function but also parameters: $ python3 >>> import functools >>> import asyncio >>> cb = functools.partial(print, "hello", "world") >>> cb2 = lambda: print("hello", "world") >>> cb functools.partial(<built-in function print>, 'hello', 'world') >>> cb2 <function <lambda> at 0x7f6011821320> My asyncio.events._format_callback() understands functools.partial() to return a compact representation of a callback.
Sign in to reply to this message.
OK. But I wonder if it wouldn't be cleaner to split _sock_connect() completely -- a first version without callback (and hence registered = False) and a second version that's a callback. Currently the logic is getting really complex and hard to follow -- everything I hate about async code in the first place... :-( Good catch on the race though! https://codereview.appspot.com/131370043/diff/1/asyncio/selector_events.py File asyncio/selector_events.py (left): https://codereview.appspot.com/131370043/diff/1/asyncio/selector_events.py#ol... asyncio/selector_events.py:356: return On 2014/08/27 15:22:57, haypo_gmail wrote: > On 2014/08/27 14:18:23, GvR wrote: > > Wouldn't it be much simpler to just unregister the callback here (if > registered) > > and not make any other changes? > > You cannot use _sock_connect() to remove the write because _sock_connect() is > not called back on timeout. > > The race condition: > > - sock_connect() -> add a writer and returns a future > - on timeout, sock_connect() and _sock_connect() are not called back and the > writer remains > - in the case of create_connection(), the socket is closed (and the writer is > not removed) > > Or maybe I misunderstood something? Acknowledged. https://codereview.appspot.com/131370043/diff/1/asyncio/selector_events.py File asyncio/selector_events.py (right): https://codereview.appspot.com/131370043/diff/1/asyncio/selector_events.py#ne... asyncio/selector_events.py:11: import functools On 2014/08/27 15:22:57, haypo_gmail wrote: > On 2014/08/27 14:18:23, GvR wrote: > > I'd rather use lambda. :-) > > functools.partial() has a better representation than lambda: not only it shows > the function but also parameters: > > $ python3 > >>> import functools > >>> import asyncio > >>> cb = functools.partial(print, "hello", "world") > >>> cb2 = lambda: print("hello", "world") > >>> cb > functools.partial(<built-in function print>, 'hello', 'world') > >>> cb2 > <function <lambda> at 0x7f6011821320> > > My asyncio.events._format_callback() understands functools.partial() to return a > compact representation of a callback. Acknowledged.
Sign in to reply to this message.
On 2014/08/27 16:40:25, GvR wrote: > OK. But I wonder if it wouldn't be cleaner to split _sock_connect() completely > -- a first version without callback (and hence registered = False) and a second > version that's a callback. Currently the logic is getting really complex and > hard to follow -- everything I hate about async code in the first place... :-( Ok, here is a new version. It is a little bit simpler > Good catch on the race though! Thanks.
Sign in to reply to this message.
https://codereview.appspot.com/131370043/diff/20001/asyncio/selector_events.py File asyncio/selector_events.py (right): https://codereview.appspot.com/131370043/diff/20001/asyncio/selector_events.p... asyncio/selector_events.py:385: except (BlockingIOError, InterruptedError): I don't think this can be reached! Unless the OSError constructor returns a subclass instance based on errno???
Sign in to reply to this message.
Sorry, the OSError constructor does that. But I still think we can make it simpler! https://codereview.appspot.com/131370043/diff/20001/asyncio/selector_events.py File asyncio/selector_events.py (right): https://codereview.appspot.com/131370043/diff/20001/asyncio/selector_events.p... asyncio/selector_events.py:357: self._sock_connect(fut, sock, address) This is direct recursion. It is also different from the original. Is that what you meant? https://codereview.appspot.com/131370043/diff/20001/asyncio/selector_events.p... asyncio/selector_events.py:376: self.remove_writer(fd) I think you can drop these two remove_* calls, and then you won't need the two add_* calls in the first except clause below -- the callbacks will just remain, since they are always already set when we enter here, and the future's done callback will reliably clean things up. Or what am I missing? https://codereview.appspot.com/131370043/diff/20001/asyncio/selector_events.p... asyncio/selector_events.py:377: if fut.cancelled(): You could move this up to the top of the function -- when the future is cancelled its done_callback will be called. And the done_callback (i.e. _soc_connect_fut_done) will remove the writer. https://codereview.appspot.com/131370043/diff/20001/asyncio/selector_events.p... asyncio/selector_events.py:383: # Jump to the except clause below. the -> any
Sign in to reply to this message.
https://codereview.appspot.com/131370043/diff/20001/asyncio/selector_events.py File asyncio/selector_events.py (right): https://codereview.appspot.com/131370043/diff/20001/asyncio/selector_events.p... asyncio/selector_events.py:385: except (BlockingIOError, InterruptedError): On 2014/08/27 21:08:59, GvR wrote: > I don't think this can be reached! Hum, good question. getsockopt() is a system call. Why getsockopt() would not be interrupted by a signal? The 4th result for my Google search "getsockopt eintr" is an issue in a Python project: https://github.com/dotcloud/zerorpc-python/issues/74 This bug report shows a getsockopt() failing with EINTR: "Unfortunately, these show up in our production systems and not in tests, so I don't have anything else but Tracebacks". I don't think that it hurts to handle InterruptedError here.
Sign in to reply to this message.
Our emails crossed, I recanted but then added a bunch of other ideas/questions. https://codereview.appspot.com/131370043/diff/20001/asyncio/selector_events.py File asyncio/selector_events.py (right): https://codereview.appspot.com/131370043/diff/20001/asyncio/selector_events.p... asyncio/selector_events.py:385: except (BlockingIOError, InterruptedError): On 2014/08/27 21:49:05, haypo_gmail wrote: [...] > I don't think that it hurts to handle InterruptedError here. Yeah, plus I was mistaken about the OSError constructor (in Python 3, so beware in Trollius).
Sign in to reply to this message.
https://codereview.appspot.com/131370043/diff/20001/asyncio/selector_events.py File asyncio/selector_events.py (right): https://codereview.appspot.com/131370043/diff/20001/asyncio/selector_events.p... asyncio/selector_events.py:357: self._sock_connect(fut, sock, address) On 2014/08/27 21:30:48, GvR wrote: > This is direct recursion. It is also different from the original. Is that what > you meant? Yes. But I don't know exactly how much signals can be received. Using subprocesses, it can be a lot. More than the maximum call depth. So I replaced the recursion with a simple loop. I'm working on a more generic fix for EINTR: http://legacy.python.org/dev/peps/pep-0475/ https://codereview.appspot.com/131370043/diff/20001/asyncio/selector_events.p... asyncio/selector_events.py:376: self.remove_writer(fd) On 2014/08/27 21:30:48, GvR wrote: > I think you can drop these two remove_* calls, and then you won't need the two > add_* calls in the first except clause below -- the callbacks will just remain, > since they are always already set when we enter here, and the future's done > callback will reliably clean things up. Or what am I missing? Right. I removed remove_writer/add_writer, it simplifies the code a lot. https://codereview.appspot.com/131370043/diff/20001/asyncio/selector_events.p... asyncio/selector_events.py:377: if fut.cancelled(): On 2014/08/27 21:30:48, GvR wrote: > You could move this up to the top of the function -- when the future is > cancelled its done_callback will be called. And the done_callback (i.e. > _soc_connect_fut_done) will remove the writer. Acknowledged. https://codereview.appspot.com/131370043/diff/20001/asyncio/selector_events.p... asyncio/selector_events.py:383: # Jump to the except clause below. On 2014/08/27 21:30:48, GvR wrote: > the -> any Done.
Sign in to reply to this message.
LGTM I take it that the original code was broken if the initial connect() call got an EINTR?
Sign in to reply to this message.
On 2014/08/30 00:07:55, GvR wrote: > LGTM Thanks for the feedback, I agree that the final version is now simpler. > I take it that the original code was broken if the initial connect() call got an > EINTR? Seriously, I don't know. It's really hard to get information on the exact behaviour of syscalls on EINTR. If you have some free time, you may read this long article: http://www.madore.org/~david/computers/connect-intr.html But the article looks to be oriented to blocking socket (according to the final example). I don't know how to test the behaviour of connect() on EINTR. Maybe with a tricky debugger and manual operations.
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
