diff --git a/docs/runtime/sqlite.mdx b/docs/runtime/sqlite.mdx
index 1358af68caa7..d3d1c600d93e 100644
--- a/docs/runtime/sqlite.mdx
+++ b/docs/runtime/sqlite.mdx
@@ -112,15 +112,15 @@ const db = new Database("./mydb.sqlite");
### `.close(throwOnError: boolean = false)`
-To close a database connection but allow existing queries to finish, call `.close(false)`:
+To close a database connection, call `.close()`:
```ts db.ts icon="/icons/typescript.svg" highlight={3}
const db = new Database();
// ... do stuff
-db.close(false);
+db.close();
```
-To close the database and throw an error if there are any pending queries, call `.close(true)`:
+Any prepared statements that were not finalized are finalized as part of closing, so the connection (and the database file) is released immediately. Using a statement after its database was closed throws an error, except `toString()`, which returns an empty string, and `finalize()`, which stays safe to call. Pass `true` to throw if the connection fails to close:
```ts db.ts icon="/icons/typescript.svg" highlight={3}
const db = new Database();
@@ -129,8 +129,8 @@ db.close(true);
```
- `close(false)` is called automatically when the database is garbage collected. It is safe to call multiple times but
- has no effect after the first.
+ `close()` is called automatically when the database is garbage collected. It is safe to call multiple times but has no
+ effect after the first.
### `using` statement
diff --git a/packages/bun-types/sqlite.d.ts b/packages/bun-types/sqlite.d.ts
index ff0065132750..57412b8d8bd8 100644
--- a/packages/bun-types/sqlite.d.ts
+++ b/packages/bun-types/sqlite.d.ts
@@ -264,9 +264,12 @@ declare module "bun:sqlite" {
/**
* Close the database connection.
*
- * It is safe to call this method multiple times. If the database is already
- * closed, this is a no-op. Running queries after the database has been
- * closed throws an error.
+ * Prepared statements that were not finalized are finalized as part of
+ * closing, so the connection is released immediately. It is safe to call
+ * this method multiple times. If the database is already closed, this is
+ * a no-op. Running queries or using statements after the database has
+ * been closed throws an error, except statement `toString()`, which
+ * returns an empty string, and `finalize()`, which stays safe to call.
*
* @example
* ```ts
@@ -274,15 +277,13 @@ declare module "bun:sqlite" {
* ```
* This is called automatically when the database instance is garbage collected.
*
- * Internally, this calls `sqlite3_close_v2`.
+ * Internally, this calls `sqlite3_close`.
*/
close(
/**
- * If `true`, throw an error if the database is in use
+ * If `true`, throw an error if the connection fails to close
* @default false
*
- * When `true`, this calls `sqlite3_close` instead of `sqlite3_close_v2`.
- *
* Learn more in the [sqlite3 documentation](https://www.sqlite.org/c3ref/close.html).
*
* In the future, Bun may default `throwOnError` to `true`, but for backwards compatibility it is `false` by default.
diff --git a/src/jsc/bindings/sqlite/JSSQLStatement.cpp b/src/jsc/bindings/sqlite/JSSQLStatement.cpp
index 83b78490b14c..5e41cc2e2716 100644
--- a/src/jsc/bindings/sqlite/JSSQLStatement.cpp
+++ b/src/jsc/bindings/sqlite/JSSQLStatement.cpp
@@ -45,6 +45,7 @@
#include "wtf/BitVector.h"
#include "wtf/FastBitVector.h"
#include "wtf/Vector.h"
+#include
#include
#include
#include "wtf/LazyRef.h"
@@ -196,11 +197,9 @@ static inline JSC::JSValue jsBigIntFromSQLite(JSC::JSGlobalObject* globalObject,
return {}; \
}
-#define CHECK_PREPARED_JIT \
- if (castedThis->stmt == nullptr || castedThis->version_db == nullptr) [[unlikely]] { \
- throwException(lexicalGlobalObject, scope, createError(lexicalGlobalObject, "Statement has finalized"_s)); \
- return {}; \
- }
+namespace WebCore {
+class JSSQLStatement;
+}
DECLARE_ALLOCATOR_WITH_HEAP_IDENTIFIER(VersionSqlite3);
@@ -217,6 +216,10 @@ class VersionSqlite3 {
sqlite3* db;
std::atomic version;
size_t reference_count;
+ // Live statement wrappers on this connection. close() finalizes each one
+ // and nulls its stmt pointer, like better-sqlite3's CloseHandles() and
+ // node:sqlite's FinalizeStatements().
+ WTF::HashSet statements;
void release()
{
@@ -475,6 +478,7 @@ class JSSQLStatement : public JSC::JSDestructibleObject {
JSSQLStatement* ptr = new (NotNull, JSC::allocateCell(globalObject->vm())) JSSQLStatement(structure, *globalObject, stmt, version_db, memorySizeChange);
if (version_db) {
++version_db->reference_count;
+ version_db->statements.add(ptr);
}
ptr->finishCreation(globalObject->vm());
return ptr;
@@ -937,14 +941,20 @@ static inline bool rebindValue(JSC::JSGlobalObject* lexicalGlobalObject, sqlite3
#undef CHECK_BIND
}
-static JSC::JSValue rebindObject(JSC::JSGlobalObject* globalObject, SQLiteBindingsMap& bindings, JSC::JSObject* target, JSC::ThrowScope& scope, sqlite3* db, sqlite3_stmt* stmt, bool safeIntegers, JSSQLStatement* statement)
+static JSC::JSValue rebindObject(JSC::JSGlobalObject* globalObject, SQLiteBindingsMap& bindings, JSC::JSObject* target, JSC::ThrowScope& scope, sqlite3* db, VersionSqlite3* versionDB, sqlite3_stmt* stmt, bool safeIntegers, JSSQLStatement* statement)
{
int count = 0;
// Reading a property off `target` can run arbitrary JS (getters, Proxy
- // traps), which can call statement.finalize() and free `stmt`. Re-validate
- // before touching `stmt` again after any callback into JS.
+ // traps), which can call statement.finalize() or db.close() and free
+ // `stmt`. Re-validate before touching `stmt` again after any callback
+ // into JS.
const auto& statementStillAlive = [&]() -> bool {
+ if (versionDB->db != db) [[unlikely]] {
+ if (!scope.exception())
+ throwException(globalObject, scope, createError(globalObject, "Database has closed"_s));
+ return false;
+ }
if (statement && statement->stmt != stmt) [[unlikely]] {
if (!scope.exception())
throwException(globalObject, scope, createError(globalObject, "Statement has finalized"_s));
@@ -1117,7 +1127,7 @@ static JSC::JSValue rebindObject(JSC::JSGlobalObject* globalObject, SQLiteBindin
return jsNumber(count);
}
-static JSC::JSValue rebindStatement(JSC::JSGlobalObject* lexicalGlobalObject, JSC::JSValue values, JSC::ThrowScope& scope, sqlite3* db, sqlite3_stmt* stmt, SQLiteBindingsMap& bindings, bool safeIntegers, JSSQLStatement* statement)
+static JSC::JSValue rebindStatement(JSC::JSGlobalObject* lexicalGlobalObject, JSC::JSValue values, JSC::ThrowScope& scope, sqlite3* db, VersionSqlite3* versionDB, sqlite3_stmt* stmt, SQLiteBindingsMap& bindings, bool safeIntegers, JSSQLStatement* statement)
{
sqlite3_clear_bindings(stmt);
JSC::JSArray* array = dynamicDowncast(values);
@@ -1125,7 +1135,7 @@ static JSC::JSValue rebindStatement(JSC::JSGlobalObject* lexicalGlobalObject, JS
if (!array) {
if (JSC::JSObject* object = values.getObject()) {
- auto res = rebindObject(lexicalGlobalObject, bindings, object, scope, db, stmt, safeIntegers, statement);
+ auto res = rebindObject(lexicalGlobalObject, bindings, object, scope, db, versionDB, stmt, safeIntegers, statement);
RETURN_IF_EXCEPTION(scope, {});
return res;
}
@@ -1154,6 +1164,10 @@ static JSC::JSValue rebindStatement(JSC::JSGlobalObject* lexicalGlobalObject, JS
} else {
value = array->getDirectIndex(lexicalGlobalObject, i);
RETURN_IF_EXCEPTION(scope, {});
+ if (versionDB->db != db) [[unlikely]] {
+ throwException(lexicalGlobalObject, scope, createError(lexicalGlobalObject, "Database has closed"_s));
+ return {};
+ }
if (statement && statement->stmt != stmt) [[unlikely]] {
throwException(lexicalGlobalObject, scope, createError(lexicalGlobalObject, "Statement has finalized"_s));
return {};
@@ -1534,13 +1548,17 @@ JSC_DEFINE_HOST_FUNCTION(jsSQLStatementExecuteFunction, (JSC::JSGlobalObject * l
int count = sqlite3_bind_parameter_count(sql.stmt);
SQLiteBindingsMap bindings { static_cast(count > -1 ? count : 0), strict };
- JSC::JSValue reb = rebindStatement(lexicalGlobalObject, bindingsAliveScope.value(), scope, db, sql.stmt, bindings, safeIntegers, nullptr);
- RETURN_IF_EXCEPTION(scope, {});
-
+ JSC::JSValue reb = rebindStatement(lexicalGlobalObject, bindingsAliveScope.value(), scope, db, versionDB, sql.stmt, bindings, safeIntegers, nullptr);
if (versionDB->db != db) [[unlikely]] {
- throwException(lexicalGlobalObject, scope, createError(lexicalGlobalObject, "Database has closed"_s));
+ // A close() during binding finalized sql.stmt via its
+ // sqlite3_next_stmt() sweep; keep the auto-destructor
+ // from finalizing it again.
+ sql.stmt = nullptr;
+ if (!scope.exception())
+ throwException(lexicalGlobalObject, scope, createError(lexicalGlobalObject, "Database has closed"_s));
return {};
}
+ RETURN_IF_EXCEPTION(scope, {});
if (!reb.isNumber()) [[unlikely]] {
return JSValue::encode(reb); /* this means an error */
@@ -1841,14 +1859,35 @@ JSC_DEFINE_HOST_FUNCTION(jsSQLStatementCloseStatementFunction, (JSC::JSGlobalObj
return JSValue::encode(jsUndefined());
}
- // sqlite3_close_v2 is used for automatic GC cleanup
- int statusCode = shouldThrowOnError ? sqlite3_close(db) : sqlite3_close_v2(db);
+ // Finalize every outstanding statement so sqlite3_close() cannot fail
+ // with SQLITE_BUSY, nulling each wrapper's handle so it never touches
+ // freed memory.
+ for (auto* statement : versionDB->statements) {
+ if (statement->stmt) {
+ sqlite3_finalize(statement->stmt);
+ statement->stmt = nullptr;
+ }
+ }
+ versionDB->statements.clear();
+
+ // Backstop for statements not owned by a wrapper, e.g. the transient one
+ // in jsSQLStatementExecuteFunction when a bound getter calls close().
+ while (sqlite3_stmt* stmt = sqlite3_next_stmt(db, nullptr)) {
+ sqlite3_finalize(stmt);
+ }
+
+ int statusCode = sqlite3_close(db);
if (statusCode != SQLITE_OK) {
+ // The statements are already gone, so the connection is unusable
+ // either way: defer the close and retire the handle.
+ sqlite3_close_v2(db);
+ }
+ versionDB->db = nullptr;
+
+ if (statusCode != SQLITE_OK && shouldThrowOnError) {
throwException(lexicalGlobalObject, scope, createError(lexicalGlobalObject, WTF::String::fromUTF8(sqlite3_errstr(statusCode))));
return {};
}
-
- versionDB->db = nullptr;
return JSValue::encode(jsUndefined());
}
@@ -2494,11 +2533,9 @@ JSC_DEFINE_HOST_FUNCTION(jsSQLStatementExecuteStatementFunctionRawRows, (JSC::JS
RELEASE_AND_RETURN(scope, {});
}
resultArray->push(lexicalGlobalObject, row);
-
- if (scope.exception()) [[unlikely]] {
- sqlite3_reset(stmt);
- RELEASE_AND_RETURN(scope, {});
- }
+ // No sqlite3_reset here: push() can run user code that
+ // finalizes `stmt` (statement.finalize() or db.close()).
+ RETURN_IF_EXCEPTION(scope, {});
if (castedThis->stmt != stmt) [[unlikely]] {
throwException(lexicalGlobalObject, scope, createError(lexicalGlobalObject, "Statement has finalized"_s));
@@ -2844,6 +2881,9 @@ void JSSQLStatement::finishCreation(VM& vm)
JSSQLStatement::~JSSQLStatement()
{
+ if (this->version_db) {
+ this->version_db->statements.remove(this);
+ }
if (this->stmt) {
sqlite3_finalize(this->stmt);
}
@@ -2873,7 +2913,7 @@ JSC::JSValue JSSQLStatement::rebind(JSC::JSGlobalObject* lexicalGlobalObject, JS
auto scope = DECLARE_THROW_SCOPE(vm);
auto* stmt = this->stmt;
- auto val = rebindStatement(lexicalGlobalObject, values, scope, this->version_db->db, stmt, this->m_bindingNames, this->useBigInt64, this);
+ auto val = rebindStatement(lexicalGlobalObject, values, scope, this->version_db->db, this->version_db, stmt, this->m_bindingNames, this->useBigInt64, this);
RETURN_IF_EXCEPTION(scope, {});
// A getter invoked while binding can finalize this statement; the callers
diff --git a/test/js/bun/sqlite/sqlite.test.js b/test/js/bun/sqlite/sqlite.test.js
index 555794e65156..4fd5000fbc6b 100644
--- a/test/js/bun/sqlite/sqlite.test.js
+++ b/test/js/bun/sqlite/sqlite.test.js
@@ -1,7 +1,7 @@
import { spawnSync } from "bun";
import { constants, Database, SQLiteError } from "bun:sqlite";
import { describe, expect, it } from "bun:test";
-import { existsSync, readdirSync, readFileSync, realpathSync, statSync, writeFileSync } from "fs";
+import { existsSync, readdirSync, readFileSync, realpathSync, rmSync, statSync, writeFileSync } from "fs";
import { bunEnv, bunExe, isMacOS, isMacOSVersionAtLeast, isWindows, tempDir } from "harness";
import { tmpdir } from "os";
import path from "path";
@@ -1543,13 +1543,61 @@ it("should close with WAL enabled", () => {
expect(readdirSync(dir).sort()).toEqual(["empty.txt", "my.db"]);
});
-it("close(true) should throw an error if the database is in use", () => {
+it("close(true) finalizes outstanding prepared statements instead of throwing", () => {
const db = new Database(":memory:");
db.exec("CREATE TABLE foo (id INTEGER PRIMARY KEY AUTOINCREMENT, name TEXT)");
db.exec("INSERT INTO foo (name) VALUES ('foo')");
const prepared = db.prepare("SELECT * FROM foo");
- expect(() => db.close(true)).toThrow("database is locked");
- prepared.finalize();
+ expect(() => db.close(true)).not.toThrow();
+ expect(() => prepared.all()).toThrow("Statement has finalized");
+});
+
+it("close(true) finalizes query() statements created after the cache filled up (#36572)", () => {
+ const db = new Database(":memory:");
+ db.exec("CREATE TABLE foo (a INTEGER)");
+ // One more distinct query string than MAX_QUERY_CACHE_SIZE, so the last
+ // statement does not fit in the query cache.
+ for (let i = 0; i <= Database.MAX_QUERY_CACHE_SIZE; i++) {
+ db.query(`SELECT a + ${i} AS v FROM foo`).all();
+ }
+ expect(() => db.close(true)).not.toThrow();
+});
+
+it("close(true) finalizes query() statements past the cache limit that are still referenced (#36572)", () => {
+ const db = new Database(":memory:");
+ db.exec("CREATE TABLE foo (a INTEGER)");
+ const statements = [];
+ for (let i = 0; i <= Database.MAX_QUERY_CACHE_SIZE; i++) {
+ statements.push(db.query(`SELECT a + ${i} AS v FROM foo`));
+ }
+ expect(() => db.close(true)).not.toThrow();
+ for (const stmt of statements) {
+ expect(() => stmt.all()).toThrow("Statement has finalized");
+ }
+});
+
+it("close(true) succeeds after unreferenced query() statements were GC'd (#36572)", () => {
+ const db = new Database(":memory:");
+ db.exec("CREATE TABLE foo (a INTEGER)");
+ for (let i = 0; i <= Database.MAX_QUERY_CACHE_SIZE * 2; i++) {
+ db.query(`SELECT a + ${i} AS v FROM foo`).all();
+ }
+ // Collects the statement wrappers; close(true) must not fail over
+ // statements that are pending sweep.
+ Bun.gc(true);
+ expect(() => db.close(true)).not.toThrow();
+});
+
+it("close(true) works when query() statements past the cache limit were already finalized", () => {
+ const db = new Database(":memory:");
+ db.exec("CREATE TABLE foo (a INTEGER)");
+ const statements = [];
+ for (let i = 0; i <= Database.MAX_QUERY_CACHE_SIZE; i++) {
+ statements.push(db.query(`SELECT a + ${i} AS v FROM foo`));
+ }
+ for (const stmt of statements) {
+ stmt.finalize();
+ }
expect(() => db.close(true)).not.toThrow();
});
@@ -1561,16 +1609,30 @@ it("close() should NOT throw an error if the database is in use", () => {
expect(() => db.close()).not.toThrow("database is locked");
});
-it("should dispose AND throw an error if the database is in use", () => {
+it("close() releases the database file so it can be deleted immediately (#36572)", () => {
+ using dir = tempDir("sqlite-close-unlink", {});
+ const file = path.join(String(dir), "x.sqlite");
+ const db = new Database(file);
+ db.exec("CREATE TABLE t (a INTEGER)");
+ db.prepare("SELECT a FROM t").all();
+ db.close();
+ // On Windows this throws EBUSY if the statement above kept the
+ // connection (and the file handle) open past close().
+ rmSync(file);
+ expect(existsSync(file)).toBe(false);
+});
+
+it("should dispose even if a prepared statement is still live", () => {
+ let prepared;
expect(() => {
- let prepared;
{
using db = new Database(":memory:");
db.exec("CREATE TABLE foo (id INTEGER PRIMARY KEY AUTOINCREMENT, name TEXT)");
db.exec("INSERT INTO foo (name) VALUES ('foo')");
prepared = db.prepare("SELECT * FROM foo");
}
- }).toThrow("database is locked");
+ }).not.toThrow();
+ expect(() => prepared.get()).toThrow("Statement has finalized");
});
it("should dispose", () => {
@@ -1656,7 +1718,7 @@ it("reports changes in Statement#run", () => {
});
it("#13082", async () => {
- async function run() {
+ async function run(op) {
const stmt = (() => {
const db = new Database(":memory:");
let stmt = db.prepare("select 1");
@@ -1666,18 +1728,21 @@ it("#13082", async () => {
Bun.gc(true);
await Bun.sleep(100);
Bun.gc(true);
- stmt.all();
- stmt.get();
- stmt.run();
+ stmt[op]();
}
- const count = 100;
+ const ops = ["all", "get", "run"];
+ const count = 99;
const runs = new Array(count);
for (let i = 0; i < count; i++) {
- runs[i] = run();
+ runs[i] = run(ops[i % ops.length]);
}
- await Promise.allSettled(runs);
+ const results = await Promise.allSettled(runs);
+ for (const result of results) {
+ expect(result.status).toBe("rejected");
+ expect(result.reason.message).toBe("Statement has finalized");
+ }
});
// The internal SQL.run / SQL.prepare / SQL.isInTransaction helpers used to
@@ -1884,6 +1949,57 @@ it("all() reports an error when a result-row push finalizes the statement", asyn
expect(exitCode).toBe(0);
});
+// A result-row push can also close the whole database (which finalizes every
+// statement) and then throw; the raw() loop must not touch the freed stmt.
+// Run in a subprocess because the unsafe variant resets freed memory and the
+// Array.prototype accessor affects every array in the process.
+it("raw() does not touch the statement when a result-row push closes the database and throws", async () => {
+ const src = `
+ const { Database } = require("bun:sqlite");
+ const out = {};
+
+ const db = new Database(":memory:");
+ db.exec("CREATE TABLE t (a INTEGER)");
+ db.run("INSERT INTO t VALUES (1), (2), (3)");
+
+ const stmt = db.query("SELECT a FROM t ORDER BY a ASC");
+ Object.defineProperty(Array.prototype, 0, {
+ configurable: true,
+ get() {
+ return undefined;
+ },
+ set(_row) {
+ db.close();
+ throw new Error("boom");
+ },
+ });
+
+ let message = "did not throw";
+ try {
+ stmt.raw();
+ } catch (e) {
+ message = e.message;
+ }
+ delete Array.prototype[0];
+ out.closeDuringRaw = message;
+
+ console.log(JSON.stringify(out));
+ `;
+
+ await using proc = Bun.spawn({
+ cmd: [bunExe(), "-e", src],
+ env: bunEnv,
+ stdout: "pipe",
+ stderr: "pipe",
+ });
+
+ const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
+
+ expect(stderr).toBe("");
+ expect(stdout.trim()).toBe(JSON.stringify({ closeDuringRaw: "boom" }));
+ expect(exitCode).toBe(0);
+});
+
// Binding an ArrayStorage-backed sparse array whose public length exceeds the
// number of slots in its backing vector must not read JSValues from beyond the
// vector. Holes fall back to the slow indexed lookup and bind as NULL. Run in