Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Workaround for small race condition in parallel doctest runner #24563

Closed
embray opened this issue Jan 19, 2018 · 10 comments
Closed

Workaround for small race condition in parallel doctest runner #24563

embray opened this issue Jan 19, 2018 · 10 comments

Comments

@embray
Copy link
Contributor

embray commented Jan 19, 2018

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.

CC: @jdemeyer

Component: doctest framework

Author: Erik Bray

Branch/Commit: 0b72ed5

Reviewer: Jeroen Demeyer

Issue created by migration from https://trac.sagemath.org/ticket/24563

@embray embray added this to the sage-8.2 milestone Jan 19, 2018
@jdemeyer
Copy link

comment:2

Some of the calls to this kill method are wrapped already in a try/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 to kill in a try/except block.

@jdemeyer
Copy link

comment:3

Maybe a better alternative solution: return a boolean from kill() to indicate whether or not os.kill() was successful in sending the signal.

@embray
Copy link
Contributor Author

embray commented Jan 22, 2018

comment:4

Replying to @jdemeyer:

Maybe a better alternative solution: return a boolean from kill() to indicate whether or not os.kill() was successful in sending the signal.

+1

@embray
Copy link
Contributor Author

embray commented Jan 22, 2018

comment:5

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.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 22, 2018

Changed commit from 8e4506e to 0b72ed5

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 22, 2018

Branch pushed to git repo; I updated commit sha1. New commits:

0b72ed5Change DocTestWorker.kill to return a value whether or not it actually signalled the process.

@embray
Copy link
Contributor Author

embray commented Jan 22, 2018

New commits:

0b72ed5Change DocTestWorker.kill to return a value whether or not it actually signalled the process.

@jdemeyer
Copy link

Reviewer: Jeroen Demeyer

@embray
Copy link
Contributor Author

embray commented Jan 30, 2018

comment:9

I like how nearly every patchbot is apparently broken...

@vbraun
Copy link
Member

vbraun commented Feb 1, 2018

Changed branch from u/embray/cygwin/doctest-kill to 0b72ed5

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants