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

fix: docker backend should not close 'engine.Tail' result #1616

Merged
merged 2 commits into from
Mar 12, 2023

Conversation

alexmt
Copy link
Contributor

@alexmt alexmt commented Mar 12, 2023

Closes #1615

The error described in #1615 is happening because Tail method of the docker backend closes the instance of io.ReadCloser it returns in defer function. As a result anything that try to read data returned by Tail method eventually will attempt to read from closes reader and get an error:

The fix is just don't close returned reader and let the consumer of Tail method do it. Good thing is that Tail is used only in one place and reader is correctly closed:

defer wg.Done()
logger := r.MakeLogger()
if err := r.logger.Log(step, multipart.New(rc)); err != nil {
logger.Error().Err(err).Msg("process logging failed")
}
_ = rc.Close()

Example of woodpecker exec output using pipeline from #1615 with the fix:

woodpecker exec .woodpecker.yaml
[step1:L0:0s] + echo step1
[step1:L1:0s] step1
[step2:L0:0s] + echo step2
[step2:L1:0s] step2

Signed-off-by: Alexander Matyushentsev <AMatyushentsev@gmail.com>
@anbraten anbraten added bug Something isn't working cli labels Mar 12, 2023
@anbraten anbraten added this to the 1.0.0 milestone Mar 12, 2023
@codecov-commenter
Copy link

codecov-commenter commented Mar 12, 2023

Codecov Report

Patch coverage has no change and project coverage change: -0.05 ⚠️

Comparison is base (a2f226f) 40.64% compared to head (c20c442) 40.59%.

❗ Current head c20c442 differs from pull request most recent head 08e6654. Consider uploading reports for the commit 08e6654 to get more accurate results

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1616      +/-   ##
==========================================
- Coverage   40.64%   40.59%   -0.05%     
==========================================
  Files         169      169              
  Lines       11083    11084       +1     
==========================================
- Hits         4505     4500       -5     
- Misses       6287     6293       +6     
  Partials      291      291              
Impacted Files Coverage Δ
pipeline/backend/docker/docker.go 0.00% <ø> (ø)
server/model/task.go 0.00% <ø> (ø)
server/queue/persistent.go 0.00% <0.00%> (ø)

... and 1 file with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@lafriks lafriks enabled auto-merge (squash) March 12, 2023 14:30
@lafriks lafriks merged commit 5e1171d into woodpecker-ci:master Mar 12, 2023
@6543 6543 added the backport-done indicates that this pull has been backported label Mar 13, 2023
6543 pushed a commit to 6543-forks/woodpecker that referenced this pull request Mar 14, 2023
…-ci#1616)

Closes woodpecker-ci#1615

The error described in
woodpecker-ci#1615 is happening
because `Tail` method of the docker backend closes the instance of
`io.ReadCloser` it returns in `defer` function. As a result anything
that try to read data returned by `Tail` method eventually will attempt
to read from closes reader and get an error:


https://github.com/woodpecker-ci/woodpecker/blob/2171212c5a31bfffc8f882716bdd503c65413eee/pipeline/backend/docker/docker.go#L229

The fix is just don't close returned reader and let the consumer of
`Tail` method do it. Good thing is that `Tail` is used only in one place
and reader is correctly closed:


https://github.com/woodpecker-ci/woodpecker/blob/2171212c5a31bfffc8f882716bdd503c65413eee/pipeline/pipeline.go#L231-L237

Example of `woodpecker exec` output using pipeline from
woodpecker-ci#1615 with the fix:

```
woodpecker exec .woodpecker.yaml
[step1:L0:0s] + echo step1
[step1:L1:0s] step1
[step2:L0:0s] + echo step2
[step2:L1:0s] step2
```

Signed-off-by: Alexander Matyushentsev <AMatyushentsev@gmail.com>
Co-authored-by: Lauris BH <lauris@nix.lv>
@6543
Copy link
Member

6543 commented Mar 14, 2023

backport -> #1620

lafriks added a commit that referenced this pull request Mar 14, 2023
)

Backport #1616

Close #1615

The error described in
#1615 is happening
because `Tail` method of the docker backend closes the instance of
`io.ReadCloser` it returns in `defer` function. As a result anything
that try to read data returned by `Tail` method eventually will attempt
to read from closes reader and get an error:



https://github.com/woodpecker-ci/woodpecker/blob/2171212c5a31bfffc8f882716bdd503c65413eee/pipeline/backend/docker/docker.go#L229

The fix is just don't close returned reader and let the consumer of
`Tail` method do it. Good thing is that `Tail` is used only in one place
and reader is correctly closed:



https://github.com/woodpecker-ci/woodpecker/blob/2171212c5a31bfffc8f882716bdd503c65413eee/pipeline/pipeline.go#L231-L237

Example of `woodpecker exec` output using pipeline from
#1615 with the fix:

```
woodpecker exec .woodpecker.yaml
[step1:L0:0s] + echo step1
[step1:L1:0s] step1
[step2:L0:0s] + echo step2
[step2:L1:0s] step2
```

Signed-off-by: Alexander Matyushentsev <AMatyushentsev@gmail.com>
Co-authored-by: Alexander Matyushentsev <AMatyushentsev@gmail.com>
Co-authored-by: Lauris BH <lauris@nix.lv>
@6543 6543 mentioned this pull request Mar 16, 2023
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-done indicates that this pull has been backported bug Something isn't working cli
Projects
None yet
Development

Successfully merging this pull request may close these issues.

woodpecker-cli exec .woodpecker.yaml: "io: read/write on closed pipe"
5 participants