StartExec does not wait for process to finish if stdout/stderr not passed in

[zombiezen]

Calling StartExec (even without Detach: true) will not wait until the process finishes if stdout/stderr are not passed in. IIUC the problem is this branch:

go-dockerclient/client.go

Lines 793 to 794 in fa0591b

	if hijackOptions.stdout == nil && hijackOptions.stderr == nil {
	close(errChanOut)

I believe the fix is to always use ioutil.Discard for stdout/stderr and synchronize on the streams closing.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

[fsouza]

no 😁

[zombiezen]

@fsouza Would you accept a PR to fix this?

[fsouza]

Hi @zombiezen, I think using ioutil.Discard makes sense. PR is welcome!

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

[flowchartsman]

If this fix is acceptable upstream, I can port it here, too. I’ll issue a PR shortly. Just really needed it in doclertest first.

[fsouza]

If this fix is acceptable upstream, I can port it here, too. I’ll issue a PR shortly. Just really needed it in doclertest first.

Sounds good! That change looks good to me

[flowchartsman]

It seems as if the fix for this is a bit more difficult than with its consequences in Dockertest. If you look at my tests, the one involving a sleep fails. I believe this is because the CreateExec step explicitly specifies that no stdin/stdout will be connected, so the Detatch: false is effectively ignored for StartExec. Still verifying this, but there's not a lot of info on the interaction between the /containers/{id}/exec and the /exec/{id}/start routes when it comes to pre-attached streams and detach on run, so it's likely going to involve some digging. I will note that do note that if you change AttachStdout: false, to be true in the getExecID test function, both tests pass, so it seems like that's what's going on.

The test with /bin/false passes, I believe, because, by not closing the error channel immediately you effectively block on latency just long enough for it to complete and report its exit status, but it doesn't solve the underlying problem and is more of a race condition than anything.

Unless I'm missing something, the options to fix this now seem to be:

1. couple Create/Start Exec` and make API change
2. make transparent API call to fetch exec definition on ExecStart and re-create it with at least stdout attached if the user seems to want to wait.
3. Do nothing and warn in the docs
4. Detect that StartExec is being called with nil writers and Detatch: false and transparently enter a loop calling ExecInspect to simulate blocking on the live process.

None are great, but at least option 4 is the least invasive. Thoughts?

[fsouza]

Thanks for the detailed analysis and apologies for the delayed response. I'm leaning towards 3, but I'm also curious how other SDKs are solving this. Do you know?

I may take a loot at it tomorrow to see if we can get some "inspiration" elsewhere :)