classification
Title: multiprocessing: use selectors module
Type: enhancement Stage: resolved
Components: Library (Lib) Versions: Python 3.4
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: giampaolo.rodola, neologix, python-dev, sbt
Priority: normal Keywords: needs review, patch

Created on 2013-09-05 16:46 by neologix, last changed 2013-09-09 16:49 by neologix. This issue is now closed.

Files
File name Uploaded Description Edit
selectors_multiprocessing.diff neologix, 2013-09-05 16:46 review
connection_selector.diff neologix, 2013-09-06 19:01 review
Messages (9)
msg197012 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2013-09-05 16:46
Here's a patch.
msg197014 - (view) Author: Richard Oudkerk (sbt) * (Python committer) Date: 2013-09-05 17:43
LGTM.

But I would move "import selectors" in multiprocessing.connection to just before the definition of wait() for Unix.  It is not needed on Windows and unnecessary imports slow down start up of new processes.
msg197017 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2013-09-05 18:47
New changeset 81f0c6358a5f by Charles-François Natali in branch 'default':
Issue #18934: multiprocessing: use selectors module.
http://hg.python.org/cpython/rev/81f0c6358a5f
msg197026 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2013-09-05 21:15
The test is failing on some (unstable) buildbots:
http://buildbot.python.org/all/builders/AMD64%20Solaris%2011%20%5BSB%5D%203.x/builds/1598/steps/test/logs/stdio
"""
======================================================================
FAIL: test_invalid_handles (test.test_multiprocessing_fork.TestInvalidHandle)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/cpython/buildslave/cc-32/3.x.snakebite-solaris11-amd64/build/Lib/test/_test_multiprocessing.py",
line 2962, in test_invalid_handles
    self.assertRaises((ValueError, OSError), conn.poll)
AssertionError: (<class 'ValueError'>, <class 'OSError'>) not raised by poll
"""

Basically, this test checks that calling poll() on an invalid
Connection (invalid FD) raises an OSError.
That's true for select, kqueue and epoll, but not for poll(), which
returns POLLNVAL.
I'm not sure about what to do here.
msg197033 - (view) Author: Richard Oudkerk (sbt) * (Python committer) Date: 2013-09-05 22:08
I remember wondering at one time why EPOLLNVAL did not exist, and realizing that closed fds are just silently unregistered by epoll().

I guess the issue is that some of the selectors indicate a bad fd on registration, and others do it when polled.

                On registration      On poll
----------------------------------------------------------------
SelectSelector  No                   Raises OSError
PollSelector    No                   No (EVENT_READ or EVENT_WRITE)
EpollSelector   Raises OSError       No
KqueueSelector  ?                    ?

It would be easiest to relax the test, perhaps by just checking that conn.poll(0) raises or returns True.

Or maybe PollSelector.select() should raise OSError if POLLNVAL is indicated.  That would match the behaviour of SelectSelector.select().
msg197053 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2013-09-06 06:57
> Richard Oudkerk added the comment:
>
> I remember wondering at one time why EPOLLNVAL did not exist, and realizing that closed fds are just silently unregistered by epoll().

Exactly.

> I guess the issue is that some of the selectors indicate a bad fd on registration, and others do it when polled.
>
>                 On registration      On poll
> ----------------------------------------------------------------
> SelectSelector  No                   Raises OSError
> PollSelector    No                   No (EVENT_READ or EVENT_WRITE)
> EpollSelector   Raises OSError       No
> KqueueSelector  ?                    ?

Kqueue raises OSError upon registration. Not sure about poll().

> It would be easiest to relax the test, perhaps by just checking that conn.poll(0) raises or returns True.

That's what I think too. Apparently, the test was added for this
issue: http://bugs.python.org/issue3321

Basically, the goal was to check that conn.poll() wouldn't crash if
passed an invalid FD (which can happen if you register a FD greater
than FD_SETSIZE in a fd_set).
So relaxing the check would still make sense.

> Or maybe PollSelector.select() should raise OSError if POLLNVAL is indicated.  That would match the behaviour of SelectSelector.select().

Yes, that would be a possibility. I'll have to give it some more thought.
msg197092 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2013-09-06 19:01
I just realized that using DefaultSelector isn't optimal for Connection:
It's fine for forkserver, since it's a long lived process, but for
Connection.poll(), this means that it'll use epoll or kqueue: which
means allocating a new epoll/kqueue object for each conn.poll().
That's a couple syscalls more (epoll_create()/epoll_ctl()/close()),
but most important it uses an extra FD per connection.

The attached patch uses PollSelector if available, otherwise SelectSelector.
msg197097 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2013-09-06 19:17
New changeset 14972c999e80 by Charles-François Natali in branch 'default':
Issue #18934: Relax test_multiprocessing.test_invalid_handles a bit: we just
http://hg.python.org/cpython/rev/14972c999e80
msg197233 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2013-09-08 09:35
New changeset 0e52b9f77dbf by Charles-François Natali in branch 'default':
Issue #18934: Use poll/select-based selectors for multiprocessing.Connection,
http://hg.python.org/cpython/rev/0e52b9f77dbf
History
Date User Action Args
2013-09-09 16:49:48neologixsetstatus: open -> closed
resolution: fixed
stage: patch review -> resolved
2013-09-08 09:35:10python-devsetmessages: + msg197233
2013-09-06 19:17:29python-devsetmessages: + msg197097
2013-09-06 19:01:01neologixsetfiles: + connection_selector.diff

messages: + msg197092
2013-09-06 06:57:40neologixsetmessages: + msg197053
2013-09-05 22:08:59sbtsetmessages: + msg197033
2013-09-05 21:15:05neologixsetmessages: + msg197026
2013-09-05 18:47:11python-devsetnosy: + python-dev
messages: + msg197017
2013-09-05 18:12:25giampaolo.rodolasetnosy: + giampaolo.rodola
2013-09-05 17:43:50sbtsetmessages: + msg197014
2013-09-05 16:46:25neologixcreate