implement graph #16
There are no files selected for viewing
This file contains 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
This file contains 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,44 @@ | ||
| /* | ||
| * Copyright FalkorDB Ltd. 2023 - present | ||
| * Licensed under the Server Side Public License v1 (SSPLv1). | ||
| */ | ||
|
|
||
| use std::{mem::MaybeUninit, ptr::null_mut}; | ||
|
|
||
| use libc::pthread_mutex_t; | ||
|
|
||
| /// Wrap C mutex as we can't use Rust Mutex. | ||
| /// Used to lock the matrix only when we apply pending changes. | ||
| pub struct CMutex { | ||
| mutex: pthread_mutex_t, | ||
| } | ||
|
|
||
| impl CMutex { | ||
| pub fn new() -> Self { | ||
| unsafe { | ||
| let mut mutex = MaybeUninit::uninit(); | ||
| libc::pthread_mutex_init(mutex.as_mut_ptr(), null_mut()); | ||
| Self { | ||
| mutex: mutex.assume_init(), | ||
| } | ||
| } | ||
| } | ||
|
|
||
| pub fn lock(&mut self) { | ||
| unsafe { | ||
| libc::pthread_mutex_lock(&mut self.mutex); | ||
| } | ||
| } | ||
|
|
||
| pub fn unlock(&mut self) { | ||
| unsafe { | ||
| libc::pthread_mutex_unlock(&mut self.mutex); | ||
| } | ||
| } | ||
|
There was a problem hiding this comment. Improve safety and error handling in The
Here's a suggested implementation for both methods: pub unsafe fn lock(&self) -> Result<(), std::io::Error> {
let result = libc::pthread_mutex_lock(self.mutex.as_ptr());
if result != 0 {
return Err(std::io::Error::from_raw_os_error(result));
}
Ok(())
}
pub unsafe fn unlock(&self) -> Result<(), std::io::Error> {
let result = libc::pthread_mutex_unlock(self.mutex.as_ptr());
if result != 0 {
return Err(std::io::Error::from_raw_os_error(result));
}
Ok(())
}These implementations:
|
||
| } | ||
|
|
||
| impl Drop for CMutex { | ||
| fn drop(&mut self) { | ||
| unsafe { libc::pthread_mutex_destroy(&mut self.mutex) }; | ||
| } | ||
| } | ||
This file contains 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,53 @@ | ||
| /* | ||
| * Copyright FalkorDB Ltd. 2023 - present | ||
| * Licensed under the Server Side Public License v1 (SSPLv1). | ||
| */ | ||
|
|
||
| use std::ptr::null_mut; | ||
|
|
||
| use libc::{pthread_rwlock_t, PTHREAD_RWLOCK_INITIALIZER}; | ||
|
|
||
| /// Wrap C rwlock as we can't use Rust RWLock. | ||
| /// Used to lock the graph. | ||
| pub struct CRWLock { | ||
| rwlock: Box<pthread_rwlock_t>, | ||
| } | ||
|
|
||
| impl CRWLock { | ||
| pub fn new() -> Self { | ||
| let mut res = CRWLock { | ||
| rwlock: Box::new(PTHREAD_RWLOCK_INITIALIZER), | ||
| }; | ||
| unsafe { | ||
| libc::pthread_rwlock_init(res.rwlock.as_mut(), null_mut()); | ||
| } | ||
| res | ||
| } | ||
|
There was a problem hiding this comment. Add error handling for pthread_rwlock_init. The initialization of the rwlock looks correct, but there's no error handling for Consider adding error handling: pub fn new() -> Result<Self, std::io::Error> {
let mut rwlock = Box::new(PTHREAD_RWLOCK_INITIALIZER);
let result = unsafe {
libc::pthread_rwlock_init(rwlock.as_mut(), null_mut())
};
if result != 0 {
Err(std::io::Error::last_os_error())
} else {
Ok(CRWLock { rwlock })
}
}This change would propagate any initialization errors to the caller. |
||
|
|
||
| pub fn acquire_read(&mut self) { | ||
| unsafe { | ||
| let res = libc::pthread_rwlock_rdlock(self.rwlock.as_mut()); | ||
| debug_assert!(res == 0, "pthread_rwlock_rdlock failed"); | ||
| } | ||
| } | ||
|
|
||
| pub fn acquire_write(&mut self) { | ||
| unsafe { | ||
| let res = libc::pthread_rwlock_wrlock(self.rwlock.as_mut()); | ||
| debug_assert!(res == 0, "pthread_rwlock_wrlock failed"); | ||
| } | ||
| } | ||
|
There was a problem hiding this comment. Improve error handling for acquire_write. Similar to Suggested improvement: pub fn acquire_write(&mut self) -> Result<(), std::io::Error> {
let result = unsafe { libc::pthread_rwlock_wrlock(self.rwlock.as_mut()) };
if result != 0 {
Err(std::io::Error::from_raw_os_error(result))
} else {
Ok(())
}
}This change ensures that any errors during lock acquisition are properly handled and communicated to the caller. |
||
|
|
||
| pub fn release(&mut self) { | ||
| unsafe { | ||
| let res = libc::pthread_rwlock_unlock(self.rwlock.as_mut()); | ||
| debug_assert!(res == 0, "pthread_rwlock_unlock failed"); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| impl Drop for CRWLock { | ||
| fn drop(&mut self) { | ||
| unsafe { libc::pthread_rwlock_destroy(self.rwlock.as_mut()) }; | ||
| } | ||
| } | ||
Oops, something went wrong.
Oops, something went wrong.
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.
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.
Improve safety and error handling in the
newmethod.The
newmethod correctly initializes the mutex, but there are a few improvements that can be made:unsafedue to the use of unsafe operations.pthread_mutex_initcall.null_mut().Here's a suggested implementation:
This implementation:
unsafe.Resultto handle potential errors.std::ptr::null()instead ofnull_mut()for the attributes pointer.pthread_mutex_initand returns an error if initialization fails.