fix: PebbleDB and noneSharedKV Close idempotent using sync.Once.#51
Open
fix: PebbleDB and noneSharedKV Close idempotent using sync.Once.#51
sync.Once.#51Conversation
…x hot upgrade process
Contributor
There was a problem hiding this comment.
Pull request overview
This PR aims to make Close() idempotent and thread-safe for the Pebble-backed IndexDB (PebbleDB) and the Pebble-backed SharedKV (noneSharedKV) by guarding shutdown with sync.Once, preventing double-close of the underlying Pebble DB.
Changes:
- Add a
sync.Oncefield (closed) toPebbleDBandnoneSharedKV. - Update both
Close()implementations to execute the underlying PebbleFlush()/Close()only once. - Add
syncimports to support the new behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| storage/sharedkv/nonekv.go | Adds sync.Once guard to make noneSharedKV.Close() run only once. |
| storage/indexdb/pebble/pebble.go | Adds sync.Once guard to make PebbleDB.Close() run only once (including a one-time flush). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
Comment on lines
20
to
+25
| func (r *noneSharedKV) Close() error { | ||
| return r.db.Close() | ||
| var err error | ||
| r.closed.Do(func() { | ||
| err = r.db.Close() | ||
| }) | ||
| return err |
Comment on lines
116
to
+123
| func (p *PebbleDB) Close() error { | ||
| // force flush data to disk | ||
| _ = p.db.Flush() | ||
| return p.db.Close() | ||
| var err error | ||
| p.closed.Do(func() { | ||
| _ = p.db.Flush() | ||
| // force flush data to disk | ||
| err = p.db.Close() | ||
| }) | ||
| return err |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request introduces a thread-safe mechanism to ensure that the
Close()method for both thePebbleDBandnoneSharedKVstorage implementations is executed only once, even if called multiple times. This prevents potential issues such as double-closing the underlying database, which could lead to panics or resource leaks.Thread safety and resource management improvements:
sync.Oncefield namedclosedto both thePebbleDBandnoneSharedKVstructs to guarantee that theClose()operation is performed only once, regardless of how many timesClose()is called. [1] [2] [3] [4]Close()methods in bothpebble.goandnonekv.goto use thesync.Oncefield, ensuring thread safety and preventing multiple close attempts on the same database instance. [1] [2]syncpackage in both files to support the new thread-safe logic. [1] [2]