[FuPeiJiang]

fixes Segmentation fault

import { Database } from "bun:sqlite";
const db = new Database(":memory:")
const stmt = db.prepare("select 1")
db.close()
console.log(stmt.all()) //succeeds
console.log(stmt.get()) //succeeds
console.log(stmt.run()) //Segmentation fault!! , should be: "Database has closed" or "Cannot use a closed database"

https://github.com/oven-sh/bun/blob/98f9e276b032a139ef341a69e7967824718390bb/src/bun.js/bindings/sqlite/JSSQLStatement.cpp

once you close the db, version_db->db will be nullptr

databases()[dbIndex]->db = nullptr;

stmt.run()

int total_changes_before = sqlite3_total_changes(castedThis->version_db->db);

https://github.com/oven-sh/bun/blob/98f9e276b032a139ef341a69e7967824718390bb/src/bun.js/bindings/sqlite/sqlite3.c

return db->nTotalChange;

---

this also happens when db is garbage collected:

https://github.com/oven-sh/bun/blob/98f9e276b032a139ef341a69e7967824718390bb/src/bun.js/bindings/sqlite/JSSQLStatement.cpp

databases()[index]->db = nullptr;

import { context } from 'esbuild' //"esbuild": "^0.20.2"
import { Database } from "bun:sqlite";

//I wasn't able to get `db` garbage collected without importing `esbuild` and using it
const ctx = await context({
    entryPoints: [],
    outdir: 'www/js',
    bundle: true,
    write: false,
    sourcemap: 'inline',
})

function get_stmt() {
    const db = new Database(":memory:")
    const stmt = db.prepare("select 1")
    return stmt
}
const stmt = get_stmt()

setTimeout(() => {
    console.log(stmt.run());
}, 2000);

---

my changes are to add a CHECK only to the .run method, and not the .all nor the .get method, since they seem to work even if the database is closed

for .run to keep working, sqlite3_total_changes() would have to be skipped, and .changes would have to always be 0, which would potentially break users' if statements depending on this

---

for the garbage collection part, I'm using reference counting on VersionSqlite3.. I don't know of a better solution yet,

[Jarred-Sumner]

Thank you for this. We can make VersionedDb extend from WTF::ThreadSafeRefCounted and that would be more consistent with what we do elsewhere

[Jarred-Sumner]

Confirming I'm able to reproduce this on Windows with the following snippet:

import { Database } from "bun:sqlite";

async function run() {
  const stmt = (() => {
    const db = new Database(":memory:");
    let stmt = db.prepare("select 1");
    db.close();
    return stmt;
  })();
  Bun.gc(true);
  await Bun.sleep(100);
  Bun.gc(true);
  console.log(stmt.all()); //succeeds
  console.log(stmt.get()); //succeeds
  console.log(stmt.run()); //Segmentation fault!! , should be: "Database has closed" or "Cannot use a closed database"
}

run();

[Jarred-Sumner]

@Electroid why isn't CI running the PR?

[FuPeiJiang]

@Jarred-Sumner Thanks for the additional information on Bun.gc(true); and await Bun.sleep(100);

, this would have been really useful, let's hope I can use it one day,

---

make VersionedDb extend from WTF::ThreadSafeRefCounted

that would try to close the database in the destructor, but version_db, which is stored in the databases() array, is never actually destroyed, databases()[dbIndex] = nullptr isn't found, only databases()[dbIndex]->db = nullptr;, which clears the fields, without freeing the struct

the way this is implemented, the array/vector can theoretically grow in size, and all the version_db inside will never be freed,

JSSQLStatement on the other hand, will probably free its reference to version_db, when ~JSSQLStatement() is reached

---

why isn't CI running the PR?

I'm looking at the logs of https://github.com/oven-sh/bun/actions/runs/10240688351?pr=13082
and it seems FAILED: CMakeFiles/bun-debug.dir/src/bun.js/bindings/webcore/Worker.cpp.o has been hit with [clang-analyzer-unix.Malloc,-warnings-as-errors]
you can probably disable it by adding -clang-analyzer-unix.Malloc to CMakeLists.txt

if(BUN_TIDY_ONLY)
    find_program(CLANG_TIDY_EXE NAMES "clang-tidy")
    set(CLANG_TIDY_COMMAND "${CLANG_TIDY_EXE}" "-checks=-*,clang-analyzer-*,-clang-analyzer-webkit.UncountedLambdaCapturesChecker" "--fix" "--fix-errors" "--format-style=webkit" "--warnings-as-errors=*")
    set_target_properties(${bun} PROPERTIES CXX_CLANG_TIDY "${CLANG_TIDY_COMMAND}")
endif()

becomes

if(BUN_TIDY_ONLY)
    find_program(CLANG_TIDY_EXE NAMES "clang-tidy")
    set(CLANG_TIDY_COMMAND "${CLANG_TIDY_EXE}" "-checks=-*,clang-analyzer-*,-clang-analyzer-webkit.UncountedLambdaCapturesChecker,-clang-analyzer-unix.Malloc" "--fix" "--fix-errors" "--format-style=webkit" "--warnings-as-errors=*")
    set_target_properties(${bun} PROPERTIES CXX_CLANG_TIDY "${CLANG_TIDY_COMMAND}")
endif()

[Jarred-Sumner]

Can you add a test?

[7heMech]

What's Bun.gc(true)?

[Jarred-Sumner]

@7heMech schedules the garbage collector manually

[Jarred-Sumner]

Thank you. Superseded by #13105