run: correctly handle only STDIN attached

[laurazard]

- What I did

Overall make the docker run -a stdin handling more consistent + some assorted cleanups.

The most significant changes are making the "print container ID and immediately exit if detached" check more consistent (also account for -a stdin instead of just stdout and stderr), and making docker run -a stdin exit if the container exits/is removed.

- How I did it

aee9eeb:

During a docker run, the CLI has some different behavior/output depending on whether the run is "detached" or not.

In some cases, the CLI is checking whether either stdin, stdout or stderr are attached, but in other cases we're only checking stdout and stderr, which leads to some inconsistencies:

$ docker run -a stdout --rm --name test alpine top
[docker kill test]
exit status 137

$ docker run -a stderr --rm --name test alpine top
[docker kill test]
exit status 137

$ docker run -a stdin --rm --name test alpine top
56820d94a89b96889478241ae68920323332c6d4cf9b51ba9340cba01e9e0565
[docker kill test]
[no exit code]

Since we're not checking for whether stdin is attached when deciding whether to early exit without receiving on statusChan, the docker run -a stdin is falling into the "detached mode" logic, which simply prints the container ID and doesn't print/return the exit code.

This patch makes the "attached" checks consistent.

8431298: small cleanup

446f36c: Since everything else after the apiClient.ContainerStart block is under an if attach conditional, we can move the "not attached" early exit up.

a3d9fc4:

Now, if running in "detached" mode, we early exit at L222.

Similarly, if attachContainer errors out, it returns an error that gets handled on L190.

As such, errCh can never be nil on L231. Remove the nil check.

a8650d8:

If STDOUT or STDERR are attached and the container exits, the streams will be closed by the daemon while the container is exiting, causing the streamer to return an error

cli/cli/command/container/hijack.go

Line 53 in 61b02e6

return err

that gets sent

cli/cli/command/container/run.go

Line 278 in 61b02e6

ch <- func() error {

and received

cli/cli/command/container/run.go

Line 225 in 61b02e6

if err := <-errCh; err != nil {

on errCh.

However, if only STDIN is attached, it's not closed (since this is attached to the user's TTY) when the container exits, so the streamer doesn't exit and nothing gets sent on errCh, meaning the CLI execution hangs receiving on errCh on L231.

Change the logic to receive on both errCh and statusChan – this way, if the container exits, we get notified on statusChan (even if only STDIN is attached), and can cancel the streamer and exit.

bae9b98: cleaned up a comment on another test

- How to verify it

$ docker run -a stdin --rm --name rborkrr alpine sh -c 'sleep 2 && exit 7'

Check that it doesn't hang, and that the container ID is not printed (like with -a stdout/stderr), but exit status 7 is.

Also, run the added e2e test:

TESTFLAGS="-test.run=TestRunAttach" make -f docker.Makefile test-e2e-non-experimental

- Description for the changelog

- Fix issue causing output of `docker run` to be inconsistent when using `--attach stdout or `--attach stderr` vs `stdin`.
- `docker run --attach stdin` now exits if the container exits.

- A picture of a cute animal (not mandatory but encouraged)

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 73.91304% with 6 lines in your changes missing coverage. Please review.

Project coverage is 59.51%. Comparing base (5afa739) to head (7dab597).
Report is 31 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5662      +/-   ##
==========================================
- Coverage   59.53%   59.51%   -0.03%     
==========================================
  Files         346      346              
  Lines       29356    29376      +20     
==========================================
+ Hits        17477    17483       +6     
- Misses      10909    10923      +14     
  Partials      970      970              

[laurazard]

@thaJeztah can you TAL? I can squash some of the commits later.

[thaJeztah]

👀 Looking; thanks!

[thaJeztah]

thanks! overall this seems to make sense; could use a second pair of eyes to see if I didn't miss anything.

Also left some ramblings/comments wondering if this would become cleaner if we'd split these flows (attached <--> detached)

[thaJeztah]

Curious (but perhaps for separate) if we should check if either STDOUT or STDERR is a terminal (for cases where one of them may be redirected). That's mostly with an (unrelated) discussion in the back of my head, where we didn't take that case into account.

[vvoland]

LGTM, left one nit

[thaJeztah]

Looks like these changes landed in the wrong commit (probably you wanted to put them in the commit before this 🙈

[laurazard]

ahhhhhh 🤦‍♀️ that's what I get for trying to do 3 things at the same time.

[laurazard]

@thaJeztah for the other commits, I think the changes are a lot more understandable commit-by-commit, but unsure whether to squash or not before pushing. WDYT?

Thinking about 8431298 + 446f36c + a3d9fc4

[thaJeztah]

@laurazard yup, I went through the commits, and IMO they're fine as they are; each describes what we're doing, so I'm fine keeping them as-is. At most, I was looking at 8431298, but let's go for what we have 👍