Page MenuHomePhabricator

[terminate] daemon thread approach
ClosedPublic

Authored by alangenfeld on Mon, Nov 11, 4:58 PM.

Details

Reviewers
prha
Group Reviewers
Restricted Project
Commits
R1:a572b5c7dddf: [terminate] daemon thread approach
Summary

Alright.

So the previous approach of sending SIGINT for termination does not work on Windows. Here is a pretty good article that covers why https://stefan.sofa-rockers.org/2013/08/15/handling-sub-process-hierarchies-python-linux-os-x/

To address this we change the approach to using an Event and then a daemon thread that listens to it then interrupts the main thread if its tripped set.

Test Plan

unit tests
azure pipeline against alangenfeld/windows
manually interrupt the sleepy pipeline on single and multi proc
manually interrupt the log spew on single and multi proc

Diff Detail

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

Event Timeline

alangenfeld created this revision.Mon, Nov 11, 4:58 PM
alangenfeld updated this revision to Diff 6431.Mon, Nov 11, 6:47 PM

dont use signal handler

alangenfeld added inline comments.Mon, Nov 11, 6:50 PM
python_modules/dagster/dagster/core/engine/engine_multiprocess.py
89–93

note: moved away from this approach due to it not working in tox for reasons i could not discern

alangenfeld updated this revision to Diff 6436.Mon, Nov 11, 7:31 PM

fixes for py2

prha accepted this revision.Tue, Nov 12, 4:27 PM
prha added a subscriber: prha.

this looks great... really easy to follow what's going on!

This ran on the azure pipeline? (It's not clear from the github branch history on alangenfeld/windows)

intensenod

This revision is now accepted and ready to land.Tue, Nov 12, 4:27 PM

queued up azure again here - https://dev.azure.com/elementl/dagster/_build/results?buildId=5453, looks like i didnt force push after my py2 fixes so the previous azure run failed on 2.7

prha added a comment.Tue, Nov 12, 5:00 PM

looks good!

alangenfeld updated this revision to Diff 6451.Tue, Nov 12, 5:19 PM

add extra catch - yield an engine event

prha accepted this revision.Tue, Nov 12, 5:22 PM
This revision was automatically updated to reflect the committed changes.