Pete Moore [:pmoore][:pete] Assignee
	Description • 6 years ago

In bug 1458873 we learned that calling c.Wait() (for c that is a *os/exec.Cmd) on an aborted command may never return.

The solution was to call c.Process().Wait() instead of c.Wait().

Later in bug 1462293 we realised that we should call c.Wait() after calling c.Process.Wait() if the process is not aborted, to make sure clean up is done.

We then discovered that since c.Wait() calls c.Process.Wait() internally, this results in two calls to c.Process.Wait() which causes a hang. I'll be raising a bug about this against the go standard library since the API docs don't handle this, nor mention it. Interestingly c.Wait() does ensure that it is only called once, but c.Process.Wait() has no such check, not documented caveat.

The cleanest solution to the problem is to kill the entire process tree (which is not supported by the go standard library APIs) but is supported by the taskkill.exe utility which ships with Windows. Since it is non-trivial to implement using win32 apis, I'm going to call taskkill.exe from generic-worker in order to kill the process tree.

This will allow us to return to using c.Wait() instead of c.Process.Wait(), and mean we don't need to treat abort/non-abort cases differently.

	Pete Moore [:pmoore][:pete] Assignee
	Comment 1 • 6 years ago

(In reply to Pete Moore [:pmoore][:pete] from comment #0)

> We then discovered that since c.Wait() calls c.Process.Wait() internally,
> this results in two calls to c.Process.Wait() which causes a hang. I'll be
> raising a bug about this against the go standard library since the API docs
> don't handle this, nor mention it. Interestingly c.Wait() does ensure that
> it is only called once, but c.Process.Wait() has no such check, not
> documented caveat.

See:
  *https://github.com/golang/go/blob/155aefe0c182f3788e44596db5f09cf94d2c6a3e/src/os/exec/exec.go#L458-L460
  * https://github.com/golang/go/blob/adcf2d59ec25e6a1f5fda7eb5c125302363657ea/src/os/exec.go#L124-L126

	[github robot]
	Comment 3 • 6 years ago

Commits pushed to master at https://github.com/taskcluster/generic-worker

https://github.com/taskcluster/generic-worker/commit/325ac117fb11c33eaf143a00d400753f467cba49
Bug 1462369 - kill entire process tree on Windows when aborting a task command

https://github.com/taskcluster/generic-worker/commit/e958eae548457e8e039b420c9c467a92291b5e93
Merge pull request #91 from taskcluster/bug1462369

Bug 1462369 - kill entire process tree on Windows when aborting a task command

	Pete Moore [:pmoore][:pete] Assignee
	Comment 4 • 6 years ago

Released in generic-worker 10.8.2.

	Pete Moore [:pmoore][:pete] Assignee
	Comment 5 • 6 years ago

Not landed in production yet, that will be done in bug 1465113.