Fix closing stdin

[rumpl]

moby tried updating hcsshim to 0.11.0 and we had some failures in our tests (see this PR).

We tracked the issue to this change, the close request wasn't forwarded any more if stdin is nil. This change moves the nil check to after the modify request, letting the process handle it.

[neersighted]

We'd also like to backport this onto the 0.11 branch (and ideally tag a release) so we can pull this into moby/moby.

[TBBle]

A nit pick, but I'd suggest flipping the contents of the if statement so it matches the other two functions.

Suggested change

	if process.stdin == nil {
	return nil
	}
	process.stdin.Close()
	process.stdin = nil
	if process.stdin != nil {
	process.stdin.Close()
	process.stdin = nil
	}

[rumpl]

Done! Thanks

[TBBle]

I'm separately curious why we don't (or don't need to) send HcsModifyProcess for the other stdio handles. Is it just that moby never calls it, so we haven't actually noticed that the contained process would not be notified for those handles? A quick look suggests the containerd shim doesn't call CloseStdout or CloseStderr either.

[helsaawy]

I'm separately curious why we don't (or don't need to) send HcsModifyProcess for the other stdio handles. Is it just that moby never calls it, so we haven't actually noticed that the contained process would not be notified for those handles? A quick look suggests the containerd shim doesn't call CloseStdout or CloseStderr either.

I think since closing IO is (almost?) always followed closely by a Close which brings down the whole process (and therefore the stdout/err handles), its not needed, and we really only to close both sides of the stdin pipe in case the process is using that as its queue to stay alive?
Plus, I think the close stdout/err methods were added as signals to the upstream pipes that io is finished, rather than signals to the process that it should stop outputting things.

[helsaawy]

@rumpl
I'm curious how you saw this situation arise, since if process.stdin is nil, then the underlying process shouldn't have a stdin handle to close.

[rumpl]

@helsaawy We had this test failing.

It's running cat | echo "something", then we close stdin which makes cat exit and we should then see the output from echo. Without sending the close message cat would never receive an EOF and would never end.

[rumpl]

Thanks for the merge!

What can we do to help get this backported to 0.11 and then get a new release

[neersighted]

Just noticed https://github.com/moby/moby/blob/20f96354699d20eeb4674d3944486b5d73c256b2/integration/container/exec_test.go#L19 -- is this potentially the missing CloseStdout in runhcs?

[neersighted]

Backport is open at #1900

[rumpl]

Just noticed https://github.com/moby/moby/blob/20f96354699d20eeb4674d3944486b5d73c256b2/integration/container/exec_test.go#L19 -- is this potentially the missing CloseStdout in runhcs?

We should definitely check if it fails still. A lot has happened in 2 years

[TBBle]

Just noticed https://github.com/moby/moby/blob/20f96354699d20eeb4674d3944486b5d73c256b2/integration/container/exec_test.go#L19 -- is this potentially the missing CloseStdout in runhcs?

We should definitely check if it fails still. A lot has happened in 2 years

From memory there was some hcsshim/containerd io-hang issue like this that was never resolved. And last time this test came up, I said the same thing: moby/moby#41479 (comment)

I thought there had been some discussion in a containerd issue of a hcsshim-side fix being needed, but I can't actually find any evidence of the issue I'm thinking of in the hcsshim, containerd, or moby repos. >_< #1296 is the closest I can find, and I'm not sure it's related because it seems to be about spurious errors, not hangs.

I will note that TestExecWithCloseStdin is excluded on Windows/containerd test runs, but is also known-flakey (times out) for Windows non-containerd test runs. So it really just might be a "takes too long" case.