Ticket #791 defect closed invalid

Opened 8 years ago

Last modified 4 years ago

reapAllProcesses occassionally, as apparently signal delivery is unreliable

Reported by: itamarst Owned by:
Priority: high Milestone:
Component: core Keywords:
Cc: itamarst, jknight, spiv, exarkun, glyph, jml Branch: branches/sigchld-791-3
Author: glyph Launchpad Bug:

Description


Change History

1

Changed 8 years ago by jknight

Has this actually been verified to be true?

2

Changed 7 years ago by glyph

  • component set to conch

It doesn't matter; see comments on #1997. We need to do this anyway.

3

Changed 7 years ago by glyph

  • component changed from conch to core

4

Changed 7 years ago by jknight

Implemented fix in source:branches/sigchld-791/

Technique: a C module which installs a signal handler for SIGCHLD, with SA_RESTART, and a C function to write to a given fd. The reactor listens on that fd, and calls reapAllProcesses when there's activity.

However, if the C module is not present, it instead starts a LoopingCall running whenever there are processes waiting to be reaped, and doesn't install a SIGCHLD handler at all.

5

Changed 7 years ago by jknight

  • cc spiv, exarkun, glyph, jml added

Also related to and should fix #1430 #1997

6

Changed 7 years ago by jknight

And also #733

7

Changed 6 years ago by therve

  • keywords review added
  • owner itamarst deleted

It seems it would need review ?

8

Changed 6 years ago by exarkun

  • keywords review removed

It doesn't have sufficient unit tests.

9

Changed 6 years ago by mithrandi

  • owner set to jknight

10

Changed 6 years ago by itamarst

The LoopingCall should also be used if someone does reactor.run(installSignalHandlers=False), if it's not already. And probably should stop running if there's no Process instances and restart if necessary (so trial is happy), again, if it's not already got that behavior in the branch.

11

Changed 5 years ago by exarkun

In addition to #733 and #1997, see #2535.

12

Changed 5 years ago by therve

  • branch set to branches/sigchld-791-2
  • author set to therve

(In [23160]) Branching to 'sigchld-791-2'

13

Changed 5 years ago by therve

I had the opportunity to look at this a bit more. I think the looping call solution is wrong. Do we know exactly what platform doesn't have SA_RESTART? I think I'd prefer the current behavior with SIGCHLD than a polling system that browses every processes launched 10 times per second. This conflicts pretty badly with #2967.

14

Changed 5 years ago by glyph

  • status changed from new to assigned
  • owner changed from jknight to glyph

15

Changed 5 years ago by glyph

  • branch changed from branches/sigchld-791-2 to branches/sigchld-791-3
  • author changed from therve to glyph

(In [23726]) Branching to 'sigchld-791-3'

16

Changed 4 years ago by exarkun

  • status changed from assigned to closed
  • resolution set to invalid

SIGCHLD delivery is not unreliable. What is meant by "unreliable" when people talk about signal delivery is that multiple instances of a particular signal may be coalesced into one call to a signal handler in a process. So polling isn't necessary to deal with "unreliable signal delivery".

However, some of the code in this branch is still probably useful for resolving some Twisted process issues. For example, sigaction SA_RESTART prevents us from breaking os.popen. So some of this code should be used to resolve some tickets.

I'm closing this ticket because the problem it describes does not exist and the solution it suggests is therefore unnecessary. Since there is useful code in the branch, I will re-associate it with the ticket it's most directed at fixing, #733 (the branch conflicts with trunk, so it needed to be merged forward anyway).

17

Changed 2 years ago by <automation>

  • owner glyph deleted
Note: See TracTickets for help on using tickets.