fix(ext/node): DatabaseSync garbage collection invalidating associated resources

ext/node/ops/sqlite/database.rs

@@ -745,7 +745,7 @@ impl DatabaseSync {
 
     Ok(StatementSync {
       inner: stmt_cell,
-      db: Rc::downgrade(&self.conn),
+      db: self.conn.clone(),
       statements: Rc::clone(&self.statements),
       ignore_next_sqlite_error: Rc::clone(&self.ignore_next_sqlite_error),
       use_big_ints: Cell::new(false),
@@ -1229,7 +1229,7 @@ impl DatabaseSync {
     Ok(Session {
       inner: raw_session,
       freed: Cell::new(false),
-      db: Rc::downgrade(&self.conn),
+      db: self.conn.clone(),
     })
   }
 

ext/node/ops/sqlite/session.rs

@@ -3,7 +3,7 @@
 use std::cell::Cell;
 use std::cell::RefCell;
 use std::ffi::c_void;
-use std::rc::Weak;
+use std::rc::Rc;
 
 use deno_core::FromV8;
 use deno_core::GarbageCollected;
@@ -87,7 +87,7 @@ pub struct Session {
   pub(crate) freed: Cell<bool>,
 
   // Hold a weak reference to the database.
-  pub(crate) db: Weak<RefCell<Option<rusqlite::Connection>>>,
+  pub(crate) db: Rc<RefCell<Option<rusqlite::Connection>>>,
 }
 
 // SAFETY: we're sure this can be GCed
@@ -134,11 +134,7 @@ impl Session {
   #[fast]
   #[undefined]
   fn close(&self) -> Result<(), SqliteError> {
-    let db_rc = self
-      .db
-      .upgrade()
-      .ok_or_else(|| SqliteError::AlreadyClosed)?;
-    if db_rc.borrow().is_none() {
+    if self.db.borrow().is_none() {
       return Err(SqliteError::AlreadyClosed);
     }
 
@@ -151,11 +147,7 @@ impl Session {
   // This method is a wrapper around `sqlite3session_changeset()`.
   #[buffer]
   fn changeset(&self) -> Result<Box<[u8]>, SqliteError> {
-    let db_rc = self
-      .db
-      .upgrade()
-      .ok_or_else(|| SqliteError::AlreadyClosed)?;
-    if db_rc.borrow().is_none() {
+    if self.db.borrow().is_none() {
       return Err(SqliteError::AlreadyClosed);
     }
     if self.freed.get() {
@@ -170,11 +162,7 @@ impl Session {
   // This method is a wrapper around `sqlite3session_patchset()`.
   #[buffer]
   fn patchset(&self) -> Result<Box<[u8]>, SqliteError> {
-    let db_rc = self
-      .db
-      .upgrade()
-      .ok_or_else(|| SqliteError::AlreadyClosed)?;
-    if db_rc.borrow().is_none() {
+    if self.db.borrow().is_none() {
       return Err(SqliteError::AlreadyClosed);
     }
     if self.freed.get() {

ext/node/ops/sqlite/statement.rs

@@ -3,7 +3,6 @@
 use std::cell::Cell;
 use std::cell::RefCell;
 use std::rc::Rc;
-use std::rc::Weak;
 
 use deno_core::GarbageCollected;
 use deno_core::ToV8;
@@ -67,7 +66,7 @@ pub type InnerStatementPtr = Rc<Cell<Option<*mut ffi::sqlite3_stmt>>>;
 #[derive(Debug)]
 pub struct StatementSync {
   pub inner: InnerStatementPtr,
-  pub db: Weak<RefCell<Option<rusqlite::Connection>>>,
+  pub db: Rc<RefCell<Option<rusqlite::Connection>>>,
   pub statements: Rc<RefCell<Vec<InnerStatementPtr>>>,
   pub ignore_next_sqlite_error: Rc<Cell<bool>>,
 
@@ -432,9 +431,8 @@ impl StatementSync {
         self.ignore_next_sqlite_error.set(false);
         return Ok(());
       }
-      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 db = self.db.borrow();
+      let db = db.as_ref().ok_or(SqliteError::AlreadyClosed)?;
 
       // SAFETY: db.handle() is valid
       unsafe {
@@ -631,9 +629,8 @@ impl StatementSync {
     scope: &mut v8::PinScope<'_, '_>,
     #[varargs] params: Option<&v8::FunctionCallbackArguments>,
   ) -> Result<RunStatementResult, SqliteError> {
-    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 db = self.db.borrow();
+    let db = db.as_ref().ok_or(SqliteError::AlreadyClosed)?;
 
     self.bind_params(scope, params)?;
 

tests/unit_node/sqlite_test.ts

@@ -452,15 +452,18 @@ Deno.test("[node/sqlite] Database backup", async () => {
   Deno.removeSync(`${tempDir}/backup.db`);
 });
 
-Deno.test("[node/sqlite] calling StatementSync methods after connection has closed", () => {
+Deno.test("[node/sqlite] calling StatementSync and Session methods after connection has closed", () => {
   const errMessage = "statement has been finalized";
+  const errMessageClosed = "database is not open";
 
   const db = new DatabaseSync(":memory:");
   db.exec("CREATE TABLE test (value INTEGER)");
   const stmt = db.prepare("INSERT INTO test (value) VALUES (?), (?)");
+  const sess = db.createSession();
   stmt.run(1, 2);
   db.close();
 
+  assertThrows(() => stmt.run(), Error, errMessageClosed);
   assertThrows(() => stmt.all(), Error, errMessage);
   assertThrows(() => stmt.expandedSQL, Error, errMessage);
   assertThrows(() => stmt.get(), Error, errMessage);
@@ -473,6 +476,9 @@ Deno.test("[node/sqlite] calling StatementSync methods after connection has clos
   );
   assertThrows(() => stmt.setReadBigInts(true), Error, errMessage);
   assertThrows(() => stmt.sourceSQL, Error, errMessage);
+
+  assertThrows(() => sess.changeset(), Error, errMessageClosed);
+  assertThrows(() => sess.patchset(), Error, errMessageClosed);
 });
 
 // Regression test for https://github.com/denoland/deno/issues/30144
@@ -1036,6 +1042,18 @@ Deno.test("[node/sqlite] numbered parameters in different order (?2, ?1)", () =>
   assertEquals(row, { a: "second_arg", b: "first_arg", __proto__: null });
 });
 
+Deno.test("[node/sqlite] Database GC should not invalidate statements and sessions", () => {
+  const db = new DatabaseSync(":memory:");
+  const stmt = db.prepare("SELECT 1");
+  const sess = db.createSession();
+  const a = [];
+  for (let i = 0; i < 100; i++) {
+    for (let i = 0; i < 1000000; i++) a.push(0); // Try to trigger GC
+    stmt.run();
+    sess.changeset();
+  }
+});
+
 // https://github.com/denoland/deno/issues/31744
 Deno.test("[node/sqlite] StatementSync alive while iterator", () => {
   using db = new DatabaseSync(":memory:");