Repository: denoland/deno
Pull request: fix(ext/node): hold weak reference to sqlite database in instances

Deterministic close and releases file lock on Windows.

Fixes https://github.com/denoland/deno/issues/29033

Changed files:
filename: ext/node/ops/sqlite/database.rs
patch: @@ -67,6 +67,7 @@ struct ApplyChangesetOptions<'a> {
 
 pub struct DatabaseSync {
   conn: Rc<RefCell<Option<rusqlite::Connection>>>,
+  statements: Rc<RefCell<Vec<*mut libsqlite3_sys::sqlite3_stmt>>>,
   options: DatabaseSyncOptions,
   location: String,
 }
@@ -209,6 +210,7 @@ impl DatabaseSync {
 
     Ok(DatabaseSync {
       conn: Rc::new(RefCell::new(db)),
+      statements: Rc::new(RefCell::new(Vec::new())),
       location,
       options,
     })
@@ -249,7 +251,18 @@ impl DatabaseSync {
       return Err(SqliteError::AlreadyClosed);
     }
 
-    *self.conn.borrow_mut() = None;
+    // Finalize all prepared statements
+    for stmt in self.statements.borrow_mut().drain(..) {
+      if !stmt.is_null() {
+        // SAFETY: `stmt` is a valid statement handle.
+        unsafe {
+          libsqlite3_sys::sqlite3_finalize(stmt);
+        }
+      }
+    }
+
+    let _ = self.conn.borrow_mut().take();
+
     Ok(())
   }
 
@@ -297,9 +310,12 @@ impl DatabaseSync {
       return Err(SqliteError::PrepareFailed);
     }
 
+    self.statements.borrow_mut().push(raw_stmt);
+
     Ok(StatementSync {
       inner: raw_stmt,
-      db: self.conn.clone(),
+      db: Rc::downgrade(&self.conn),
+      statements: Rc::clone(&self.statements),
       use_big_ints: Cell::new(false),
       allow_bare_named_params: Cell::new(true),
       is_iter_finished: false,
@@ -547,7 +563,7 @@ impl DatabaseSync {
     Ok(Session {
       inner: raw_session,
       freed: Cell::new(false),
-      db: self.conn.clone(),
+      db: Rc::downgrade(&self.conn),
     })
   }
 }

filename: ext/node/ops/sqlite/session.rs
patch: @@ -3,7 +3,7 @@
 use std::cell::Cell;
 use std::cell::RefCell;
 use std::ffi::c_void;
-use std::rc::Rc;
+use std::rc::Weak;
 
 use deno_core::op2;
 use deno_core::GarbageCollected;
@@ -23,8 +23,8 @@ pub struct Session {
   pub(crate) inner: *mut ffi::sqlite3_session,
   pub(crate) freed: Cell<bool>,
 
-  // Hold a strong reference to the database.
-  pub(crate) db: Rc<RefCell<Option<rusqlite::Connection>>>,
+  // Hold a weak reference to the database.
+  pub(crate) db: Weak<RefCell<Option<rusqlite::Connection>>>,
 }
 
 impl GarbageCollected for Session {
@@ -65,7 +65,15 @@ impl Session {
   // Closes the session.
   #[fast]
   fn close(&self) -> Result<(), SqliteError> {
-    if self.db.borrow().is_none() {
+    let db_rc = self.db.upgrade().ok_or_else(|| {
+      SqliteError::create_enhanced_error::<()>(
+        ffi::SQLITE_MISUSE,
+        &SqliteError::AlreadyClosed.to_string(),
+        None,
+      )
+      .unwrap_err()
+    })?;
+    if db_rc.borrow().is_none() {
       return SqliteError::create_enhanced_error(
         ffi::SQLITE_MISUSE,
         &SqliteError::AlreadyClosed.to_string(),
@@ -82,7 +90,15 @@ impl Session {
   // This method is a wrapper around `sqlite3session_changeset()`.
   #[buffer]
   fn changeset(&self) -> Result<Box<[u8]>, SqliteError> {
-    if self.db.borrow().is_none() {
+    let db_rc = self.db.upgrade().ok_or_else(|| {
+      SqliteError::create_enhanced_error::<Box<[u8]>>(
+        ffi::SQLITE_MISUSE,
+        &SqliteError::AlreadyClosed.to_string(),
+        None,
+      )
+      .unwrap_err()
+    })?;
+    if db_rc.borrow().is_none() {
       return SqliteError::create_enhanced_error(
         ffi::SQLITE_MISUSE,
         &SqliteError::AlreadyClosed.to_string(),
@@ -105,7 +121,15 @@ impl Session {
   // This method is a wrapper around `sqlite3session_patchset()`.
   #[buffer]
   fn patchset(&self) -> Result<Box<[u8]>, SqliteError> {
-    if self.db.borrow().is_none() {
+    let db_rc = self.db.upgrade().ok_or_else(|| {
+      SqliteError::create_enhanced_error::<Box<[u8]>>(
+        ffi::SQLITE_MISUSE,
+        &SqliteError::AlreadyClosed.to_string(),
+        None,
+      )
+      .unwrap_err()
+    })?;
+    if db_rc.borrow().is_none() {
       return SqliteError::create_enhanced_error(
         ffi::SQLITE_MISUSE,
         &SqliteError::AlreadyClosed.to_string(),

filename: ext/node/ops/sqlite/statement.rs
patch: @@ -3,6 +3,7 @@
 use std::cell::Cell;
 use std::cell::RefCell;
 use std::rc::Rc;
+use std::rc::Weak;
 
 use deno_core::op2;
 use deno_core::v8;
@@ -26,7 +27,8 @@ pub struct RunStatementResult {
 #[derive(Debug)]
 pub struct StatementSync {
   pub inner: *mut ffi::sqlite3_stmt,
-  pub db: Rc<RefCell<Option<rusqlite::Connection>>>,
+  pub db: Weak<RefCell<Option<rusqlite::Connection>>>,
+  pub statements: Rc<RefCell<Vec<*mut ffi::sqlite3_stmt>>>,
 
   pub use_big_ints: Cell<bool>,
   pub allow_bare_named_params: Cell<bool>,
@@ -36,10 +38,16 @@ pub struct StatementSync {
 
 impl Drop for StatementSync {
   fn drop(&mut self) {
-    // SAFETY: `self.inner` is a valid pointer to a sqlite3_stmt
-    // no other references to this pointer exist.
-    unsafe {
-      ffi::sqlite3_finalize(self.inner);
+    if self.statements.borrow().contains(&self.inner) {
+      self
+        .statements
+        .borrow_mut()
+        .retain(|stmt| *stmt != self.inner);
+      // SAFETY: `self.inner` is a valid pointer to a sqlite3_stmt
+      // no other references to this pointer exist.
+      unsafe {
+        ffi::sqlite3_finalize(self.inner);
+      }
     }
   }
 }
@@ -146,7 +154,8 @@ impl StatementSync {
           } else if value.abs() <= MAX_SAFE_JS_INTEGER {
             v8::Number::new(scope, value as f64).into()
           } else {
-            let db = self.db.borrow();
+            let db_rc = self.db.upgrade().ok_or(SqliteError::InUse)?;
+            let db = db_rc.borrow();
             let db = db.as_ref().ok_or(SqliteError::InUse)?;
             let handle = db.handle();
 
@@ -293,7 +302,8 @@ impl StatementSync {
       let value: v8::Local<v8::BigInt> = value.try_into().unwrap();
       let (as_int, lossless) = value.i64_value();
       if !lossless {
-        let db = self.db.borrow();
+        let db_rc = self.db.upgrade().ok_or(SqliteError::InUse)?;
+        let db = db_rc.borrow();
         let db = db.as_ref().ok_or(SqliteError::InUse)?;
         // SAFETY: lifetime of the connection is guaranteed by the rusqlite API.
         let handle = unsafe { db.handle() };
@@ -310,7 +320,8 @@ impl StatementSync {
       // as it lives as long as the StatementSync instance.
       unsafe { ffi::sqlite3_bind_int64(raw, index, as_int) }
     } else {
-      let db = self.db.borrow();
+      let db_rc = self.db.upgrade().ok_or(SqliteError::InUse)?;
+      let db = db_rc.borrow();
       let db = db.as_ref().ok_or(SqliteError::InUse)?;
       // SAFETY: lifetime of the connection is guaranteed by the rusqlite API.
       let handle = unsafe { db.handle() };
@@ -327,7 +338,8 @@ impl StatementSync {
 
   fn check_error_code(&self, r: i32) -> Result<(), SqliteError> {
     if r != ffi::SQLITE_OK {
-      let db = self.db.borrow();
+      let db_rc = self.db.upgrade().ok_or(SqliteError::InUse)?;
+      let db = db_rc.borrow();
       let db = db.as_ref().ok_or(SqliteError::InUse)?;
       // SAFETY: lifetime of the connection is guaranteed by the rusqlite API.
       let handle = unsafe { db.handle() };
@@ -384,7 +396,8 @@ impl StatementSync {
 
             let e = bare_named_params.insert(bare_name, i);
             if e.is_some() {
-              let db = self.db.borrow();
+              let db_rc = self.db.upgrade().ok_or(SqliteError::InUse)?;
+              let db = db_rc.borrow();
               let db = db.as_ref().ok_or(SqliteError::InUse)?;
               // SAFETY: lifetime of the connection is guaranteed by the rusqlite API.
               let handle = unsafe { db.handle() };
@@ -416,7 +429,8 @@ impl StatementSync {
             }
 
             if r == 0 {
-              let db = self.db.borrow();
+              let db_rc = self.db.upgrade().ok_or(SqliteError::InUse)?;
+              let db = db_rc.borrow();
               let db = db.as_ref().ok_or(SqliteError::InUse)?;
               // SAFETY: lifetime of the connection is guaranteed by the rusqlite API.
               let handle = unsafe { db.handle() };
@@ -511,7 +525,8 @@ impl StatementSync {
     scope: &mut v8::HandleScope,
     #[varargs] params: Option<&v8::FunctionCallbackArguments>,
   ) -> Result<RunStatementResult, SqliteError> {
-    let db = self.db.borrow();
+    let db_rc = self.db.upgrade().ok_or(SqliteError::InUse)?;
+    let db = db_rc.borrow();
     let db = db.as_ref().ok_or(SqliteError::InUse)?;
 
     self.bind_params(scope, params)?;
@@ -720,7 +735,8 @@ impl StatementSync {
     unsafe {
       let raw = ffi::sqlite3_expanded_sql(self.inner);
       if raw.is_null() {
-        let db = self.db.borrow();
+        let db_rc = self.db.upgrade().ok_or(SqliteError::InUse)?;
+        let db = db_rc.borrow();
         let db = db.as_ref().ok_or(SqliteError::InUse)?;
         let handle = db.handle();
 

filename: tests/unit_node/sqlite_test.ts
patch: @@ -333,3 +333,13 @@ Deno.test("[node/sqlite] StatementSync empty blob", () => {
 
   db.close();
 });
+
+Deno.test("[node/sqlite] Database close locks", () => {
+  const db = new DatabaseSync(`${tempDir}/test4.db`);
+  const statement = db.prepare(
+    "CREATE TABLE test (key INTEGER PRIMARY KEY, value TEXT)",
+  );
+  statement.run();
+  db.close();
+  Deno.removeSync(`${tempDir}/test4.db`);
+});