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:

Status badges

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 embray

  • Status changed from new to needs_review

comment:2 Changed 4 years ago by jdemeyer

  • Status changed from needs_review to needs_work

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.

comment:3 follow-up: Changed 4 years ago by jdemeyer

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

Last edited 4 years ago by jdemeyer (previous) (diff)

comment:4 in reply to: ↑ 3 Changed 4 years ago by embray

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

comment:5 Changed 4 years ago by embray

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.

Last edited 4 years ago by embray (previous) (diff)

comment:6 Changed 4 years ago by git

  • Commit changed from 8e4506ea84eb812b66a2e7bd0bbf5432da4ddb46 to 0b72ed55a5261fe632e296b4677faa43f79e5611

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.

comment:7 Changed 4 years ago by embray

  • Status changed from needs_work to needs_review

New commits:

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

comment:8 Changed 4 years ago by jdemeyer

  • Reviewers set to Jeroen Demeyer
  • Status changed from needs_review to positive_review

comment:9 Changed 4 years ago by embray

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

comment:10 Changed 4 years ago by vbraun

  • Branch changed from u/embray/cygwin/doctest-kill to 0b72ed55a5261fe632e296b4677faa43f79e5611
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.