Opened 4 years ago

Closed 2 years ago

#4429 defect closed fixed (fixed)

epoll reactor stdio fails when output redirected to a file

Reported by: exarkun Owned by: itamar
Priority: normal Milestone: Twisted-12.1
Component: core Keywords:
Cc: itamar@…, nikita.vetoshkin@… Branch: branches/epoll-EPERM-4429
(diff, github, buildbot, log)
Author: itamar Launchpad Bug:

Description

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

Change History (17)

comment:1 Changed 4 years ago by itamar

  • Cc itamar@… added

comment:2 Changed 4 years ago by <automation>

  • Owner glyph deleted

comment:3 Changed 3 years ago by 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.

comment:4 Changed 2 years ago by itamarst

  • Author set to itamarst
  • Branch set to branches/epoll-EPERM-4429

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

comment:5 Changed 2 years ago by itamar

  • Author changed from itamarst to itamar
  • Owner set to itamar

comment:6 Changed 2 years ago by itamar

  • Keywords review added
  • Milestone set to Twisted-12.1
  • Owner itamar deleted

comment:7 Changed 2 years ago by jknight

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

comment:8 Changed 2 years ago by exarkun

  • Owner set to exarkun
  • Status changed from new to assigned

comment:9 Changed 2 years ago by exarkun

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

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 2 years ago by itamar

  • Keywords review added
  • Owner itamar 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 2 years ago by nekto0n

I think this comment should fixed too.

comment:12 Changed 2 years ago by nekto0n

  • Cc nikita.vetoshkin@… added

comment:13 Changed 2 years ago by itamar

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 2 years ago by therve

  • Keywords review removed
  • Owner set to exarkun
  • 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 2 years ago by therve

  • Owner changed from exarkun to itamar

comment:16 Changed 2 years ago by 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.

comment:17 Changed 2 years ago by itamarst

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

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