gh-157184: Scale the join() alarm in the multiprocessing kill tests - #157185
gh-157184: Scale the join() alarm in the multiprocessing kill tests#157185iamsharduld wants to merge 2 commits into
Conversation
…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!
|
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 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. |
_kill_process()guards thejoin()of the killed child with aSIGALRMso that a blockedwaitpid()becomes a readable error instead of a hang. The alarm has been a literal10seconds 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 interruptedos.waitpid()itself.test.support.LONG_TIMEOUTis 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--timeoutfor slow workers. TheSHORT_TIMEOUTdocs point the same way: "If a test usingSHORT_TIMEOUTstarts to fail randomly on slow buildbots, useLONG_TIMEOUTinstead."_kill_process()already usessupport.SHORT_TIMEOUTa few lines above for its event wait.math.ceil()keeps the alarm from being cancelled outright if a very short--timeoutscales the value below one second.Four tests go through this helper, in every start-method variant:
test_interrupt,test_interrupt_no_handler,test_terminateandtest_kill.Verified by making the child slow to die (a
SIGINThandler that sleeps 12 seconds), which is what a loaded machine looks like from the parent's side, and running the realtest_interrupt. On Linux with the fork start method, the configuration that failed on CI, and on macOS with spawn:signal.alarm(10)join took too long— FAILUREjoin took too long— FAILUREWithout the injected delay,
test_multiprocessing_spawn,test_multiprocessing_forkserver,test_multiprocessing_fork,test_multiprocessing_main_handlingandtest_concurrent_futurespass (1427 tests).