Page MenuHomePhabricator

[terminate] daemon thread approach
ClosedPublic

Authored by alangenfeld on Nov 11 2019, 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
Branch
alangenfeld/windows
Lint
Lint OK
Unit
No Unit Test Coverage

Event Timeline

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

dont use signal handler

alangenfeld added inline comments.Nov 11 2019, 6:50 PM
python_modules/dagster/dagster/core/engine/engine_multiprocess.py
83–87

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.Nov 11 2019, 7:31 PM

fixes for py2

prha accepted this revision.Nov 12 2019, 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.Nov 12 2019, 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.Nov 12 2019, 5:00 PM

looks good!

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

add extra catch - yield an engine event

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