fix(store/v2): chunk.Close is called twice
#23168
Conversation
lfz941
requested review from
cool-develope,
kocubinski and
a team
as code owners
📝 WalkthroughWalkthroughThe pull request introduces a modification to the Changes
Sequence DiagramsequenceDiagram
participant Loader as Load Method
participant Goroutine as Chunk Processing Goroutine
participant Writer as Output Writer
Loader->>Goroutine: Start chunk processing
Goroutine->>Goroutine: Wrap chunk handling in anonymous function
Goroutine-->>Writer: Copy chunk data
Goroutine->>Goroutine: Defer chunk close
Goroutine--xGoroutine: Ensure proper resource cleanup
The sequence diagram illustrates the updated approach of handling chunk processing, emphasizing the isolated resource management within the goroutine's context. 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)
175-183: Consider handling chunk.Close() errorsCurrently, the code defers
chunk.Close()without checking the returned error. While such errors are uncommon, logging them can help diagnose underlying file or I/O issues.func() { - defer chunk.Close() + defer func() { + if err := chunk.Close(); err != nil { + // log or handle the error appropriately + } + }() _, err = io.Copy(pw, chunk) ... }()
📜 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.
🔇 Additional comments (1)
store/v2/snapshots/store.go (1)
175-183: Better resource managementWrapping the chunk copying logic inside an anonymous function with
defer chunk.Close()ensures that each chunk is closed exactly once within its own concise scope. This eliminates the previous risk of callingchunk.Close()multiple times and improves clarity around resource management.
| } | ||
| chunk.Close() | ||
| pw.Close() | ||
| func() { |
There was a problem hiding this comment.
Why do we scope it within an anonymous function instead of just removing the defer? Given that it is called at 181 anyway.
There was a problem hiding this comment.
If we removing the defer, I think we should call chunk.Close() when io.Copy returns error.
That means code will be changed to:
// defer chunk.Close() // remove this line
_, err = io.Copy(pw, chunk)
if err != nil {
chunk.Close()
_ = pw.CloseWithError(err)
return
}
chunk.Close()
pw.Close()I think both of them are equal, wraped to anonymous function or using above style, both are fine
There was a problem hiding this comment.
I see yeah, let's merge, thanks!
Similar to #23146
Summary by CodeRabbit