Skip to content
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

Wait to finish reading logs before calling Wait() on pipeline #1010

Merged
merged 1 commit into from
Jul 31, 2022

Conversation

thestr4ng3r
Copy link
Contributor

This fixes errors like the following and chopped off
logs with the local backend:

{"level":"error","error":"read |0: file already closed","time":"2022-07-04T17:42:29+02:00","message":"copy limited logStream part"}

The reason for this issue is that exec's Wait(), which is used
internally by the this backend, closes the stdout/stderr pipe, so we
must wait until all reading is done before calling it.

See https://pkg.go.dev/os/exec?utm_source=gopls#Cmd.StdoutPipe:

Wait will close the pipe after seeing the command exit, so most
callers need not close the pipe themselves. It is thus incorrect
to call Wait before all reads from the pipe have completed.

Instead of using a WaitGroup, one could call the reading synchronously,
but with the step.Detached case probably still needing it as a goroutine,
it might become less readable.

An alternative solution could be to move this synchronization into the local backend
implementation itself, for example by constructing a new io.Pipe(), copying everything
into it and waiting for this to be done before calling the exec Wait().
But I think it might be better in general to wait until all log has been read before considering
a pipeline finished.

I have also sometimes encountered errors like these:

{"level":"error","error":"read |0: file already closed","time":"2022-07-04T17:42:29+02:00","message":"copy limited logStream part"}
{"level":"error","error":"rpc error: code = Unknown desc = database is locked","time":"2022-07-04T17:42:29+02:00","message":"grpc error: upload(): code: Unknown: rpc error: code = Unknown desc = database is locked"}
{"level":"error","repo":"thestr4ng3r/ci-test","build":"23","id":"178","image":"/bin/bash","stage":"build","error":"rpc error: code = Unknown desc = database is locked","time":"2022-07-04T17:42:29+02:00","message":"log stream upload error"}

Maybe it is related. In those cases, I did not get any logs at all in the webui. After the fix, I have not seen those anymore, but they were sporadic before too.

This fixes errors like "read |0: file already closed" and chopped off
logs with the local backend.
The reason for this issue is that exec's Wait(), which is used
internally by the this backend, closes the stdout/stderr pipe, so we
must wait until all reading is done before calling it.

See https://pkg.go.dev/os/exec?utm_source=gopls#Cmd.StdoutPipe:
> Wait will close the pipe after seeing the command exit, so most
> callers need not close the pipe themselves. It is thus incorrect
> to call Wait before all reads from the pipe have completed.
@6543 6543 added bug Something isn't working server labels Jul 4, 2022
@6543 6543 added this to the 1.0.0 milestone Jul 31, 2022
@6543 6543 merged commit 7031904 into woodpecker-ci:master Jul 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working server
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants