-
Notifications
You must be signed in to change notification settings - Fork 240
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Problem: memiavl background snapshot rewriting panic when shutdown #1292
Problem: memiavl background snapshot rewriting panic when shutdown #1292
Conversation
Solution: - gracefully cancel the task when shutdown
Warning Rate Limit Exceeded@yihuang has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 5 minutes and 36 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. WalkthroughThe codebase has been updated to integrate context-based cancellation into various operations, primarily focusing on snapshot rewriting and import processes. The Changes
Poem
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 Configration File (
|
Signed-off-by: yihuang <huang@crypto.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files selected for processing (5)
- CHANGELOG.md (1 hunks)
- memiavl/db.go (8 hunks)
- memiavl/import.go (2 hunks)
- memiavl/multitree.go (2 hunks)
- memiavl/snapshot.go (8 hunks)
Additional comments: 10
memiavl/multitree.go (1)
- 360-360: The
WriteSnapshot
method now correctly takes acontext.Context
parameter, which is used to control the flow of the snapshot writing process. This is a good practice for managing long-running operations and allows for cancellation and timeout control.memiavl/snapshot.go (4)
355-356: The
WriteSnapshot
function has been correctly updated to accept acontext.Context
parameter, which is used to manage the cancellation and timeouts of the snapshot writing process.369-369: The
writeSnapshot
function has been updated to take acontext.Context
parameter, which is a good practice for managing cancellation and timeouts during the snapshot writing process.468-470: The
snapshotWriter
struct has been correctly augmented with actx
field to store the context object, which is used for cancellation checks in thewriteLeaf
method.514-520: The
writeLeaf
method includes a cancellation check based on the context everyCancelCheckInterval
leaves written, which is a good practice for long-running operations to respond to cancellation requests.memiavl/db.go (5)
54-56: The addition of the
snapshotRewriteCancel
field to theDB
struct is a good implementation for handling graceful shutdowns. This field will be used to cancel the snapshot rewrite operation when the database is closed.421-421: The
snapshotRewriteCancel
is set tonil
after the snapshot rewrite goroutine completes. This is a good practice as it helps avoid potential memory leaks by allowing the garbage collector to reclaim the context resources.633-639: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [636-647]
The
RewriteSnapshot
method now accepts acontext.Context
parameter, which is used to support cancellation of the snapshot rewrite operation. This change is crucial for the feature implemented in this PR and aligns with the objective of preventing panics during shutdown.
715-719: The creation of a new context with a cancellation function in the
rewriteSnapshotBackground
method is a key part of the solution to the panic issue. This context is used to control the lifecycle of the snapshot rewrite goroutine.765-777: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [757-773]
The
Close
method has been updated to cancel the snapshot rewrite goroutine if it's running. This is done by calling thesnapshotRewriteCancel
function and waiting for the goroutine to finish. This change is essential for ensuring a graceful shutdown and preventing the panic issue that this PR aims to solve.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- CHANGELOG.md (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- CHANGELOG.md
@@ -133,7 +134,7 @@ | |||
return errors.New("version overflows uint32") | |||
} | |||
|
|||
return writeSnapshot(dir, uint32(version), func(w *snapshotWriter) (uint32, error) { | |||
return writeSnapshot(context.Background(), dir, uint32(version), func(w *snapshotWriter) (uint32, error) { |
Check failure
Code scanning / gosec
Potential integer overflow by integer type conversion Error
Signed-off-by: yihuang <huang@crypto.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, is it the fix for #1180 before?
maybe |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #1292 +/- ##
===========================================
+ Coverage 15.98% 35.88% +19.90%
===========================================
Files 80 116 +36
Lines 6201 10698 +4497
===========================================
+ Hits 991 3839 +2848
- Misses 5130 6482 +1352
- Partials 80 377 +297
|
Thanks for fixing this! |
Yes, can't reproduce with smaller IdleTimeout now |
Confirmed this fixed the panic during shutdown |
c2d8b40
Solution:
👮🏻👮🏻👮🏻 !!!! REFERENCE THE PROBLEM YOUR ARE SOLVING IN THE PR TITLE AND DESCRIBE YOUR SOLUTION HERE !!!! DO NOT FORGET !!!! 👮🏻👮🏻👮🏻
PR Checklist:
make
)make test
)go fmt
)golangci-lint run
)go list -json -m all | nancy sleuth
)Thank you for your code, it's appreciated! :)
Summary by CodeRabbit