Opened 3 years ago

Last modified 3 years ago

#5680 defect new

test_timelyProcessExited is fragile and may not catch actual problems

Reported by: itamar Owned by: itamar
Priority: normal Milestone:
Component: core Keywords:
Cc: Branch: branches/process-test-5680
(diff, github, buildbot, log)
Author: itamar Launchpad Bug:

Description

JP pointed out that test_timelyProcessExited in twisted.internet.test_process may fail without us noticing:

The problem is that the timeout support in runReactor is based on the IReactorTime implementation of the reactor it is running. It's quite likely that if the reactor is broken, instead of failing:

  1. the child will exit
  2. the reactor will not process the signal right away
  3. the timeout will elapse
  4. the reactor will wake up
  5. the signal will be processed, calling processEnded
  6. reactor.stop() will be called
  7. runReactor will notice the reactor was stopped and not raise a timeout exception
  8. the test will pass

Change History (3)

comment:1 Changed 3 years ago by itamarst

  • Author set to itamarst
  • Branch set to branches/process-test-5680

(In [34411]) Branching to 'process-test-5680'

comment:2 Changed 3 years ago by itamar

  • Author changed from itamarst to itamar
  • Keywords review added
  • Owner itamar deleted

Ready for review; also covers #5679. I scheduled but haven't yet checked a buildbot run:

http://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/process-test-5680

comment:3 Changed 3 years ago by therve

  • Keywords review removed
  • Owner set to itamar

Looks good. I would add a parameter to assertTrue with the value of exist - start so that we can debug it in case of problems. Please merge after that.

Note: See TracTickets for help on using tickets.