-
Notifications
You must be signed in to change notification settings - Fork 1.3k
use mutex to allow one write at a time #4028
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
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.
Code Review
This pull request aims to prevent concurrent writes to a BLE characteristic by introducing a mutex. While the approach of using a mutex per characteristic is sound, the Mutex implementation it relies on has a critical race condition that renders it ineffective, failing to prevent concurrent writes. Additionally, the newly introduced map of mutexes is not cleared, leading to a memory leak. My review includes comments on these critical and high-severity issues with suggestions for fixing them.
|
/gemini review |
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.
Code Review
This pull request introduces a mutex to serialize write operations to BLE characteristics, which is a good practice to prevent concurrent modification issues. The implementation of the Mutex class is also updated, fixing a critical race condition present in the previous version. The new mutex is correctly applied in BleTransport using a try...finally block to ensure the lock is always released. However, there is a concern in the new Mutex implementation where errors from previous lock holders are silently ignored. This can mask underlying problems and should be addressed. I've left a specific comment with a suggestion.
| try { | ||
| await previousLock; | ||
| } catch (_) {} |
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.
Silently swallowing errors from a previous lock holder is risky. While allowing the next waiter to acquire the lock is a valid strategy for robustness, ignoring the error completely can hide serious bugs in the code that uses the mutex. If a previous operation failed, the resource protected by the mutex could be in an inconsistent state. Logging the error provides crucial visibility for debugging such scenarios.
try {
await previousLock;
} catch (e, s) {
// It's important to log that a previous lock holder failed to aid debugging.
// Consider using a proper logger instead of print.
print('Mutex: previous lock holder failed with error: $e');
}
pls trap it and give me the code that can produce this issue sir. |
The issue is that checking if the lock is free and acquiring the lock are two separate steps with an await in between. This results in a race condition where another coroutine could acquire the lock while its being set on the first one, making both of them assume they have the lock. The new implementation is atomic and is based off the implementation of the mutex package
https://github.com/hoylen/dart-mutex/blob/master/lib/src/read_write_mutex.dart#L256