fix(store/v2): using defer to ensure close db handle
#20871
Conversation
WalkthroughWalkthroughThe Changes
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 as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
![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
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Commits
Files that changed from the base of the PR and between 51d33ad and eb525ed3730fa762ec0bacdafa9952ce190fba2e.
Files selected for processing (1)
- store/v2/commitment/metadata.go (2 hunks)
Additional context used
Path-based instructions (1)
store/v2/commitment/metadata.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Additional comments not posted (2)
store/v2/commitment/metadata.go (2)
69-69: Good use ofdeferfor resource management.The addition of
defer batch.Close()ensures that the batch is closed properly, even if an error occurs during the operations. This is a good practice for resource management and enhances the robustness of the code.
91-91: Returnnilupon success.Returning
nilupon a successful write operation is a good practice. It clearly indicates that the operation completed without errors.
There was a problem hiding this comment.
This change as is is now incorrect because the caller won't ever know if batch.Close() succeeded and that's critical. I have prescribed the proper fix down below /cc @tac0turtle @julienrbrt
| @@ -87,7 +88,7 @@ func (m *MetadataStore) flushCommitInfo(version uint64, cInfo *proof.CommitInfo) | |||
| if err := batch.WriteSync(); err != nil { | |||
| return err | |||
| } | |||
| return batch.Close() | |||
| return nil | |||
There was a problem hiding this comment.
This change is no longer correct though. The proper fix when you introduce defer would be to use a named return variable for the error and then in the defer check if the return error was already nil or not then set it so
func (m *MetadataStore) flushCommitInfo(version uint64, cInfo *proof.CommitInfo) (err error) {
defer func() {
cErr := batch.Close()
if err == nil {
err = cErr
}
}()
...
return nil
}There was a problem hiding this comment.
Yep, we should change to this if we care about the error of batch.Close.
BTW, if we care about the cErr, I think it's better to wrap it with err(if err is not nil)?
There was a problem hiding this comment.
nice catch.
we should retain the close error. @odeke-em's approach is correct. happy to approve once implemented.
yukionfire
force-pushed
the
fix/ensure-close-db
branch
from
5d44a4f to
e00eb86
Compare
|
@kocubinski Hi sir, suggestion changes done |
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
store/v2/commitment/metadata.go (1)
62-96: Review offlushCommitInfoMethod1. Error Handling and Resource Management:
The use of a named return variable (err) and thedeferblock for closing the batch operation is appropriate. This ensures that the batch is always closed, even if an error occurs during the method execution, which aligns with the PR's objective.2. Code Logic:
- The method checks if
cInfois null and returns early if it is, which is sensible for an empty or initializing store.- The batch operation is created and deferred closure is set up to handle errors from the
batch.Close()method. This is crucial for proper resource management.- The method handles setting values in the batch and writes them synchronously, ensuring data integrity.
3. Error Handling in
defer:
Thedeferblock correctly checks iferris nil before setting it tocErr. This prevents overwriting of more significant errors by the error frombatch.Close(), which is a good practice. However, there might be a need to consider how errors frombatch.Close()are handled iferris not nil.4. Uber Golang Style Guide Compliance:
- Proper error handling is used.
- Naming conventions are followed.
- The code is concise and well-organized.
5. Potential Improvements:
- Consider logging or wrapping the error from
batch.Close()iferris not nil, to ensure that it does not go unnoticed.Overall, the changes are well-implemented and improve the robustness and clarity of the function. The method now properly handles resource cleanup and error propagation, which are critical for database operations.
Consider enhancing error visibility when
batch.Close()fails but another error is already present. This could be achieved by logging or wrapping the error.
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Commits
Files that changed from the base of the PR and between eb525ed3730fa762ec0bacdafa9952ce190fba2e and e00eb862d020524fc9a6b56f7a544d71a0b04d5e.
Files selected for processing (1)
- store/v2/commitment/metadata.go (1 hunks)
Additional context used
Path-based instructions (1)
store/v2/commitment/metadata.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
There was a problem hiding this comment.
Hey @yukionfire, thanks for the update but you didn't have to change err := to err = and then skipping the returns, no need for that edit at all since named returns handle that already, please simply just ensure your diff contains the named return (err error) and then just the defer
|
@odeke-em Hi sir, suggestion changes addressed |
yukionfire
force-pushed
the
fix/ensure-close-db
branch
from
8872c3c to
f27eaaf
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Commits
Files that changed from the base of the PR and between e00eb862d020524fc9a6b56f7a544d71a0b04d5e and f27eaaf.
Files selected for processing (1)
- store/v2/commitment/metadata.go (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- store/v2/commitment/metadata.go
batchwill miss to close if oprations ofbatchencount error, this PR usesdeferto avoid thisSummary by CodeRabbit