Repository navigation
gh-157184: Scale the join() alarm in the multiprocessing kill tests - #157185
iamsharduld wants to merge 2 commits into
Conversation
Sorry, something went wrong.
…ests _kill_process() interrupts the join() of the killed child with SIGALRM so that a blocked waitpid() becomes a readable error instead of a hang, but the alarm has been a fixed 10 seconds since 2013. On a build slow enough that reaping the child legitimately takes longer, it fires on a healthy run: seen on the UBSan CI job, where the alarm interrupted os.waitpid() itself. Use support.LONG_TIMEOUT, which is documented for detecting hangs and is scaled by regrtest for slow workers, as the same function already does for its event wait.
sharktide
left a comment
There was a problem hiding this comment.
The code implementation looks good to me, although I've been gone a few months and coming back, I don't remember if we used to do news entries on purely test fail fixes which are non-user facing changes. If something changed recently, please let me know!
Sorry, something went wrong.
|
Nothing changed; you are right and I was wrong to add one. The devguide lists test changes among the cases that do not need a NEWS entry, and on a sibling PR of mine (#157158) @kumaraditya303 asked for the entry to be removed and applied the skip news label. I have removed it here too, so bedevere/news will need that label from a triager. The same applies to my other test-only PRs that still carry an entry (#157162, #157171); I will leave those alone unless a triager prefers them removed as well, to avoid churn on PRs nobody has looked at yet. |
Sorry, something went wrong.
_kill_process() guards the join() of the killed child with a SIGALRM so that a blocked waitpid() becomes a readable error instead of a hang. The alarm has been a literal 10 seconds since it was added in 2013 (cc5c728), so on a build slow enough that reaping the child legitimately takes longer than that, it fires on a healthy run and fails the test. That is what happened on the UBSan job of 57594aa (traceback in the issue): the alarm interrupted os.waitpid() itself.
test.support.LONG_TIMEOUT is documented for this exact purpose, "Timeout in seconds to detect when a test hangs [...] It should not be used to mark a test as failed if the test takes 'too long'", and regrtest scales it from --timeout for slow workers. The SHORT_TIMEOUT docs point the same way: "If a test using SHORT_TIMEOUT starts to fail randomly on slow buildbots, use LONG_TIMEOUT instead." _kill_process() already uses support.SHORT_TIMEOUT a few lines above for its event wait. math.ceil() keeps the alarm from being cancelled outright if a very short --timeout scales the value below one second.
Four tests go through this helper, in every start-method variant: test_interrupt, test_interrupt_no_handler, test_terminate and test_kill.
Verified by making the child slow to die (a SIGINT handler that sleeps 12 seconds), which is what a loaded machine looks like from the parent's side, and running the real test_interrupt. On Linux with the fork start method, the configuration that failed on CI, and on macOS with spawn:
Without the injected delay, test_multiprocessing_spawn, test_multiprocessing_forkserver, test_multiprocessing_fork, test_multiprocessing_main_handling and test_concurrent_futures pass (1427 tests).