Fix #2069: Fix race condition between accept() handler and TCPServer.stop()#2075
Fix #2069: Fix race condition between accept() handler and TCPServer.stop()#2075bdarnell merged 3 commits intotornadoweb:masterfrom
Conversation
…TCPServer.stop()
bdarnell
left a comment
There was a problem hiding this comment.
The IOLoop isn't supposed to call handlers that have been removed, so you shouldn't be reaching this point. The default IOLoop removes any pending callbacks from self._events, and while I don't think AsyncIOLoop handles this as gracefully it doesn't look like it could call into a removed callback.
That means that the sequence of events is not "queue accept handler, stop server, call accept handler", it's "call accept handler, which synchronously stops the server while add_accept_handler's callback is still on the stack". (this happens sporadically depending on whether the packets arrive fast enough for all the recv calls to be satisfied on the fast synchronous path)
This could have a simpler solution: have add_accept_handler schedule the callbacks with IOLoop.current().add_callback instead of running them directly. This would ensure that you can't close the listening socket while the accept handler is running. However, it would be a slight behavior change in multi-process mode that could result in worse load balancing (since one process will be able to pull multiple connections off the backlog faster. This is less of a concern with SO_REUSEPORT).
tornado/netutil.py
Outdated
| A callable is returned (``None`` was returned before). | ||
| """ | ||
| io_loop = IOLoop.current() | ||
| removed = [] |
There was a problem hiding this comment.
My preferred idiom for this is removed = [False] (and then use if removed[0] and removed[0] = True.
|
I'm not sure why the Python 2.7 builds seem to have get stuck on Travis-CI. Perhaps a Heisenbug? |
|
It doesn't look like a heisenbug - both 2.7 and 2.7.8 runs stopped at the same point in the Any response to my comments above? I can't decide whether it's better to change the interface to this function by adding the returned callable, or keep the interface the same but potentially change the performance. |
|
The interface change doesn't break compatibility, as people can still use the old idiom of calling The suggestion of using |
No description provided.