cloud: close Reader before resetting in ResumingReader #75149
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
adityamaru
requested review from
knz,
a team and
dt
and removed request for
a team
|
This change is |
adityamaru
commented
Jan 19, 2022
pkg/cloud/cloud_io.go
Outdated
| @@ -231,6 +231,7 @@ func (r *ResumingReader) Read(p []byte) (int, error) { | |||
| } | |||
| log.Errorf(r.Ctx, "Retry IO: error %s", lastErr) | |||
| lastErr = nil | |||
| r.Reader.Close() | |||
There was a problem hiding this comment.
hmm actually i think we need a nil check here incase the call to r.Open() failed.
knz
approved these changes
Jan 19, 2022
There was a problem hiding this comment.
LGTM modulo the additional fix you've identified already
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @adityamaru and @dt)
pkg/cloud/cloud_io.go, line 234 at r1 (raw file):
Previously, adityamaru (Aditya Maru) wrote…
hmm actually i think we need a nil check here incase the call to
r.Open()failed.
yep
This change `Close()`s the Reader before resetting it when we encounter a resumable error in the ResumingReader. This is particularly important for the http external storage provide, since forgetting to call Close() results in goroutine leaks from go1.17.6 onwards. See: golang/go#50652 Release note: None
adityamaru
force-pushed
the
close-http-on-retry
branch
from
3c4995c
to
bea8127
Compare
|
Bazel failure is an unrelated timeout of kvserver package. bors r=knz |
|
Build succeeded: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This change
Close()s the Reader before resetting it when weencounter a resumable error in the ResumingReader. This is particularly
important for the http external storage provide, since forgetting to
call Close() results in goroutine leaks from go1.17.6 onwards.
See: golang/go#50652
Fixes: #75143
Release note: None