Opened 7 years ago

Closed 5 years ago

#4429 defect closed fixed (fixed)

epoll reactor stdio fails when output redirected to a file

Reported by: Jean-Paul Calderone Owned by: Itamar Turner-Trauring
Priority: normal Milestone: Twisted-12.1
Component: core Keywords:
Cc: itamarst, nekto0n Branch: branches/epoll-EPERM-4429
branch-diff, diff-cov, branch-cov, buildbot
Author: itamar

Description

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

Change History (17)

comment:1 Changed 7 years ago by Itamar Turner-Trauring

Cc: itamarst added

comment:2 Changed 7 years ago by <automation>

Owner: Glyph deleted

comment:3 Changed 5 years ago by Itamar Turner-Trauring

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.

comment:4 Changed 5 years ago by itamarst

Author: itamarst
Branch: branches/epoll-EPERM-4429

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

comment:5 Changed 5 years ago by Itamar Turner-Trauring

Author: itamarstitamar
Owner: set to Itamar Turner-Trauring

comment:6 Changed 5 years ago by Itamar Turner-Trauring

Keywords: review added
Milestone: Twisted-12.1
Owner: Itamar Turner-Trauring deleted

comment:7 Changed 5 years ago by jknight

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

comment:8 Changed 5 years ago by Jean-Paul Calderone

Owner: set to Jean-Paul Calderone
Status: newassigned

comment:9 Changed 5 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: changed from Jean-Paul Calderone to Itamar Turner-Trauring
Status: assignednew

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?

comment:10 Changed 5 years ago by Itamar Turner-Trauring

Keywords: review added
Owner: Itamar Turner-Trauring deleted

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.

comment:11 Changed 5 years ago by nekto0n

I think this comment should fixed too.

comment:12 Changed 5 years ago by nekto0n

Cc: nekto0n added

comment:13 Changed 5 years ago by Itamar Turner-Trauring

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

comment:14 Changed 5 years ago by therve

Keywords: review removed
Owner: set to Jean-Paul Calderone
  • 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.

comment:15 Changed 5 years ago by therve

Owner: changed from Jean-Paul Calderone to Itamar Turner-Trauring

comment:16 Changed 5 years ago by Itamar Turner-Trauring

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.

comment:17 Changed 5 years ago by itamarst

Resolution: fixed
Status: newclosed

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

Note: See TracTickets for help on using tickets.