Handle errors on attach properly #6589
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rhatdan 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 |
openshift-ci-robot
added
the
approved
|
Fixes: #6575 |
|
LGTM |
| err := containers.Attach(ic.ClientCxt, name, detachKeys, bindings.PFalse, bindings.PTrue, input, output, errput, attachReady) | ||
| attachErr <- err | ||
| attachErr = containers.Attach(ic.ClientCxt, name, detachKeys, bindings.PFalse, bindings.PTrue, input, output, errput, attachReady) | ||
| attachReady <- true |
There was a problem hiding this comment.
This looks wrong. attachReady is passed to containers.Attach(...) so I think we're in a race.
There was a problem hiding this comment.
I think we actually don't guarantee that attachReady is set if containers.Attach exits with error. We could make that a guarantee of containers.Attach? Or maybe make attachErr a channel and do a select{} on attachReady, attachErr
There was a problem hiding this comment.
I guess we could set attachReady twice.
Signed-off-by: Daniel J Walsh <[email protected]>
|
@vrothberg @mheon Rewritten using select statement. |
|
LGTM |
|
Changes LGTM, but one of the tests looks to be upset. |
|
/lgtm |
github-actions
bot
added
the
locked - please file new issue/PR
Signed-off-by: Daniel J Walsh [email protected]