Skip to content

Conversation

@DracoLi
Copy link
Contributor

@DracoLi DracoLi commented Nov 17, 2025

Why this should be merged

This cleans up the cacheDB close logic by removing an unnecessary mutex and using an atomic.Bool instead, which makes the code simpler and cheaper to reason about while preserving the same behavior on close.

This PR is a follow up to this comment in #4425.

How this works

cacheDB now tracks the closed state via an atomic.Bool rather than closeMu. We still rely on the underlying DB’s own synchronization to protect any in-flight disk I/O; the cache itself doesn’t need extra protection at close.
If a Get races with Close, the cache lookup will fail and the underlying DB will return a “db closed” error as expected.

How this was tested

Unit tests

Need to be documented in RELEASES.md?

No, internal implementation detail only

@DracoLi DracoLi marked this pull request as ready for review November 17, 2025 15:56
Copilot AI review requested due to automatic review settings November 17, 2025 15:56
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Refactors the cacheDB implementation to use atomic.Bool instead of a mutex-protected boolean for tracking closed state, simplifying the synchronization logic while maintaining the same thread-safety guarantees.

Key Changes:

  • Replaced sync.RWMutex and boolean field with atomic.Bool for closed state tracking
  • Updated all methods to use atomic operations instead of mutex locks
  • Simplified Close() method to use CompareAndSwap for idempotent close behavior

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@DracoLi DracoLi requested review from a team, joshua-kim and yacovm and removed request for a team November 17, 2025 15:57
@rrazvan1
Copy link
Contributor

If a Get races with Close, the cache lookup will fail and the underlying DB will return a “db closed” error as expected.

But if the cache has that value, the value will be returned, even though the db is closed.
Is this an issue?

@DracoLi
Copy link
Contributor Author

DracoLi commented Nov 19, 2025

But if the cache has that value, the value will be returned, even though the db is closed.
Is this an issue?

Not an issue here.

@yacovm yacovm added this pull request to the merge queue Nov 21, 2025
Merged via the queue into master with commit eb11349 Nov 21, 2025
35 checks passed
@yacovm yacovm deleted the dl/blockdb-cache-no-mutex branch November 21, 2025 16:31
@github-project-automation github-project-automation bot moved this to Done 🎉 in avalanchego Nov 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done 🎉

Development

Successfully merging this pull request may close these issues.

4 participants