Repository: denoland/deno
Pull request: fix(ext/node): `DatabaseSync` garbage collection invalidating associated resources

Resolves #31732, following the proposed fix there.

> **Note:** when the database is explicitly `close()`d, the associated resources (`StatementSync` and `Session`) are already dropped due to `Database::close()` taking away these references. Hence no additional change is needed to invalidate them; we simply check whether they are `None`.
>
> https://github.com/denoland/deno/blob/0bd52c4640208ae025ad3ccd6b6cbbfbe5317b60/ext/node/ops/sqlite/database.rs#L507
>
> https://github.com/denoland/deno/blob/0bd52c4640208ae025ad3ccd6b6cbbfbe5317b60/ext/node/ops/sqlite/database.rs#L512

A remaining question is how we should distinguish between the error codes `StatementFinalized` and `AlreadyClosed`. Apparently, statements are "finalized" (`inner` set to `None`) only when the database is closed, so it seems that these two codes are currently equivalent. The added test case expects two different error messages, but it does not seem very natural.

Changed files:
filename: ext/node/ops/sqlite/database.rs
patch: @@ -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(),
     })
   }
 

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::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() {

filename: ext/node/ops/sqlite/statement.rs
patch: @@ -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)?;
 

filename: tests/unit_node/sqlite_test.ts
patch: @@ -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:");