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

[exarkun]

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

[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.

[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

[glyph]

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

[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 :-).

[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.

[exarkun]

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

[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.

[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?

[zooko]

I would be interested in contributing a cygwin buildslave.

Yes, I want to supply a patch.

[exarkun]

Cool, a cygwin slave would be great.

[zooko]

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

[exarkun]

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

[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

[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.

[zooko]

This issue also affects setuptools_trial --  setuptools_trial issue #2

[zooko]

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

[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?

[glyph]

#4153 was a duplicate of this.

[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.

[zooko]

Good idea!

[exarkun]

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

[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

[exarkun]

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

[itamar]

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.

[exarkun]

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.

[exarkun]

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

[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.

[itamarst]

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

[itamar]

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.

[zooko]

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?)

[itamar]

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

ReviewProcess documents the things you should be checking.

[exarkun]

(In [31595]) Remove copyright date

refs #2234

[exarkun]

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

[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.

[zooko]

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.

[itamar]

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.

[exarkun]

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!

[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.)

[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?

[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).

[itamarst]

(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.