fix(storev2/snapshots): Close StateChanges channel in doRestoreSnapshot
#21106
Conversation
WalkthroughWalkthroughThe recent changes involve modifications to channel management in the 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 (
|
|
@hieuvubk your pull request is missing a changelog! |
![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
Files selected for processing (2)
- store/v2/commitment/store.go (1 hunks)
- store/v2/snapshots/manager.go (1 hunks)
Additional context used
Path-based instructions (2)
store/v2/commitment/store.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.store/v2/snapshots/manager.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Additional comments not posted (1)
store/v2/commitment/store.go (1)
317-318: Ensure channel usage after closure is safe.The
defer close(chStorage)statement ensures that the channel is closed when the function exits, preventing resource leaks. Ensure that the channelchStorageis not used after it is closed to avoid potential panics.
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (2)
- store/v2/commitment/store.go (1 hunks)
- store/v2/snapshots/manager.go (2 hunks)
Files skipped from review due to trivial changes (1)
- store/v2/commitment/store.go
Additional context used
Path-based instructions (1)
store/v2/snapshots/manager.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
store/v2/snapshots/manager.go
Outdated
| if commitmentErr != nil { | ||
| return commitmentErr |
There was a problem hiding this comment.
Synchronize error handling with the new goroutine.
The current check for commitmentErr will not work correctly because the goroutine might not have finished executing. Use a channel to synchronize error handling.
- if commitmentErr != nil {
- return commitmentErr
+ if err := <-commitmentErrCh; err != nil {
+ return err
}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if commitmentErr != nil { | |
| return commitmentErr | |
| if err := <-commitmentErrCh; err != nil { | |
| return err |
store/v2/snapshots/manager.go
Outdated
| var commitmentErr error | ||
| go func() { | ||
| defer close(chStorage) | ||
| nextItem, err = m.commitSnapshotter.Restore(snapshot.Height, snapshot.Format, streamReader, chStorage) | ||
| if err != nil { | ||
| commitmentErr = err | ||
| } | ||
| }() | ||
|
|
There was a problem hiding this comment.
Ensure proper error handling and resource management.
The new goroutine introduced for handling the restoration process captures errors in commitmentErr. However, commitmentErr is checked immediately after the goroutine is started, which might not capture errors correctly. Consider using a channel to synchronize error handling.
- var commitmentErr error
- go func() {
- defer close(chStorage)
- nextItem, err = m.commitSnapshotter.Restore(snapshot.Height, snapshot.Format, streamReader, chStorage)
- if err != nil {
- commitmentErr = err
- }
- }()
+ commitmentErrCh := make(chan error, 1)
+ go func() {
+ defer close(chStorage)
+ _, err := m.commitSnapshotter.Restore(snapshot.Height, snapshot.Format, streamReader, chStorage)
+ commitmentErrCh <- err
+ }()Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var commitmentErr error | |
| go func() { | |
| defer close(chStorage) | |
| nextItem, err = m.commitSnapshotter.Restore(snapshot.Height, snapshot.Format, streamReader, chStorage) | |
| if err != nil { | |
| commitmentErr = err | |
| } | |
| }() | |
| var commitmentErrCh = make(chan error, 1) | |
| go func() { | |
| defer close(chStorage) | |
| nextItem, err = m.commitSnapshotter.Restore(snapshot.Height, snapshot.Format, streamReader, chStorage) | |
| commitmentErrCh <- err | |
| }() |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- store/v2/snapshots/manager.go (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- store/v2/snapshots/manager.go
| } | ||
| close(chStorage) |
There was a problem hiding this comment.
Lets add a comment so it doesn't get linted away in a future refactor
There was a problem hiding this comment.
I don't fully follow which problem is fixed here? could you please add some test cases?
There was a problem hiding this comment.
It's hard to present it as a unit test, but u can try this scripts
My understanding is in storage.Restore we loop through the channel:
for kvPair := range chStorage {This loop never ends until the channel is closed.
chStorage := make(chan *corestore.StateChanges, defaultStorageChannelBufferSize)
defer close(chStorage)
storageErrs := make(chan error, 1)
go func() {
defer close(storageErrs)
err := m.storageSnapshotter.Restore(snapshot.Height, chStorage)
if err != nil {
storageErrs <- err
}
}()
nextItem, err = m.commitSnapshotter.Restore(snapshot.Height, snapshot.Format, streamReader, chStorage)
...
// wait for storage snapshotter to complete
if err := <-storageErrs; err != nil {
return errorsmod.Wrap(err, "storage snapshotter")
}With our current implement, close(chStorage) not be called => loop never ends
There was a problem hiding this comment.
This means there are no test cases to check this restoring flow.
There was a problem hiding this comment.
i think its because the mockStorageSnapshotter we use in restore test doesn't do anything, let me update that in my test PR
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- store/v2/snapshots/manager.go (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- store/v2/snapshots/manager.go
| } | ||
| close(chStorage) |
There was a problem hiding this comment.
This means there are no test cases to check this restoring flow.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- store/v2/snapshots/manager.go (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- store/v2/snapshots/manager.go
Description
Closes: #XXXX
We pass state change to a channel in
commitSnapshotter.Restorethen read it instorageSnapshotter.Restore. Seem like the channel does not close, got an infinity loop in storage restore.Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit