`node:sqlite`: Connection lost if `DatabaseSync` is garbage-collected

[ayuusweetfish]

Version: Deno 2.3.2 and above

Reproduction case

Environment: Linux (reproduced on ARM64 and x86-64)

Program:

import { DatabaseSync } from 'node:sqlite'
const db = new DatabaseSync(':memory:')
const stmt = db.prepare(`SELECT 1`)

const a = []
while (true) {
  for (let i = 0; i < 1000000; i++) a.push(0)   // Try to trigger GC
  console.log(a.length)
  stmt.run()
}

After a few iterations, this fails with a confusing error:

run 21000000
run 22000000
run 23000000
error: Uncaught (in promise) Error: Database is already in use
  stmt.run()
       ^
    at file:///path/to/test.js:9:8

Cause

In the case above, there is no reachable strong reference to the DatabaseSync object (db), so it may be garbage-collected even when the StatementSync (stmt) is still alive.

When this happens, the underlying rusqlite::Connection is dereferenced and released, invalidating the weak reference from the StatementSync Rust struct.

deno/ext/node/ops/sqlite/database.rs

Line 236 in 0bd52c4

pub conn: Rc<RefCell<Option<rusqlite::Connection>>>,

deno/ext/node/ops/sqlite/statement.rs

Line 70 in 0bd52c4

pub db: Weak<RefCell<Option<rusqlite::Connection>>>,

Then, in StatementSync::run(), the weak reference reads None, thus throwing an InUse error, which bears the error message "Database is already in use" as seen above.

deno/ext/node/ops/sqlite/statement.rs

Line 621 in 0bd52c4

let db_rc = self.db.upgrade().ok_or(SqliteError::InUse)?;

This issue emerged between v2.3.1 and v2.3.2 (previous versions run correctly until memory is exhausted: RangeError: Invalid array length). I have not taken time to bisect on the commits, but #29210 appears highly relevant.

Solution

I think we can go back to a strong Rc in the statement. Then, when DatabaseSync::close() is explicitly called, we should invalidate all associated StatementSync objects by dropping the references in them (we can simply change the error code above from InUse to AlreadyClosed). In this way, when DatabaseSync is garbage-collected, the StatementSync objects stay valid. I'm willing to help if it sounds good ^ ^

[bartlomieju]

I asked @littledivy to take a look at the suggestion for the fix.

[bartlomieju]

@ayuusweetfish we discussed this offline and the solution sounds good. PRs are welcome!

[ayuusweetfish]

I have drafted a PR (#31737). Please take a look!