Skip to content
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

Take lock over Snapshotter state during callback #6882

Merged
merged 5 commits into from
Mar 5, 2025

Conversation

eddyashton
Copy link
Member

Another fix spun out of #6616.

This Snapshotter class has a std::mutex member, which is locked in most of its member functions to prevent concurrent access to its member state. The one function that doesn't try to lock, is snapshot_, the callback that comes via the ThreadMessaging system so is almost guaranteed to race with other calls. I think the reason we didn't lock there is that locking for the whole function causes a deadlock (this callback tries to .commit() a Tx, which calls back into the Snapshotter to work out if that should trigger another snapshot...). But that's fine, we can drop the lock while we're not accessing any member state.

This isn't great, and potentially risks leaving orphaned items in the pending_snapshots collection? But I think the fix is worthwhile for now, until we have a better task system to work with.

@eddyashton eddyashton requested a review from a team as a code owner March 4, 2025 16:46
@maxtropets maxtropets added the run-long-test Run Long Test job label Mar 4, 2025
@eddyashton eddyashton added this pull request to the merge queue Mar 5, 2025
Merged via the queue into microsoft:main with commit 6a79fcc Mar 5, 2025
21 checks passed
@eddyashton eddyashton deleted the snapshotter_lock_in_callback branch March 5, 2025 12:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
run-long-test Run Long Test job
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants