-
Notifications
You must be signed in to change notification settings - Fork 74
[WIP] Experimental FD lock support #2813
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
base: main
Are you sure you want to change the base?
Conversation
|
I'll analyze this and get back to you. |
How to use the Graphite Merge QueueAdd the label mergequeue to this PR to add it to the merge queue. You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
Add experimental cross-process MLS commit locking using
|
| } | ||
|
|
||
| /// Open or create the lock file for a given group ID. | ||
| fn open_lock_file(&self, group_id: &[u8]) -> Result<File, std::io::Error> { |
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.
Empty group_id yields an empty filename, so open_lock_file tries to open the directory and fails. Consider rejecting empty group_id early (e.g., return std::io::ErrorKind::InvalidInput).
- fn open_lock_file(&self, group_id: &[u8]) -> Result<File, std::io::Error> {
- let path = self.lock_file_path(group_id);
+ fn open_lock_file(&self, group_id: &[u8]) -> Result<File, std::io::Error> {
+ if group_id.is_empty() {
+ return Err(std::io::Error::new(std::io::ErrorKind::InvalidInput, "empty group_id"));
+ }
+ let path = self.lock_file_path(group_id);
OpenOptions::new()
.read(true)
.write(true)
.create(true)
.open(path)
}🚀 Reply to ask Macroscope to explain or update this suggestion.
👍 Helpful? React to give us feedback.
| impl MlsGroupGuard { | ||
| /// Create a new guard by acquiring a blocking write lock. | ||
| fn new(file: File) -> Result<Self, CommitLockError> { | ||
| // Box the lock so it has a stable address |
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.
Suggestion: avoid converting the lock Box to a raw pointer before fallible steps. Defer Box::into_raw until after the guard is boxed and the write lock succeeds, or ensure error paths convert back with Box::from_raw so RwLock/File drop.
- let lock = Box::into_raw(Box::new(RwLock::new(file)));
+ let lock_box = Box::new(RwLock::new(file));
@@
- let guard = (*lock)
+ let guard = (*lock_box)
.write()
.map_err(|e| CommitLockError::LockAcquire(e.to_string()))?;
@@
- let guard = Box::into_raw(Box::new(guard));
+ let guard = Box::into_raw(Box::new(guard));
+ let lock = Box::into_raw(lock_box);
@@
- Ok(Self { guard, lock })
+ Ok(Self { guard, lock })🚀 Reply to ask Macroscope to explain or update this suggestion.
👍 Helpful? React to give us feedback.


Add cross-process file locking for MLS group commits
TL;DR
Implemented a file-based locking mechanism for MLS group commits that works across processes, preventing race conditions when multiple clients access the same group.
What changed?
commit_lock.rsmodule with platform-specific implementations:fd-lockfor file-based locking that works across processesGroupCommitLockto create lock files in a directory based on the database path and installation IDHow to test?
Why make this change?
Previously, the locking mechanism only worked within a single process. This could lead to race conditions and data corruption when multiple clients (in different processes) tried to modify the same group simultaneously. The file-based locking ensures that only one process can modify a group at a time, preventing potential data corruption and ensuring consistency across clients.