Usually by just breaking a section, you see the top, and you see the bottom, and a big space of white in between, not too practical if you want to put it on a sheet. A hidden little icon in the section view however will make this possible, you can. Answers.com is the place to go to get the answers you need and to ask the questions you want.
Join GitHub today
GitHub is home to over 36 million developers working together to host and review code, manage projects, and build software together.
Sign upHave a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
commented Oct 14, 2014
Never close attached stream before both stdout and stderr have written Signed-off-by: Andy Goldstein [email protected] |
referenced this pull request Oct 14, 2014
ClosedFix stdout premature EOF #8381
reviewed Oct 14, 2014
@@ -520,7 +520,7 @@ func (b *Builder) run(c *daemon.Container) error { | |
// FIXME (LK4D4): Also, maybe makes sense to call 'logs' job, it is like attach | |
// but without hijacking for stdin. Also, with attach there can be race | |
// condition because of some output already was printed before it. | |
return<-b.Daemon.Attach(&c.StreamConfig, c.Config.OpenStdin, c.Config.StdinOnce, c.Config.Tty, nil, nil, b.OutStream, b.ErrStream) | |
return<-b.Daemon.Attach(&c.StreamConfig, c.Config.OpenStdin, c.Config.StdinOnce, c.Config.Tty, nil, b.OutStream, b.ErrStream) |
Oct 14, 2014
@LK4D4 didn't you already change this to use logs?
Oct 14, 2014
reviewed Oct 14, 2014
// Get a reader end of a pipe that is attached as stdout to the container. |
ifp, err:= streamConfig.StdoutPipe(); err != nil { |
stdoutDone = true |
Oct 14, 2014
Setting this shouldn't be required. We are going to error out anyway.
Oct 14, 2014
![Error 8571 Sections Do Not Fit Vertically On The Page Error 8571 Sections Do Not Fit Vertically On The Page](https://ars.els-cdn.com/content/image/3-s2.0-B9780123964601000093-f09-12-9780123964601.jpg)
commented Oct 14, 2014
Do you all generally prefer the nJobs for loop that currently exists in master, or the 2 'done' booleans with the break statement? |
commented Oct 14, 2014
Actually, scratch that, since we have to wait for stdout and stderr to finish before we can close stdin |
force-pushed the ncdc:3631-stdout-premature-eof branch from d188761
to 8fc8703
Oct 14, 2014
commented Oct 15, 2014
@ncdc I'm slightly in favor of keeping the nJobs loop but like the removal of ping @creack as it seems to be introduced in #329 |
commented Oct 15, 2014
@tonistiigi how's this for keeping nJobs? https://github.com/ncdc/docker/compare/3631-stdout-premature-eof-njobs |
commented Oct 15, 2014
Oh, that won't work as-is, because if you do 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 |
reviewed Oct 15, 2014
ifcStderr, err:= streamConfig.StderrPipe(); err != nil { |
log.Errorf('attach: stdout pipe: %s', err) |
} else { |
io.Copy(&ioutils.NopWriter{}, cStderr) |
} |
stderrDone = true |
Oct 15, 2014
This all stuff seems to be in different goroutines, so this code is racy, probably you should use channels for this notifications.
Oct 16, 2014
Oct 21, 2014
How does this look? https://github.com/ncdc/docker/compare/3631-stdout-premature-eof-njobs-2
Oct 21, 2014
Now better, so you want to save nJobs
idea? I think it will be better to just write stdout job finished
or something like this. Maybe in addition to number...
Oct 21, 2014
I'm fine getting rid of nJobs
if everyone is ok with that.
Oct 21, 2014
Oct 21, 2014
How does this look? https://github.com/ncdc/docker/compare/3631-stdout-premature-eof-2
commented Oct 21, 2014
@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 edit: also, just to be clear, what branch were you talking about? |
commented Oct 21, 2014
@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. |
commented Oct 21, 2014
@tonistiigi and for clarity, the 'regression' was in my branch |
commented Oct 21, 2014
@ncdc ok, I though this was in Looking at |
commented Oct 22, 2014
@ncdc Would you mind to post your branch here? |
commented Oct 22, 2014
@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 |
commented Oct 22, 2014
Okay, keep us updated! |
commented Oct 22, 2014
@LK4D4 this is the best I've been able to come up with so far for an integration test. I was hoping to imitate |
commented Oct 22, 2014
I remember that @tibor wrote something cool with |
commented Oct 22, 2014
I obviously don't like the Also, once this issue is fixed, we won't exactly be able to wait for a 'stream closed' error... |
force-pushed the ncdc:3631-stdout-premature-eof branch from 8fc8703
to ab2e935
Oct 22, 2014
commented Oct 22, 2014
@LK4D4@tonistiigi PR updated, please see latest diffs. |
Fix stdout premature EOF
force-pushed the ncdc:3631-stdout-premature-eof branch from ab2e935
to 5572dbb
Oct 22, 2014
referenced this pull request Oct 22, 2014
Mergedarchive: preserve hardlinks in Tar and Untar #8046
commented Oct 22, 2014
@ncdc LGTM |
commented Oct 23, 2014
Very good indeed! |
commented Oct 29, 2014
@LK4D4 do we need other reviews so this can hopefully be merged in? |
commented Oct 29, 2014
LGTM |
pushed a commit that referenced this pull request Oct 29, 2014
merged commit f936a10
into moby:masterOct 29, 2014
1 check passed
referenced this pull request Oct 29, 2014
Closedpanic: runtime error: invalid memory address or nil pointer dereference #8832
referenced this pull request Oct 30, 2014
MergedFix panic on slow log consumer. #8866
referenced this pull request Nov 5, 2014
ClosedLearning to use Pulls tool, just ignore #8967
referenced this pull request Nov 6, 2014
Closed[DOC] post-1.3.1-docs-update-1 #8991
referenced this pull request in jessfraz/docker Nov 26, 2014
Merged commit of the following: