Page MenuHomePhabricator

worker: make windows workers daemons
ClosedPublic

Authored by wlis on Nov 30 2017, 2:26 PM.

Details

Summary

The windows workers weren't daemons and were not correctly killed when ctrl-c'd from the terminal. Withi this change when the main thread is killed, all daemons get killed as well.
I also reduced the time we give to workers to cleanup nicely to not have people ctrl-c'ing when they get inpatient.

The output when threads clened up nicely:

PS C:\<dir>> hg.exe sparse --disable-profile SparseProfiles/<profile>.sparse
interrupted!

The output when threads don't clenup in 1 sec:

PS C:\<dir> hg.exe sparse --enable-profile SparseProfiles/<profile>.sparse
failed to kill worker threads while handling an exception
interrupted!
Exception in thread Thread-4 (most likely raised during interpreter shutdown):
PS C:\<dir>>
Test Plan

Run hg command on windows (pull/update/sparse). Ctrl-C'd sparse --enable-profile command that was using threads and observed in proces explorer that all threads got killed.
ran tests on CentOS

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

wlis created this revision.Nov 30 2017, 2:26 PM
ikostia accepted this revision.Nov 30 2017, 4:10 PM
ikostia added a subscriber: ikostia.

This *looks* reasonable to me, but I am afraid I could be missing something important. Anyway, since we can turn it off via a config, I'd say it's good to be in the hotfixes.

durham added a subscriber: durham.Nov 30 2017, 4:29 PM
durham added inline comments.
mercurial/worker.py
286

Why only do it on keyboard interrupt? What if there's another exception? If you did it for all exceptions, you could drop the trykillworkers() inside the loop, and just throw the exception up to here.

wlis added inline comments.Nov 30 2017, 4:39 PM
mercurial/worker.py
286

this is a very good point. will do.

wlis updated this revision to Diff 4024.Nov 30 2017, 7:02 PM
ikostia accepted this revision.Dec 1 2017, 11:11 AM
durin42 added a subscriber: durin42.Dec 5 2017, 5:50 PM

Is this missing a dependent revision annotation?

(specifically on D1460?)

durin42 accepted this revision.

This looks good to me, but I managed to confirm that it does depend on D1460 by looking at patch metadata via hg phabread, so I've added the necessary annotation.

This revision is now accepted and ready to land.Dec 11 2017, 12:24 PM
This revision was automatically updated to reflect the committed changes.
wlis added inline comments.Dec 18 2017, 2:04 PM
mercurial/worker.py
285

I forgot to update this diff with KeyboardInterrupt again which doesn't derive from Exception, but BaseException instead. I will try to get the 1 line patch up asap.
Currently the ctrl-c wouldn't be handled gracefully on windows