Fixed the run output log files to be populated when debug=true #53
Conversation
ashrithagoramane
requested review from
shk3,
dfinninger,
danielfritsch and
siddarthapaladugu
and removed request for
shk3
venv.go
Outdated
| } | ||
| }(stream) | ||
| *o = fmt.Sprint(buff.String()) |
There was a problem hiding this comment.
This step could have concurrency issue and race condition with line 248,249 when the main routine dumps the streams into the output strings.
Have you considered https://pkg.go.dev/io#TeeReader? It might need some plumbing to get it work, but using a buffer to collect output is probably much safer than dealing with strings in a multithread context.
In fact, if I had time, I would consider making CommandOutput.Stderr and CommandOutput.Stdout buffers, so we can write to the output files once a while to avoid empty log files when ansible puller crashed, but this would be for future.
There was a problem hiding this comment.
The real issue is on the string write part on CommandOutput.Stderr and outString. So in the modified implementation, the concurrency issue still exists. Sorry that my original comment on the streamOutput function was a bit misleading.
Actually, reading this part of code more closely, I realize the previous statement about race condition isn't correct. This part actually branches and ends when c.StreamOutput is set, so CommandOutput.Stderr and CommandOutput.Stdout won't be touched by two routines. Sorry for the confusion here caused by the bad readability of the legacy code.
There was a problem hiding this comment.
There is a minor point about ignoring the errors returned by cmd.StdoutPipe() and cmd.StderrPipe(), but this is an existing issue in the code, and I'm fine with leaving it out of the scope of this PR.
ashrithagoramane
changed the title
…put to stdout and the CommandOutput struct
Issue with the logs being output to file when debug flag is set to true.
When the debug=true the StreamOuput code block only streams the output to STDOUT and fails to populate the stdout and stderr field.