Opened 8 years ago

Closed 3 years ago

#2234 enhancement closed fixed (fixed)

use poll reactor on POSIX platform that provide poll(2)

Reported by: exarkun Owned by: itamar
Priority: normal Milestone:
Component: core Keywords:
Cc: zooko, zooko@…, itamar Branch: branches/poll-default-2234-3
(diff, github, buildbot, log)
Author: itamarst, exarkun Launchpad Bug:

Description

Currently the default is always select. We can do better than this.

Change History (45)

comment:1 Changed 8 years ago by glyph

  • Priority changed from normal to low

comment:2 Changed 6 years ago by zooko

Tahoe, the Least-Authority Filesystem, currently has the following reactor-selection login in its Makefile:

http://allmydata.org/trac/tahoe/browser/Makefile?rev=2847

17	PLAT = $(strip $(shell $(PYTHON) -c "import sys ; print sys.platform"))
...
25	        ifeq ($(PLAT),linux2)
26	                # This is to work-around #402, and anyway the poll reactor is probably better on Linux, if
27	                # we have a lot of open fds.
28	                ifeq ($(REACTOR),)
29	                        REACTOR := poll
30	                endif
31	        endif
32	        ifeq ($(PLAT),cygwin)
33	                # The cygwin select reactor seems to run out of fds in unit tests -- it writes "filedescriptor
34	                # out of range in select()".  Setting reactor=poll fixes that.
35	                ifeq ($(REACTOR),)
36	                        REACTOR := poll
37	                endif
38	        endif
... 
46	ifneq ($(REACTOR),)
47	        REACTOROPT := --reactor=$(REACTOR)
48	else
49	        REACTOROPT :=
50	endif

It seems like poll reactor could be the default reactor on platforms which support poll reactor, and then Tahoe could stop doing anything about reactor choice at all in its Makefile.

comment:3 Changed 6 years ago by zooko

The following tahoe issue ticket now contains a link to this ticket:

http://allmydata.org/trac/tahoe/ticket/402 # bug in Twisted, triggered by pyOpenSSL-0.7

comment:4 Changed 6 years ago by glyph

You lost me at "Makefile". Would you mind rephrasing that chunk of stuff in a language that humans understand - like, say, Python? :-)

comment:5 Changed 6 years ago by warner

As background, the Makefile stuff that Zooko quoted above is from the Tahoe
project's top-level Makefile, which really just contains convenient aliases
so I can type 'make test' instead of 'PYTHONPATH=localstuff trial
--reactor=poll allmydata.test'.

To work around #3218 (which causes foolscap and tahoe test failures under
Twisted-8.x and pyOpenSSL-0.7 on the select reactor), we changed this
Makefile to add --reactor=poll to the trial command line, but since not all
platforms support pollreactor, there are a bunch of conditionals to make sure
we don't use --reactor=poll on, say, windows. (we might have something
elsewhere to use iocpreactor there).

If trial could be conveniently invoked from python code, we would probably
replace this 'make test' target with a python script that did something like:

from twisted.python import runtime
r = "default"
if runtime.platformType == "posix":
    r = "poll"
sys.exit(trialRunner(suite, reactor=r))

(in fact, zooko would probably write a setuptools plugin to make 'python
setup.py test' do exactly this :-).

comment:6 Changed 6 years ago by zooko

In fact, I've already written such a setuptools plugin, by borrowing code from the Elise project, but there may have been some problem using it, or else I just got distracted before I finished it. I'll probably dig it up again, especially since Barry Warsaw just wished for better setuptools integration for tests: http://mail.python.org/pipermail/python-dev/2008-August/081764.html

But, actually this has little to do with the topic of this ticket, which is that choosing the select reactor as the default doesn't seem as good as choosing poll reactor as the default, if there is a poll reactor available.

comment:7 Changed 6 years ago by zooko

  • Cc zooko added

comment:8 Changed 6 years ago by zooko

  • Cc zooko@… added

comment:9 Changed 6 years ago by exarkun

Poll shouldn't be used by default on OS X.

comment:10 Changed 6 years ago by zooko

Okay, so how about if the default reactor is poll on linux and cygwin and select on everything else? That's what the Tahoe makefile currently does.

comment:11 Changed 6 years ago by exarkun

cygwin isn't an officially supported platform.

Other than that, no problems with that suggestion are immediately obvious.

Do you want to supply a patch?

comment:12 Changed 6 years ago by zooko

  • Owner changed from glyph to zooko
  • Status changed from new to assigned

I would be interested in contributing a cygwin buildslave.

Yes, I want to supply a patch.

comment:13 Changed 6 years ago by exarkun

Cool, a cygwin slave would be great.

comment:14 Changed 6 years ago by zooko

Please provide name, password, and master for the cygwin buildbot. Also, would you like a cygwin buildbot for pyOpenSSL too?

comment:15 Changed 6 years ago by exarkun

A comment on the tracker wouldn't be a good place for most of that information. Find me on IRC.

comment:16 Changed 6 years ago by zooko

Okay, we have a cygwin buildslave now, but it goes into a fast loop and fills my filesystem with test log:

http://buildbot.twistedmatrix.com/builders/cygwin-py2.5-select/builds/3

comment:17 Changed 6 years ago by zooko

#3529 is a bug which prevents me from running a buildslave on cygwin. Until that bug is fixed or worked-around in some way so that I can run a cygwin buildslave, I will delay working on this issue.

comment:18 Changed 6 years ago by zooko

This issue also affects setuptools_trial -- setuptools_trial issue #2

comment:19 Changed 6 years ago by zooko

This ticket has been mentioned on the tahoe-dev list:
http://allmydata.org/pipermail/tahoe-dev/2009-January/001027.html

comment:20 Changed 6 years ago by zooko

So I was just hacking setuptools_trial to select the poll reactor on cygwin or linux2:

http://allmydata.org/trac/setuptools_trial/changeset/13

When I realized -- hey wait a minute, why am I waiting for the cygwin buildslave to work? Why don't we fix this ticket for all the platforms which are already passing Twisted buildbot tests and worry about cygwin once we have a working cygwin buildslave?

comment:21 Changed 5 years ago by glyph

#4153 was a duplicate of this.

comment:22 Changed 5 years ago by exarkun

I agree. How about restricting the scope of this ticket. I propose just one thing: if the platform is linux, use poll reactor as the default; for all other cases, continue to use select reactor as the default.

Once this is implemented, we can file tickets for further refinements to the selection scheme.

comment:23 Changed 5 years ago by zooko

  • Summary changed from Select default reactor based on platform and available libraries to use poll reactor if on linux

Good idea!

comment:24 Changed 5 years ago by exarkun

  • Author set to exarkun
  • Branch set to branches/poll-default-2234

(In [28660]) Branching to 'poll-default-2234'

comment:25 Changed 5 years ago by exarkun

(In [28661]) Parameterize more of how Platform works so I can pretend to be on OS X when I'm on Linux

refs #2234

comment:26 Changed 5 years ago by exarkun

There's a stdio/pollreactor issue. I filed #4352 for it. This issue is blocked until that's resolved.

comment:27 Changed 5 years ago by itamar

  • Cc itamar added

As long as we're changing this, I'd want these defaults:

Linux: epoll, switching to POSIX default if unavailable
POSIX: poll if available, otherwise select

Simply trying epoll,poll,select in that order on POSIX platforms would accomplish this.

comment:28 Changed 5 years ago by exarkun

  • Priority changed from low to normal

Eventually, yes. I'm not going to do all that, though. I'm just going to make a simple change. Further customizing the selection logic should be easier based on the outcome of this ticket, though.

comment:29 Changed 5 years ago by exarkun

  • Branch changed from branches/poll-default-2234 to branches/poll-default-2234-2

(In [28680]) Branching to 'poll-default-2234-2'

comment:30 Changed 5 years ago by itamar

A note on the current implementation: many platforms are POSIX but not Mac OS X, not just Linux. One *hopes* all of them have poll, but somehow that seems too much to ask... It would be nice not to break the default reactor on such platforms, even if they're unsupported.

Checking for existence of poll object in select module might be better.

comment:31 Changed 3 years ago by itamarst

  • Author changed from exarkun to itamarst, exarkun
  • Branch changed from branches/poll-default-2234-2 to branches/poll-default-2234-3

(In [31590]) Branching to 'poll-default-2234-3'

comment:32 Changed 3 years ago by itamar

  • Keywords review added
  • Owner zooko deleted
  • Status changed from assigned to new
  • Summary changed from use poll reactor if on linux to use poll reactor on POSIX platform that provide poll(2)

Ready for review, I think. Buildbot run:

http://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/poll-default-2234-3

The problems appear to be normal ongoing windows issues.

comment:33 Changed 3 years ago by zooko

  • Keywords reviewed added; review removed

Reviewed: I read through this patch and didn't see anything wrong with it. I liked the clarification that this isn't actually choosing the "best" reactor.

(Do I need to do anything else to mark this ticket as reviewed?)

comment:34 Changed 3 years ago by itamar

Standard procedure is to remove "review" keyword and reassign back to author.

ReviewProcess documents the things you should be checking.

comment:35 Changed 3 years ago by zooko

  • Keywords review added; reviewed removed
  • Owner set to zooko
  • Status changed from new to assigned

comment:36 Changed 3 years ago by exarkun

(In [31595]) Remove copyright date

refs #2234

comment:37 Changed 3 years ago by exarkun

What's this stuff about "modern OS X"? OS X 10.6.7, the Apple distributed Python executable does not have it.

comment:38 Changed 3 years ago by itamar

AFAICT the *OS* at least has it, which is why I assumed Python exposed it:
http://developer.apple.com/library/mac/#documentation/Darwin/Reference/ManPages/man2/poll.2.html

According to this - http://bugs.python.org/issue5154 (as usual, foom knows the answer to everything), Apple chose not to expose because certain fds aren't supported. The only relevant one is PTYs, but that is an issue (why does OS X suck?).

The implication of the linked bug is that some releases of Python might decide expose a still-broken poll, so possibly hardcoding select() on OS X is still the right thing to do... Maybe we should ask James, since he's well informed.

comment:39 Changed 3 years ago by zooko

  • Keywords review removed
  • Owner changed from zooko to itamar
  • Status changed from assigned to new

Okay, I have reviewed that patch and checked for the things mentioned in ReviewProcess. Other than the issue raised in comment:38, it is ready to merge.

comment:40 Changed 3 years ago by itamar

  • Keywords review added
  • Owner itamar deleted

I reverted back to old behavior (no poll on OSX), and added description of why we are not choosing e.g. epoll on Linux just yet.

http://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/poll-default-2234-3 doesn't show any relevant errors AFAICT.

comment:41 Changed 3 years ago by exarkun

  • Keywords review removed
  • Owner set to itamar

Thanks!

  1. Please remove the date from test_default.py's copyright header
  2. Can you change the docstring style for isMacOSX in runtime.py too?
  3. I think the news fragment would benefit from a "The ..." at the beginning.

I wonder if the level of detail now in the default reactor's description (in the twisted_reactors.py dropin file) will scale to more complex selection schemes. We'll find out, I guess. :)

Please address the above to your satisfaction and then merge. Thanks!

comment:42 follow-up: Changed 3 years ago by glyph

I just noticed http://twistedmatrix.com/trac/changeset/31761 go by in my RSS reader, and I have one comment:

We shouldn't have FUD about our own code, especially in comments. The issues with epoll on Linux and poll(2) on OS X are nicely well-described. But the issues with kqueue ("isn't in great shape") and IOCP ("probably not quite ready for prime time") are super vague, and, in the latter case, I think possibly wrong.

Please link to specific issues in that comment so that a future maintainer can know whether it's an appropriate time to update the code to select a better default reactor. (Also, while you're at it, transforming "#4429" into the appropriate URL would be a kindness to future readers.)

comment:43 in reply to: ↑ 42 ; follow-up: Changed 3 years ago by itamar

Replying to glyph:

I'll make the descriptions more accurate (remind me what the kqueue issues are? besides PTY support.)

That being said: if IOCP really doesn't have any reported issues (which I doubt, but will verify) would you be willing to have IOCP the default on Windows?

comment:44 in reply to: ↑ 43 Changed 3 years ago by glyph

Replying to itamar:

Replying to glyph:

I'll make the descriptions more accurate (remind me what the kqueue issues are? besides PTY support.)

PTY support's a big one, but ... hrm. the relevant ticket is a big mess that doesn't really describe the issues, but that's probably the thing to link to. Somebody should update the summary there. Possibly in the comment you could just say that it is "out of date".

That being said: if IOCP really doesn't have any reported issues (which I doubt, but will verify) would you be willing to have IOCP the default on Windows?

"any" reported issues? :) I think the important thing would be fewer reported issues than the select reactor on Windows :). Well, that, and no significant missing functionality. SSL was the big one for a long time, but now that that's taken care of, what's left? Do serial ports work? Standard I/O? I know that subprocesses use the same gross hack as all the other Windows reactors.

I'd definitely be willing - happy, even - to have it be the default, but I'm happy to be overridden here by anyone with an issue that I haven't thought of (Pavel or Jean-Paul might know about something).

comment:45 Changed 3 years ago by itamarst

  • Resolution set to fixed
  • Status changed from new to closed

(In [31768]) Merge poll-default-2234-3.

Author: exarkun, itamar
Reviewer: exarkun, glyph
Fixes: #2234

On platforms that support it (Linux, FreeBSD, and other non-OS X POSIX platforms) use poll reactor by default.

Note: See TracTickets for help on using tickets.