libpod: fix race when closing STDIN #11864
Conversation
openshift-ci
bot
added
the
approved
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: flouthoc, Luap99 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Looks like at least one of those |
|
Removing these entirely breaks scenarios where STDIN closes immediately again - Podman will hang forever waiting for input that never comes. It begins to sound like we need a dedicated "Closed" bool somewhere? |
|
How about I keep CloseWrite but do not log the error? |
|
@Luap99 how about we abstract the output from channel into a struct ? and we can use channel to relay error back if any with the actual result and we can process error at caller level Something like type Result struct {
ActualData string
Error error
}
ch := make(chan Result) |
|
@Luap99 Can we catch and ignore duplicate close errors and print the others? |
Luap99
changed the title
|
I pushed a new version which closes STDIN in same goroutine so the race should be gone. |
|
LGTM |
libpod/oci_attach_linux.go
Outdated
| var err error | ||
| select { | ||
| case err = <-receiveStdoutError: | ||
| return err | ||
| case err = <-stdinDone: | ||
| // copy stdin is done, close it |
There was a problem hiding this comment.
Semantics here aren't a precise match, we need to only do the one-sided close if err is nil
There was a problem hiding this comment.
fixed
Also added another fix for podman-remote run where stdin was never closed.
There is a race where `conn.Close()` was called before `conn.CloseWrite()`. In this case `CloseWrite` will fail and an useless error is printed. To fix this we move the the `CloseWrite()` call to the same goroutine to remove the race. This ensures that `CloseWrite()` is called before `Close()` and never afterwards. Also fixed podman-remote run where the STDIN was never was closed. This is causing flakes in CI testing. [NO TESTS NEEDED] Fixes containers#11856 Signed-off-by: Paul Holzinger <[email protected]>
|
LGTM |
|
/lgtm |
github-actions
bot
added
the
locked - please file new issue/PR
What this PR does / why we need it:
There is a race where
conn.Close()was called beforeconn.CloseWrite().In this case
CloseWritewill fail and an useless error is printed. Tofix this we move the the
CloseWrite()call to the same goroutine toremove the race. This ensures that
CloseWrite()is called beforeClose()and never afterwards.This is causing flakes in CI testing.
How to verify it
not possible, CI flake
Which issue(s) this PR fixes:
Fixes #11856
Special notes for your reviewer: