fix: move exec inside loop - #1183
Conversation
508d95f to
1439246
Compare
aims to resolve containernetworking#855 Signed-off-by: QxBytes <39818795+QxBytes@users.noreply.github.com>
1439246 to
fdc67a2
Compare
|
@squeed @LionelJouin this might have been under the radar for awhile |
jellonek
left a comment
jellonek
left a comment
There was a problem hiding this comment.
IMO good idea + nice tests, while implementation - i'm not sure about it...
| // Retry the command on "text file busy" errors | ||
| for i := 0; i <= 5; i++ { | ||
| stdout = &bytes.Buffer{} | ||
| stderr = &bytes.Buffer{} |
There was a problem hiding this comment.
Why you are creating new objects instead of just calling https://pkg.go.dev/bytes#Buffer.Reset at the start of each iteration?
| stdout = &bytes.Buffer{} | ||
| stderr = &bytes.Buffer{} | ||
| c := exec.CommandContext(ctx, pluginPath) | ||
| c.Env = environ |
There was a problem hiding this comment.
is there a possibility that c.Env could change between iterations?
There was a problem hiding this comment.
c.Env would come from environ which I don't expect to change during the lifetime of the call.
| stderr = &bytes.Buffer{} | ||
| c := exec.CommandContext(ctx, pluginPath) | ||
| c.Env = environ | ||
| c.Stdin = bytes.NewBuffer(stdinData) |
There was a problem hiding this comment.
why you are recreating it again and again, while it could be simply resetted?
There was a problem hiding this comment.
If I resetted like stdout or stderr I'd need to write the stdinData back in right? I expect the retries case to be hit very rarely.
Signed-off-by: Alexander <39818795+QxBytes@users.noreply.github.com>
3 participants
Aims to resolve #855
Moves exec command into the for loop so each retry gets a new instance of the command. Previously, we would retry using the same exec command which would always fail due to https://cs.opensource.google/go/go/+/refs/tags/go1.26.2:src/os/exec/exec.go;l=146 (command cannot be reused after calling
Runon it).Reran the unit test several dozen times without any flakiness.