Skip to content

Commit a9c66e0

Browse files
authored
libsql: Use RwLock instead of RefCell for local connection (#2118)
The `local::Connection` struct has the following safety comments: ```rust // SAFETY: This is safe because we compile sqlite3 w/ SQLITE_THREADSAFE=1 unsafe impl Send for Connection {} // SAFETY: This is safe because we compile sqlite3 w/ SQLITE_THREADSAFE=1 unsafe impl Sync for Connection {} ``` This is correct with respect to the `*mut ffi::sqlite3` field, but NOT with respect to the `RefCell<Option<AuthHook>>` field. RefCell is not thread safe. Heavy load in a real application using local databases occasionally sees panics due to concurrent access to this RefCell. I don't have a simple repro right now, but this change eliminated the panics for me. Let me know if you'd like some extra work done like a repro or tests (not sure I'll have time but I will try..)
2 parents bc5d2eb + fb39e14 commit a9c66e0

File tree

2 files changed

+16
-15
lines changed

2 files changed

+16
-15
lines changed

libsql/Cargo.toml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,7 @@ core = [
6363
"dep:bitflags",
6464
"dep:bytes",
6565
"dep:futures",
66+
"dep:parking_lot",
6667
]
6768
stream = [
6869
"dep:futures",

libsql/src/local/connection.rs

Lines changed: 15 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -12,8 +12,8 @@ use super::{Database, Error, Result, Rows, RowsFuture, Statement, Transaction};
1212
use crate::TransactionBehavior;
1313

1414
use libsql_sys::ffi;
15-
use std::cell::RefCell;
1615
use std::{ffi::c_int, fmt, path::Path, sync::Arc};
16+
use parking_lot::RwLock;
1717

1818
/// A connection to a libSQL database.
1919
#[derive(Clone)]
@@ -25,7 +25,7 @@ pub struct Connection {
2525
#[cfg(feature = "replication")]
2626
pub(crate) writer: Option<crate::replication::Writer>,
2727

28-
authorizer: RefCell<Option<AuthHook>>,
28+
authorizer: Arc<RwLock<Option<AuthHook>>>,
2929
}
3030

3131
impl Drop for Connection {
@@ -68,7 +68,7 @@ impl Connection {
6868
drop_ref: Arc::new(()),
6969
#[cfg(feature = "replication")]
7070
writer: db.writer()?,
71-
authorizer: RefCell::new(None),
71+
authorizer: Arc::new(RwLock::new(None)),
7272
};
7373
#[cfg(feature = "sync")]
7474
if let Some(_) = db.sync_ctx {
@@ -93,7 +93,7 @@ impl Connection {
9393
drop_ref: Arc::new(()),
9494
#[cfg(feature = "replication")]
9595
writer: None,
96-
authorizer: RefCell::new(None),
96+
authorizer: Arc::new(RwLock::new(None)),
9797
}
9898
}
9999

@@ -473,7 +473,7 @@ impl Connection {
473473
}
474474
}
475475

476-
*self.authorizer.borrow_mut() = hook.clone();
476+
*self.authorizer.write() = hook.clone();
477477

478478
let (callback, user_data) = match hook {
479479
Some(_) => {
@@ -633,7 +633,7 @@ impl Connection {
633633
pub(crate) fn wal_insert_handle(&self) -> WalInsertHandle<'_> {
634634
WalInsertHandle {
635635
conn: self,
636-
in_session: RefCell::new(false),
636+
in_session: RwLock::new(false),
637637
}
638638
}
639639
}
@@ -647,7 +647,7 @@ unsafe extern "C" fn authorizer_callback(
647647
accessor: *const ::std::os::raw::c_char,
648648
) -> ::std::os::raw::c_int {
649649
let conn = user_data as *const Connection;
650-
let hook = unsafe { (*conn).authorizer.borrow() };
650+
let hook = unsafe { (*conn).authorizer.read() };
651651
let hook = match &*hook {
652652
Some(hook) => hook,
653653
None => return ffi::SQLITE_OK,
@@ -688,37 +688,37 @@ unsafe extern "C" fn authorizer_callback(
688688

689689
pub(crate) struct WalInsertHandle<'a> {
690690
conn: &'a Connection,
691-
in_session: RefCell<bool>,
691+
in_session: RwLock<bool>
692692
}
693693

694694
impl WalInsertHandle<'_> {
695695
pub fn insert_at(&self, frame_no: u32, frame: &[u8]) -> Result<()> {
696-
assert!(*self.in_session.borrow());
696+
assert!(*self.in_session.read());
697697
self.conn.wal_insert_frame(frame_no, frame)
698698
}
699699

700700
pub fn in_session(&self) -> bool {
701-
*self.in_session.borrow()
701+
*self.in_session.read()
702702
}
703703

704704
pub fn begin(&self) -> Result<()> {
705-
assert!(!*self.in_session.borrow());
705+
assert!(!*self.in_session.read());
706706
self.conn.wal_insert_begin()?;
707-
self.in_session.replace(true);
707+
*self.in_session.write() = true;
708708
Ok(())
709709
}
710710

711711
pub fn end(&self) -> Result<()> {
712-
assert!(*self.in_session.borrow());
712+
assert!(*self.in_session.read());
713713
self.conn.wal_insert_end()?;
714-
self.in_session.replace(false);
714+
*self.in_session.write() = false;
715715
Ok(())
716716
}
717717
}
718718

719719
impl Drop for WalInsertHandle<'_> {
720720
fn drop(&mut self) {
721-
if *self.in_session.borrow() {
721+
if *self.in_session.read() {
722722
if let Err(err) = self.conn.wal_insert_end() {
723723
tracing::error!("{:?}", err);
724724
Err(err).unwrap()

0 commit comments

Comments
 (0)