GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||||||
GeezerVison is showing github.com — gh-157184: Scale the join() alarm in the multiprocessing kill tests by iamsharduld · Pull Request #157185 · python/cpython Conversation
|
| Linux, fork | macOS, spawn | |
|---|---|---|
signal.alarm(10) |
join took too long — FAILURE |
join took too long — FAILURE |
| this change | SUCCESS | SUCCESS |
Without the injected delay, test_multiprocessing_spawn, test_multiprocessing_forkserver, test_multiprocessing_fork, test_multiprocessing_main_handling and test_concurrent_futures pass (1427 tests).
Sorry, something went wrong.
There was an error while loading. Please reload this page.
…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.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
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.
There was an error while loading. Please reload this page.
|
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. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
I wrote a different fix which logs the timeout in the error message: #160281. I close this PR, thanks for your contribution anyway! |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.