-
Notifications
You must be signed in to change notification settings - Fork 4.7k
node:sqlite: implement the module and pass the Node v26.3.0 test suite #32498
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 58 commits
4a170f5
f929f05
dec6507
0eddfab
8a18929
b3800ae
bf2650c
a68db4a
83a0f85
2a2c0c7
7716574
b2f0425
25f9045
12e685d
f3d95f1
250fc76
a1287ce
4e8e24a
a44b9b0
2d2a796
96daac9
4e1d327
80d8d33
9e04f81
a1c79fb
5eb0d95
4f8df1c
f255534
7b655ae
8b4565d
78f8f22
266d9c3
9fc9e16
a1ee62a
0ebf660
3ce63f3
b2bb72b
b7b5fbd
696d0ab
d826e92
54a50e8
f62f0f2
ff63622
72cfee9
d3636ac
7da13ac
b6ae651
e996adb
1bbe87a
b7140d3
6c723c7
a46b1f8
f79754b
37a6f6c
6cab772
90025c2
4481f87
7a6477b
40d501d
0b75f9c
9d9c72e
fc4aad1
5fafbd8
88c72b4
7960275
ed378a6
4d19528
13bdb62
dde35fb
90c2be6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -279,8 +279,16 @@ extern "C" void Bun__closeAllSQLiteDatabasesForTermination() | |
| auto& dbs = _instance->databases; | ||
|
|
||
| for (auto& db : dbs) { | ||
| if (db->db) | ||
| sqlite3_close(db->db); | ||
| if (db->db) { | ||
| // close_v2: with unfinalized statements still alive, plain | ||
| // sqlite3_close() returns SQLITE_BUSY and leaves the connection | ||
| // open, which would leak it once the pointer is nulled below. | ||
| sqlite3_close_v2(db->db); | ||
| // Prevent VersionSqlite3::release() (invoked later by the GC | ||
| // finalizer during VM teardown) from closing the same handle | ||
| // again, which would be a use-after-free. | ||
| db->db = nullptr; | ||
| } | ||
| } | ||
| } | ||
|
Comment on lines
279
to
293
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Nit: this exit handler now closes Extended reasoning...What
By default Bun does not destroy the VM on exit — Why it's an inconsistency this hunk surfacesThis PR (a) introduces Step-by-step proofconst { DatabaseSync } = require('node:sqlite');
const db = new DatabaseSync('/tmp/test.db');
db.exec('PRAGMA journal_mode = WAL');
db.exec('CREATE TABLE IF NOT EXISTS t(x); INSERT INTO t VALUES (1)');
// no db.close(); script ends naturally
Under Node.js (natural exit, not Under Bun with ImpactLow. Not memory-unsafe (process is exiting; OS reclaims fds). Not data loss — SQLite recovers the WAL on the next open of the database. The observable effect is leftover Suggested fixMaintain a per-VM (or per-
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed it's a real parity gap (Node closes DatabaseSync handles on environment teardown, so a clean exit checkpoints WAL and removes the -wal/-shm sidecars; Bun's default exit path doesn't destruct the VM, so unclosed node:sqlite handles never get sqlite3_close_v2). Deferring it from this PR as you suggest — it needs a per-VM registry of open JSDatabaseSync handles walked from the exit handler with the same worker gating, which deserves its own change and tests; noting it in the PR description as a known follow-up. No data loss in the meantime (SQLite recovers the WAL on next open). |
||
|
|
||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.