fix(store/v2): chunkBody.Close and chunkFile.Close both are called twice
#23271
Conversation
📝 WalkthroughWalkthroughThe pull request modifies the Changes
Possibly related PRs
Suggested reviewers
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
store/v2/snapshots/store.go (1)
314-318: Consider combining errors from both Close operations.While the current implementation is good, consider handling the case where both
chunkBody.Close()andchunkFile.Close()fail. You could useerrors.Jointo combine both errors:func (s *Store) saveChunk(chunkBody io.ReadCloser, index uint32, snapshot *types.Snapshot, chunkHasher, snapshotHasher hash.Hash) (err error) { + var bodyErr, fileErr error defer func() { - if cErr := chunkBody.Close(); cErr != nil { - err = errors.Wrapf(cErr, "failed to close snapshot chunk body %d", index) + if cErr := chunkBody.Close(); cErr != nil { + bodyErr = errors.Wrapf(cErr, "failed to close snapshot chunk body %d", index) } }() // ... existing code ... defer func() { - if cErr := chunkFile.Close(); cErr != nil { - err = errors.Wrapf(cErr, "failed to close snapshot chunk file %d", index) + if cErr := chunkFile.Close(); cErr != nil { + fileErr = errors.Wrapf(cErr, "failed to close snapshot chunk file %d", index) } + if bodyErr != nil || fileErr != nil { + err = errors.Join(bodyErr, fileErr) + } }()
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
store/v2/snapshots/store.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
store/v2/snapshots/store.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: tests (03)
- GitHub Check: tests (02)
- GitHub Check: tests (01)
- GitHub Check: tests (00)
- GitHub Check: test-simapp-v2
- GitHub Check: test-integration
- GitHub Check: test-system-v2
- GitHub Check: build (arm64)
- GitHub Check: build (amd64)
- GitHub Check: Analyze
- GitHub Check: golangci-lint
- GitHub Check: Summary
🔇 Additional comments (2)
store/v2/snapshots/store.go (2)
301-301: LGTM! Good use of named return parameter.The named return parameter
erris well-suited for the deferred error handling pattern being implemented.
302-306: LGTM! Well-structured error handling for resource cleanup.The deferred function properly captures and wraps any errors that occur during
chunkBody.Close(), providing good context for debugging.
lfz941
force-pushed
the
fix/duplicate-close
branch
from
f4d880e to
dbfa2d8
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
store/v2/snapshots/store.go (1)
313-318: Consider enhancing error handling for multiple closure failures.While the current implementation is correct, consider preserving both errors when both
chunkBodyandchunkFilefail to close.func (s *Store) saveChunk(chunkBody io.ReadCloser, index uint32, snapshot *types.Snapshot, chunkHasher, snapshotHasher hash.Hash) (err error) { + var chunkBodyErr error defer func() { if cErr := chunkBody.Close(); cErr != nil { - err = errors.Wrapf(cErr, "failed to close snapshot chunk body %d", index) + chunkBodyErr = errors.Wrapf(cErr, "failed to close snapshot chunk body %d", index) + if err == nil { + err = chunkBodyErr + } else { + err = errors.Wrapf(err, "also failed to close chunk body: %v", chunkBodyErr) + } } }()
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
store/v2/snapshots/store.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
store/v2/snapshots/store.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: tests (00)
- GitHub Check: test-simapp-v2
- GitHub Check: test-system-v2
- GitHub Check: Analyze
- GitHub Check: Summary
🔇 Additional comments (2)
store/v2/snapshots/store.go (2)
301-301: LGTM! Good improvement to error handling.The addition of a named return parameter
errenhances readability and makes error propagation more explicit.
302-306: LGTM! Well-structured error handling for resource cleanup.The deferred closure of
chunkBodywith proper error wrapping and contextual information is a good practice.
Summary by CodeRabbit
Bug Fixes
Refactor