New issue
Have 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
Add NewDirectIOWithTerminal #2310
Add NewDirectIOWithTerminal #2310
Conversation
LGTM |
container_linux_test.go
Outdated
|
||
<-status | ||
|
||
out := buf.String() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this needs a direct.Wait() or close to fix the race. You can test locally with go test -c -race
The tests failed with a data race, but not quite sure I understand what is racing though. Restarted but captured the race stack in this gist https://gist.github.com/dmcgowan/674b2dd6c3dfcce3fe54b565f5ce1ba2 |
@dmcgowan io will race if you try to read from the buffer if the process hasn't finished flushing |
Signed-off-by: Evan Hazlett <ejhazlett@gmail.com>
a4bf17b
to
6b4355d
Compare
@dmcgowan thanks for the gist. i've updated to fix the race. thx! |
Codecov Report
@@ Coverage Diff @@
## master #2310 +/- ##
==========================================
- Coverage 45.54% 45.51% -0.03%
==========================================
Files 83 83
Lines 9185 9190 +5
==========================================
Hits 4183 4183
- Misses 4326 4331 +5
Partials 676 676
Continue to review full report at Codecov.
|
LGTM |
Found this PR introduces a bug. Clients which were calling |
This adds
NewDirectIOWithTerminal
as well as a test for PTY (TestContainerPTY
) to test that output has control codes.Refs: #2307