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

Cancellation on Windows: Pass /T to taskkill #795

Merged
merged 1 commit into from
Jul 15, 2018
Merged

Cancellation on Windows: Pass /T to taskkill #795

merged 1 commit into from
Jul 15, 2018

Conversation

amitsaha
Copy link
Contributor

@amitsaha amitsaha commented Jul 13, 2018

/T will make taskkill kill all the child processes as well. This should improve the cancellation behavior. (#794)

Please let me know if I should file a separate issue.

`/T` will make `taskkill` kill all the child processes as well.  This should improve the cancellation behavior. (#794)
@lox
Copy link
Contributor

lox commented Jul 15, 2018

This seems sensible. I'll do some testing on Windows.

@lox
Copy link
Contributor

lox commented Jul 15, 2018

Some interesting context and alternative approaches in golang/dep#862.

@lox lox merged commit c964524 into buildkite:master Jul 15, 2018
@amitsaha
Copy link
Contributor Author

thanks for the merge @lox

@amitsaha
Copy link
Contributor Author

Some interesting context and alternative approaches in golang/dep#862.

Very useful reference. Any objections to incorporating TerminateTree() for the cancellation as implemented here?

@lox
Copy link
Contributor

lox commented Jul 16, 2018

@amitsaha I didn't see any huge upside in TerminateTree() over shelling out to taskkill, did you?

@amitsaha
Copy link
Contributor Author

@lox It looks like TerminateTree would kill the entire tree i.e. including processes spawned by child processes of the parent process? (whereas, it seems taskkill /T would kill only the immediate child processes)? (I haven't verified the taskkill /T behavior, so could be wrong)

@lox
Copy link
Contributor

lox commented Jul 16, 2018

Ah right, in that case I'd absolutely support TerminateTree() being used, I assumed the /T would have the same behaviour.

@amitsaha
Copy link
Contributor Author

I will have to look into this more, but it seems taskkill /T does also terminate child processes spawned by child processes.

@amitsaha
Copy link
Contributor Author

indeed, that's the case, so, we don't need TerminateTree. Some demo code: https://github.com/amitsaha/golang-powershell-task-kill-demo

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

Successfully merging this pull request may close these issues.

2 participants