Skip to content
This repository was archived by the owner on Feb 8, 2021. It is now read-only.

Conversation

@bergwolf
Copy link
Member

@bergwolf bergwolf commented May 6, 2017

If either stdoutPipe or stderrPipe reads to EOF, do not close the other
one so that it can read to EOF as well. Otherwise we might lose output
in the other pipe.

This works because if runv ends the stdout/stderr streams, it always
closes the writer part of both pipes. Then io.Copy() will get EOF and
return success here.

Also no need to wait stdin go routine because streamCopy() runs in a go
routine itself thus no one is really waiting out there.

Some observations found during looking at io.Pipe() source code:

  1. reading from a write-side closed pipe returns all buffered data and then EOF
  2. reading from a read-side closed pipe returns ErrClosedPipe and no data

So streamCopy() cannot close stdout/stderr pipes from the reader side and expect io.Copy() to still read buffered data from them.

This might fix the failure in http://ci.hypercontainer.io:8080/job/hyperd-auto/404/console

If either stdoutPipe or stderrPipe reads to EOF, do not close the other
one so that it can read to EOF as well. Otherwise we might lose output
in the other pipe.

This works because if hyperstart ends the stdout/stderr streams, it always
closes the writer part of both pipes. Then io.Copy() will get EOF and
return success here.

Also no need to wait stdin go routine because streamCopy() runs in a go
routine itself thus no one is really waiting out there.

Signed-off-by: Peng Tao <bergwolf@gmail.com>
}
}
if tty.Stdin != nil {
wg.Add(1)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why remove this one?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmmm.... having read the commit message, looks reasonable...

@gnawux
Copy link
Member

gnawux commented May 7, 2017

LGTM

1 similar comment
@laijs
Copy link
Contributor

laijs commented May 8, 2017

LGTM

@laijs laijs merged commit 9a99c10 into hyperhq:master May 8, 2017
gnawux added a commit to gnawux/hyper that referenced this pull request May 10, 2017
- hyperhq/runv#498

Signed-off-by: Wang Xu <gnawux@gmail.com>
@bergwolf bergwolf deleted the streamcopy branch August 15, 2017 08:56
jimoosciuc pushed a commit to jimoosciuc/runv that referenced this pull request May 26, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants