epoll reactor stdio fails when output redirected to a file

[exarkun]

This is like #2259, but only for epollreactor. #2259 fixed the behavior for all the other reactors.

[itamar]

Implementing this with immediate writes that are assumed to not block, and reads driven by coiterate(), may be sufficient for a first pass; producer API can be done as separate ticket.

[itamarst]

(In [34301]) Branching to 'epoll-EPERM-4429'

[itamar]

 http://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/epoll-EPERM-4429

Design is basically as described above in comment 3.

[jknight]

Why a 0.001sec delay rather than 0sec? Other reactors will trigger file reads/writes every iteration.

[exarkun]

Thanks.

1. twisted.internet.epollreactor isn't necessarily importable. New tests need to handle the import error and skip as necessary.
2. EpollEPERMTests creates a reactor and leaks a bunch of its state. See ReactorBuilder for all the things you have to do to clean up after a reactor. It seems like most or all of the tests on this class actually should be ReactorBuilder tests and moved to a general stdio test module. The functionality should work on all reactors, not just epoll, shouldn't it?
3. To stick with the theme foom raised, the Twisted Committee Against Arbitrary Magic Numbers recommends a delay of 0 instead of 0.001.
4. PosixReactorBase._disconnectSelectable seems to have a lot more logic than _ContinuousPolling._disconnectSelectable. Does this stdio implementation still support half close? Does it clean up the readers and writers properly?

[itamar]

Review comments were all addressed; new build:

 http://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/epoll-EPERM-4429

I'm pretty sure the documentation buildslave's complaints are bogus.

[nekto0n]

I think this comment should fixed too.

[itamar]

It is fixed, in the branch under review. You can see the branch in the ticket description at the top of the page.

[therve]

• pyflakes:

  twisted/internet/test/test_stdio.py:52: local variable 'stdio' is assigned to but never used
  twisted/internet/test/test_stdio.py:76: local variable 'stdio' is assigned to but never used
  twisted/internet/test/test_epollreactor.py:10: 'EPollReactor' imported but unused
  twisted/internet/_posixstdio.py:14: 'errno' imported but unused
  twisted/internet/_posixstdio.py:14: 'os' imported but unused
  

• +                self._loop.start(0.00000001, now=False)
  

  Maybe it's worth reusing task._EPSILON?

• +            if e.errno == errno.EPERM:
  +                self._continuousPolling.addReader(reader)
  

  Can you add a small comment around those exceptions?

• There are some chars after 80 in test_epollreactor.py *

          self.extraFile = file(path, "r+")
  

  Shouldn't we close this in tearDown?

Some small questions, but the branch seems fine, please merge.

[itamar]

Closing file in tearDown may cause problem storing extraFile as attribute was supposed to fix, where reactor cleanup tries to operate on closed file descriptor. So I'm not going to bother. Everything else is fixed.

[itamarst]

(In [34346]) Merge epoll-EPERM-4429: epoll reactor now supports filesystem files about as well as other reactors do.

Author: itamar Review: exarkun, jknight, therve Fixes: #4429

If you do redirect stdin or stdout to a file, epoll reactor will no longer blow up.