-
Notifications
You must be signed in to change notification settings - Fork 12
fix: use std::sync::Mutex for SQLite connection to avoid tokio runtime panics #164
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
Conversation
📝 WalkthroughWalkthroughThe pull request replaces the asynchronous mutex ( Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Suggested labels
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
eca9dec to
fbe9052
Compare
✅ Coverage: 90.7% → 90.71% (+0.01%) |
1 similar comment
✅ Coverage: 90.7% → 90.71% (+0.01%) |
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.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@crates/mdk-sqlite-storage/src/lib.rs`:
- Line 74: The use of std::sync::Mutex::lock() currently calls unwrap() and can
panic on a poisoned mutex; update all lock sites to handle PoisonError by
replacing unwrap() with unwrap_or_else(|e| e.into_inner()) so the code recovers
the inner guard instead of panicking—apply this change at all Mutex::lock()
calls in the file (notably inside the with_connection method and other methods
that acquire the same mutex) so every call (e.g., where
connection_mutex.lock().unwrap()) becomes
connection_mutex.lock().unwrap_or_else(|e| e.into_inner()).
…e panics
Replace tokio::sync::Mutex with std::sync::Mutex for the SQLite connection.
Since rusqlite operations are inherently synchronous, there's no benefit to
using tokio's async Mutex, and blocking_lock() panics when called from within
a tokio async runtime context ("Cannot block the current thread from within a runtime").
This allows consumers to call MDK methods directly from async code without
needing to wrap calls in std::thread::spawn() to escape the runtime context.
fbe9052 to
73d9a30
Compare
✅ Coverage: 90.7% → 90.71% (+0.01%) |
| use openmls_traits::storage::{StorageProvider, traits}; | ||
| use rusqlite::Connection; | ||
| use tokio::sync::Mutex; | ||
| use std::sync::Mutex; |
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.
why not parking_lot?
I know I approved earlier but I think I actually want to take a close look into this before we move on it.
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.
Ok - I've had a more thorough look and I'm happy.
| use openmls_traits::storage::{StorageProvider, traits}; | ||
| use rusqlite::Connection; | ||
| use tokio::sync::Mutex; | ||
| use std::sync::Mutex; |
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.
Ok - I've had a more thorough look and I'm happy.
Problem
MdkSqliteStorageusestokio::sync::Mutexfor the SQLite connection and callsblocking_lock()to access it. This panics when called from within a tokio async runtime context:This forces consumers (like whitenoise-rs) to wrap ALL MDK calls in
std::thread::spawn()to escape the tokio runtime context, adding significant complexity and overhead.Solution
Replace
tokio::sync::Mutexwithstd::sync::Mutex. Since rusqlite operations are inherently synchronous, there's no benefit to using tokio's async Mutex.std::sync::Mutex::lock()does not panic in async contexts - it simply blocks until the lock is acquired.Changes
connection: Arc<tokio::sync::Mutex<Connection>>toArc<std::sync::Mutex<Connection>>.blocking_lock()calls to.lock().unwrap()Impact
This PR fixes runtime panics in MdkSqliteStorage by switching the SQLite connection synchronization from tokio::sync::Mutex to std::sync::Mutex, allowing blocking rusqlite operations to run from within a Tokio async runtime without triggering "Cannot block the current thread from within a runtime". Consumers can now call MDK storage methods directly from async contexts without spawning dedicated threads to avoid panics.
What changed:
Security impact:
Protocol changes:
API surface:
Testing: