Do you all generally prefer the nJobs for loop that currently exists in master, or the 2 "done" booleans with the break statement?

Actually, scratch that, since we have to wait for stdout and stderr to finish before we can close stdin

@ncdc I'm slightly in favor of keeping the nJobs loop but like the removal of stdinCloser very much if it doesn't change any behavior(I can't see how it could). There's some added confusion with the name stdinCloser as it has almost nothing to do with process stdin. To rephrase your last comment: we have to wait for stdout and stderr to finish before we can close the attached stream

ping @creack as it seems to be introduced in #329

@tonistiigi how's this for keeping nJobs? https://github.com/ncdc/docker/compare/3631-stdout-premature-eof-njobs

Oh, that won't work as-is, because if you do

docker run --rm -it fedora bash
exit

it will hang waiting for the stdin job to complete. If you hit enter, it will finish and return you to the host's shell.

I think we need to finish attach when either all 3 jobs have finished, or both stdout and stderr have finished, whichever happens first. Something more like this: https://github.com/ncdc/docker/compare/3631-stdout-premature-eof-njobs-2

@ncdc Can you explain the bash regression you were having with the previous version a bit more. I don't see any differences compared to v1.3.0 myself.

edit: also, just to be clear, what branch were you talking about?

@tonistiigi that was just from me fiddling around with nJobs. If all you do is get rid of the stdinCloser logic, the stdin job won't complete until the client closes its end of the stream, which happens when you hit enter after attempting to exit from the shell. If the client opens stdin, the engine needs to close its half of the stdin stream when stdout and stderr finish.

@tonistiigi and for clarity, the "regression" was in my branch

@ncdc ok, I though this was in 3631-stdout-premature-eof not 3631-stdout-premature-eof-njobs

Looking at 3631-stdout-premature-eof-njobs you have also removed stdin.Close() calls, not only stdinCloser(). This should cause the bash thing. I also don't understand the added cStdin.Close() in the end.

@ncdc Would you mind to post your branch here?

@LK4D4 I'm currently attempting to write an integration test that fails with master but is fixed with my branch. I also think @tonistiigi might be on to something simpler - we maybe can just remove the stdinCloser concept altogether. I'll update this branch once things are closer to ready. Sound ok?

Okay, keep us updated!

@LK4D4 this is the best I've been able to come up with so far for an integration test. I was hoping to imitate TestAttachMultipleAndRestart but for some reason the StdoutPipe would always get all the data.

func TestAttachSlowConsumer(t *testing.T) {                                         
  defer deleteAllContainers()                                                        

  c := exec.Command("/bin/bash", "-c", "echo | "+dockerBinary+` run --rm -i busybox /bin/sh -c "dd if=/dev/zero of=/foo bs=1024 count=2000 &>/dev/null; catv /foo" | /bin/bash -c "sleep 2; wc -c"`)

  output, err := c.Output()                                                          
  if err != nil {
    t.Fatal(err)
  } 

  // assert that output == 2*1024*2000

  logDone("attach - slow consumer")                                                 
} 

I remember that @tibor wrote something cool with tty in tests. Maybe he can help.

I obviously don't like the sleep 2 in the test, but I don't currently know of a way to wait the exact amount of time required for the output stream to be closed prematurely, as the daemon is the only thing that's aware of that happening, and the attach job essentially swallows the error and returns an ok status regardless.

Also, once this issue is fixed, we won't exactly be able to wait for a "stream closed" error...

@LK4D4 @tonistiigi PR updated, please see latest diffs.

@ncdc LGTM

Very good indeed!
LGTM

@LK4D4 do we need other reviews so this can hopefully be merged in?

LGTM