Hello,

I have a code that uses multiprocessing.Pipe to communicate with subprocesses. Spawning 500 subprocesses this way works like a charm, but when spawning about 600 of them the pipe ends raise the exception: "handle out of range in select()". I realized that this is because of the FD_SETSIZE limit. To address the situation I quickly hacked together a patch that uses poll() instead of select(), which solves the problem for me. I don't know the reason why select() was chosen for this task (maybe because of windows) but wouldn't it be better to use polling where possible?

I've attached the tester. Beware, running it may use up all memory in your system, so be careful!

Gergely Kalman

And this is the patch that I wrote.

It applies to python 3.2.

Hope this helps

Gergely Kalman

I would rate this issue as a performance bug, not a mere feature request. If the python process has more than 1023 open file descriptors, multiprocessing.Pipe.poll() becomes unusable. This is a serious barrier to using multiprocessing in a complex server.

I support this change. Putting an arbitrary limitation on the amount of supported subprocesses is disastrous for complex software.

Gergely's patch seems good. I would only like to suggest a small cosmetic refinement to it, which removes some dead code.

This code has changed a lot in Python 3.3 (it is now located in Lib/multiprocessing/connection.py). Can you post a patch against the development tip ("default" branch)?

See http://docs.python.org/devguide/setup.html if you need more information.

The analogous code within Modules/selectmodule.c uses
  #ifdef HAVE_POLL
to guard the poll-using code, to support non-Windows platforms that don't have "poll".

Presumably a patch for this should do the same.

Also, I see that Modules/selectmodule.c has some painful-looking workarounds involving "HAVE_BROKEN_POLL", which presumably would also be applicable here.

[for reference: issue 11743 covered Antoine's rewrite of the connection class to be pure python, for 3.3 (re msg138310)]

issue 16259 has just been closed as a dup of this one.  Does this mean that this one will be fixed in Python 2.x too?

Apologies, I meant:

issue 16269 has just been closed as a dup of this one.  Does this mean that this one will be fixed in Python 2.x too?

Not necessarily. It just means the other one was a duplicate.

That was my fear; I raise an issue hurting my 2.x servers in
production, and its closed as duplicate instead of not-going-to-fix?

If an issue is a duplicate of another one it gets closed as a duplicate, and that's it basically.
This issue is still open and this is where the matter should be discussed.

A preliminary patch is in attachment.
By default it uses select() but looks for ValueError (raised in case FD_SETSIZE gets hit) and falls back on using poll().

This is the failure I get when running tests on Linux.
It is related to issue 3321 and I'm not sure what to do with it (remove the test maybe?).


======================================================================
FAIL: test_invalid_handles (__main__.TestInvalidHandle)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "Lib/test/test_multiprocessing.py", line 2852, in test_invalid_handles
    self.assertRaises((ValueError, IOError), conn.poll)
AssertionError: (<class 'ValueError'>, <class 'OSError'>) not raised by poll

I don't like this patch since it makes the implementation poorly testable. Moreover, behaviour might change subtly when the fd becomes > 512. I think that instead the code should always use poll() on platforms where it is available.

By the way, I think this is a bug rather than a performance issue.

Using poll() by default is controversial for 2 reasons, I think:

#1 - a certain slowdown is likely to be introduced (I'll measure it)

#2 - current wait() implementation allows to specify a list of file descriptors and/or Connections objects. 
select() can deal with both while poll() does not (it will return a list of integers rather than a list of Connection instances).

I'm not sure how "public" multiprocessing.connection.wait() is considered and how much backward compatibility should matter in this case.


> behaviour might change subtly when the fd becomes > 512

What do you mean?

> A preliminary patch is in attachment.
> By default it uses select() but looks for ValueError (raised in case 
> FD_SETSIZE gets hit) and falls back on using poll().
>
> This is the failure I get when running tests on Linux.
> It is related to issue 3321 and I'm not sure what to do with it (remove > the test maybe?).

I guess the patch could do

    if any(x[1] & POLLNVAL for x in ret):
        raise ValueError('invalid file descriptor')

Also, I don't think there is any need to unregister the fds since you are just removing entries from an internal dict which will be garbage collected.

I don't know if patching 2.7, 3.2, 3.3 to use/fallback on poll() would be allowed as a bug fix.  When, if ever, will the next 2.7 release happen?

> Using poll() by default is controversial for 2 reasons, I think:
> 
> #1 - a certain slowdown is likely to be introduced (I'll measure it)

That sounds like premature optimization. If you are concerned about that
you could add some caching of the poll object.

> #2 - current wait() implementation allows to specify a list of file
> descriptors and/or Connections objects. 
> select() can deal with both while poll() does not (it will return a
> list of integers rather than a list of Connection instances).

Well, can't you just create a mapping of the fds to the objects?

Your patch already has that problem by the way, and it's even worse
since it will trigger in random conditions (when some fd is > 512).

Still not getting what you refer to when you talk about > 512 fds problem.

> Using poll() by default is controversial for 2 reasons, I think:
>
> #1 - a certain slowdown is likely to be introduced (I'll measure it)

With a single fd poll is a bit faster than select:

$ python -m timeit -s 'from select import select' 'select([0],[],[],0)'
100000 loops, best of 3: 2.99 usec per loop

$ python -m timeit -s 'from select import poll, POLLIN' 'p=poll();p.register(0,POLLIN);p.poll(0)'
100000 loops, best of 3: 2.8 usec per loop

The single fd case is the most important one -- see below.

> #2 - current wait() implementation allows to specify a list of file
> descriptors and/or Connections objects. 
> select() can deal with both while poll() does not (it will return a 
> list of integers rather than a list of Connection instances).
>
> I'm not sure how "public" multiprocessing.connection.wait() is 
> considered and how much backward compatibility should matter in this > case.

It was introduced in Python 3.3 and is only really there to allow cross platform Windows/Unix multiplexing.  It is (now) also used internally by Connection.poll() and Queue.get() with a single fd.

In retrospect it would probably have been better to have implemented poll style multiplexing on Windows.

> Still not getting what you refer to when you talk about > 512 fds problem.

By 512 I mean FD_SETSIZE :)

> Still not getting what you refer to when you talk about > 512 fds
> problem.

Whether you get back the original objects or only their fds will depend on whether some fd was larger than FD_SETSIZE.

See also http://bugs.python.org/issue14635.
This problem affects any single use of select(): instead of using an ad-hoc wrapper in each module, it would probably make sense to add a higher level selector class to the select module which would fallback on the right syscall (i.e. poll() if available, or /dev/poll on Solaris-like).

> This problem affects any single use of select(): instead of using an 
> ad-hoc wrapper in each module, it would probably make sense to add a 
> higher level selector class to the select module which would fallback on 
> the right syscall (i.e. poll() if available, or /dev/poll on Solaris-
> like).

Doesn't Solaris have poll()?  If so then I don't see why one would want to use /dev/poll in the single fd case.

>> This problem affects any single use of select(): instead of using an
>> ad-hoc wrapper in each module, it would probably make sense to add a
>> higher level selector class to the select module which would fallback on
>> the right syscall (i.e. poll() if available, or /dev/poll on Solaris-
>> like).
>
> Doesn't Solaris have poll()?  If so then I don't see why one would want to use /dev/poll in the single fd case.

Because it offers better performance than poll(): you don't have to
keep passing the FD at each syscall (note that I'm not talking about
the signal FD case, but about a generic polling API).

Also note that microbenchmarks with one FD isn't really meaningful,
since in real life the FD won't be ready at least part of the time:
like Antoine, I think that worrying about performance impact is really
a premature optimization (unless real benchmarks prove otherwise).

New patch in attachment.
It always uses poll() and maintains and internal fd/Connection map.
I get one failure due to the returned list being sorted differently than when using select() though.


======================================================================
FAIL: test_wait_integer (__main__.TestWait)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "Lib/test/test_multiprocessing.py", line 3277, in test_wait_integer
    self.assertEqual(res, [p.sentinel, b])
AssertionError: Lists differ: [<multiprocessing.connection.C... != [7, <multiprocessing.connectio...

First differing element 0:
<multiprocessing.connection.Connection object at 0x7f8924fccd30>
7

- [<multiprocessing.connection.Connection object at 0x7f8924fccd30>, 7]
?                                                                  ---

+ [7, <multiprocessing.connection.Connection object at 0x7f8924fccd30>]
?  +++




I don't how important this is.
If it's not tests can be adapted accordingly.

The order of the results is probably unimportant.

Updated patch including test fixes is in attachment.

The patch looks good, however there's something really bothering me:
in issue #14635, the same type of patch was applied to telnetlib,
here, it's multiprocessing and AFAICT, any single use of select() in
the standard library is subject to this FD_SETSIZE limitation.
That why I think it could probably be a good idea to expose a
high-level "selector" object in the select module, which would use the
"right" syscall transparently (e.g. select, poll or /dev/poll), with a
unified API. This would make writing portable and efficient I/O
multiplexing code much easier, not only in the stdlib, but also for
end-users.

I know. I proposed something like that here: http://mail.python.org/pipermail/python-ideas/2012-May/015223.html.
In theory all the necessary pieces are already there. What's missing is an agreement on what the API should look like, and that's the hard part 'cause it should be the most generic as possible.

> I know. I proposed something like that here:
> http://mail.python.org/pipermail/python-ideas/2012-May/015223.html.
> In theory all the necessary pieces are already there. What's missing
> is an agreement on what the API should look like, and that's the hard
> part 'cause it should be the most generic as possible.

Well, there was a lot of bikeshedding and pie-in-the-sky arguments in
that thread, but I think the original idea of a small wrapper is good
enough. Let Guido do the grand async shakeup separately.

Also, I've changed my mind: I think select would be an ok module for
this :)

Well, for now I'd say let's just check in this patch as-is.
I would be keen on considering this a bug and hence address the patch for Python 2.7, 3.2, 3.3 and 3.4.

New changeset 5530251d9cac by Giampaolo Rodola' in branch '3.2':
Fix issue 10527: make multiprocessing use poll() instead of select() if available.
http://hg.python.org/cpython/rev/5530251d9cac

New changeset d89891f3f769 by Giampaolo Rodola' in branch '3.3':
Fix issue 10527: make multiprocessing use poll() instead of select() if available.
http://hg.python.org/cpython/rev/d89891f3f769

New changeset e971a70984b8 by Giampaolo Rodola' in branch 'default':
Fix issue 10527: make multiprocessing use poll() instead of select() if available.
http://hg.python.org/cpython/rev/e971a70984b8

New changeset c5c27b84d7af by Giampaolo Rodola' in branch '2.7':
Fix issue 10527: make multiprocessing use poll() instead of select() if available.
http://hg.python.org/cpython/rev/c5c27b84d7af

> New changeset c5c27b84d7af by Giampaolo Rodola' in branch '2.7':
> Fix issue 10527: make multiprocessing use poll() instead of select() if available.
> http://hg.python.org/cpython/rev/c5c27b84d7af

This changeset broke many buildbots, at least:
http://buildbot.python.org/all/builders/x86%20XP-5%202.7/builds/439/steps/test/logs/stdio

  File "D:\Buildslave\2.7.moore-windows\build\lib\multiprocessing\connection.py", line 203, in <module>
    if hasattr(select, 'poll'):
NameError: name 'select' is not defined

(I reopen the issue)

New changeset 7cf4ea64f603 by Giampaolo Rodola' in branch '2.7':
issue 10527: fix missing import
http://hg.python.org/cpython/rev/7cf4ea64f603

New changeset d565d862545c by Giampaolo Rodola' in branch '3.2':
issue 10527: fix missing import
http://hg.python.org/cpython/rev/d565d862545c

My bad, sorry. It should be fixed now.

The commits did not have the intended effect.

They just define a _poll() function (and only on Windows) and it is not referenced anywhere else.

I will look in to fixing this -- on 2.7 and 3.2 this will need to be done in the C code.

What do you mean? The intent was to use poll() instead of select() anywhere available in order to avoid running out of fds.
The change didn't affect Windows because as of right now select() is the only thing available.

> What do you mean? The intent was to use poll() instead of select() 
> anywhere available in order to avoid running out of fds.
> The change didn't affect Windows because as of right now select() is 
> the only thing available.

The change *only* effects Windows.  Currently the code goes

    if sys.platform != 'win32':
        ...
    else:
        if hasattr(select, 'poll'):
            def _poll(fds, timeout):
                ...
        else:
            def _poll(fds, timeout):
                ...

So _poll() is only defined when sys.platform == 'win32'.

Furthermore, the _poll() function is never used anywhere: ConnectionBase.poll() uses Connection._poll(), which uses wait(), which uses select().

It looks like the change to multiprocessing/connection.py committed does not match the one uploaded as issue10527-3.patch

changeset 81174:e971a70984b8
     1.1 --- a/Lib/multiprocessing/connection.py
     1.2 +++ b/Lib/multiprocessing/connection.py
     1.3 @@ -509,6 +509,27 @@ if sys.platform != 'win32':
     1.4          return c1, c2
     1.5  
     1.6  else:
     1.7 +    if hasattr(select, 'poll'):
     1.8 +        def _poll(fds, timeout):
     1.9 +            if timeout is not None:
    1.10 +                timeout = int(timeout) * 1000  # timeout is in milliseconds
    1.11 +            fd_map = {}
    1.12 +            pollster = select.poll()
    1.13 +            for fd in fds:
    1.14 +                pollster.register(fd, select.POLLIN)
    1.15 +                if hasattr(fd, 'fileno'):
    1.16 +                    fd_map[fd.fileno()] = fd
    1.17 +                else:
    1.18 +                    fd_map[fd] = fd
    1.19 +            ls = []
    1.20 +            for fd, event in pollster.poll(timeout):
    1.21 +                if event & select.POLLNVAL:
    1.22 +                    raise ValueError('invalid file descriptor %i' % fd)
    1.23 +                ls.append(fd_map[fd])
    1.24 +            return ls
    1.25 +    else:
    1.26 +        def _poll(fds, timeout):
    1.27 +            return select.select(fds, [], [], timeout)[0]
    1.28  
    1.29      def Pipe(duplex=True):
    1.30          '''

issue10527-3.patch:
diff --git a/Lib/multiprocessing/connection.py b/Lib/multiprocessing/connection.py
--- a/Lib/multiprocessing/connection.py
+++ b/Lib/multiprocessing/connection.py
@@ -861,6 +861,27 @@
         return [o for o in object_list if o in ready_objects]
 
 else:
+    if hasattr(select, 'poll'):
+        def _poll(fds, timeout):
+            if timeout is not None:
+                timeout = int(timeout) * 1000  # timeout is in milliseconds
+            fd_map = {}
+            pollster = select.poll()
+            for fd in fds:
+                pollster.register(fd, select.POLLIN)
+                if hasattr(fd, 'fileno'):
+                    fd_map[fd.fileno()] = fd
+                else:
+                    fd_map[fd] = fd
+            ls = []
+            for fd, event in pollster.poll(timeout):
+                if event & select.POLLNVAL:
+                    raise ValueError('invalid file descriptor %i' % fd)
+                ls.append(fd_map[fd])
+            return ls
+    else:
+        def _poll(fds, timeout):
+            return select.select(fds, [], [], timeout)[0]
 
     def wait(object_list, timeout=None):
         '''
@@ -870,12 +891,12 @@
         '''
         if timeout is not None:
             if timeout <= 0:
-                return select.select(object_list, [], [], 0)[0]
+                return _poll(object_list, 0)
             else:
                 deadline = time.time() + timeout
         while True:
             try:
-                return select.select(object_list, [], [], timeout)[0]
+                return _poll(object_list, timeout)
             except OSError as e:
                 if e.errno != errno.EINTR:
                     raise

Damn, you're right. I must have messed up something while porting the patch from 3.2 all the way up to 3.4 as the merge produced some conflicts.

New changeset 831f49cc00fc by Giampaolo Rodola' in branch 'default':
fix for previous commit related to issue 10527 which didn't have the intended effect as per http://bugs.python.org/issue10527#msg179895
http://hg.python.org/cpython/rev/831f49cc00fc

3.3 and 3.4 branches should now be fixed.
2.7 and 3.2 still need to be fixed and the code from connections.py removed.
Sorry for the mess up.

New changeset da5e520a7ba5 by Richard Oudkerk in branch '2.7':
Issue #10527: Use poll() instead of select() for multiprocessing pipes
http://hg.python.org/cpython/rev/da5e520a7ba5

New changeset abf111b9a464 by Richard Oudkerk in branch '3.2':
Issue #10527: Use poll() instead of select() for multiprocessing pipes
http://hg.python.org/cpython/rev/abf111b9a464

New changeset f07435fa6736 by Richard Oudkerk in branch '2.7':
Issue #10527: Remove dead code
http://hg.python.org/cpython/rev/f07435fa6736

New changeset 49d45151b9ed by Richard Oudkerk in branch '3.2':
Issue #10527: Remove dead code
http://hg.python.org/cpython/rev/49d45151b9ed