Opened 4 years ago
Closed 4 years ago
#24563 closed defect (fixed)
Workaround for small race condition in parallel doctest runner
Reported by: | embray | Owned by: | |
---|---|---|---|
Priority: | minor | Milestone: | sage-8.2 |
Component: | doctest framework | Keywords: | |
Cc: | jdemeyer | Merged in: | |
Authors: | Erik Bray | Reviewers: | Jeroen Demeyer |
Report Upstream: | N/A | Work issues: | |
Branch: | 0b72ed5 (Commits, GitHub, GitLab) | Commit: | 0b72ed55a5261fe632e296b4677faa43f79e5611 |
Dependencies: | Stopgaps: |
Description
This is hard to reproduce reliably, but can happen if a worker process outlives its deadline and w.kill()
is called, but the process has exited on its own by the time killpg()
is called.
I've had this happen a few times on Cygwin where process shutdown tends to take a little longer.
Change History (10)
comment:1 Changed 4 years ago by
- Status changed from new to needs_review
comment:2 Changed 4 years ago by
- Status changed from needs_review to needs_work
comment:3 follow-up: ↓ 4 Changed 4 years ago by
Maybe a better alternative solution: return a boolean from kill()
to indicate whether or not os.kill()
was successful in sending the signal.
comment:4 in reply to: ↑ 3 Changed 4 years ago by
Replying to jdemeyer:
Maybe a better alternative solution: return a boolean from
kill()
to indicate whether or notos.kill()
was successful in sending the signal.
+1
comment:5 Changed 4 years ago by
Although maybe instead of "successful in sending the signal" it should be "successful in sending the signal or is already dead". After all what I've written will allow other exceptions to be raised.
No, I changed my mind. I'll do it the way you suggested.
comment:6 Changed 4 years ago by
- Commit changed from 8e4506ea84eb812b66a2e7bd0bbf5432da4ddb46 to 0b72ed55a5261fe632e296b4677faa43f79e5611
Branch pushed to git repo; I updated commit sha1. New commits:
0b72ed5 | Change DocTestWorker.kill to return a value whether or not it actually signalled the process.
|
comment:7 Changed 4 years ago by
- Status changed from needs_work to needs_review
New commits:
0b72ed5 | Change DocTestWorker.kill to return a value whether or not it actually signalled the process.
|
comment:8 Changed 4 years ago by
- Reviewers set to Jeroen Demeyer
- Status changed from needs_review to positive_review
comment:9 Changed 4 years ago by
I like how nearly every patchbot is apparently broken...
comment:10 Changed 4 years ago by
- Branch changed from u/embray/cygwin/doctest-kill to 0b72ed55a5261fe632e296b4677faa43f79e5611
- Resolution set to fixed
- Status changed from positive_review to closed
Some of the calls to this
kill
method are wrapped already in atry
/except Exception
block. So it seems to be a feature that this would raise an exception if the worker is already dead. I would therefore prefer to just wrap the one remaining call tokill
in atry
/except
block.