Repository navigation
stabilization of node:sqlite module #57445
Description
Activity
node:sqliteis already unflagged. I would be supportive of dropping the experimental warning as well.As far as officially stabilizing the module goes, I think there is still some work to do. I think targeting Node v25 in October could be a realistic goal, but there aren't a lot of people working on the effort. Another hurdle is a lack of reviews from collaborators - things move slowly when it takes roughly a week to get changes merged.
Is there a rough list of what needs done before it can be marked stable?
I'd be happy to review or contribute.If it's not stable by v24 could it be backported to 24 (and ideally older versions). One of the reasons I want this stabilized soon is so it can be used by libraries / apps with lower minimum supported node versions.
Another hurdle is a lack of reviews from collaborators - things move slowly when it takes roughly a week to get changes merged...
These are not always readily visible. If there are specific PRs please tag me and I'll be happy to review.
Reacted by Colin Ihrig- changed the title
[-]stabalization of node:sqlite module[/-][+]stabilization of node:sqlite module[/+]on Mar 14, 2025 Is there a rough list of what needs done before it can be marked stable?
Not exactly. There are currently some open issues and PRs. Some (not all) of those need to be resolved. I know that @billywhizz had mentioned some potential performance optimizations as well that I think would be good to explore.
If it's not stable by v24 could it be backported to 24
I don't see it making the v24.0.0 cutoff, but it could likely be backported.
Reacted by Andrew Johnston and Trivikram Kamati can take a look at any work that is outstanding over next few days and can hopefully help with any effort to stabilize this going forward.
Agreed this is a very useful feature, would be nice to have it stable. But please not before async support is added! #54307
@KingSupernova31 I don't think async support should be required to be implemented before the sync API is stable. We just need enough of the async API sketched out to be confident that it meshes with the sync model.
For example, whether DatabaseSync should be renamed Database if we want sync and async access via the same class, or if there will be a new DatabaseAsync class with only async methods.
Reacted by Tobias Petry, Michael Kriese, Isaac King, Adam, 鸣音 and wanton7This issue has been marked as stale due to 90 days of inactivity.
It will be automatically closed in 30 days if no further activity occurs. If this is still relevant, please leave a comment or update it to keep it open.Reacted by Michael KrieseI couldn't find a written list of blockers for node:sqlite stabilization, so I went through the open
sqliteissues and PRs and drafted one below. Corrections and additions welcome.
1. Design decision that can't be deferred
- Settle whether an async API gets a separate class or shares
Database. Per the discussion in sqlite: mark as release candidate #61262 and here, async doesn't need to ship first, but the extension point does need to be decided. - Decide on
DatabaseSync→Database. In February, @louwers, @geeksilva97 and @cjihrig (non-blocking) all indicated the rename made sense, but no PR was opened andmainstill hasDatabaseSync. This is the one item that gets materially harder after stabilization, so it's worth either doing or explicitly closing out. - sqlite: add batched async Database API #62015 (batched async
Database) is at MVP and hasn't moved since May. It still needs docs, and it has an open question aboutv8::Promiselosing async context that could use guidance from someone who knows that area.
Note that as it stands, #62015 adds an async
Databasein contrast to the existingDatabaseSync. So merging that PR would cement the existingDatabaseSyncnaming.What I'd suggest is that we rename
DatabaseSync→Database, but have a separatenode:sqlite/promisesimport path for the async API, consistent with the established convention for APIs likefsandstream.2. Memory safety
- node:sqlite segfaults when db.close() is called from a user-defined function callback during query execution #63180 —
db.close()from a user-defined function segfaults (confirmed-bug). Fixed by sqlite: prevent database close during callbacks #64743. - sqlite: reentrancy into a running statement from a user-defined function is unguarded #65102 - Reentrancy into a running statement is unguarded. Fix in sqlite: reject connection access from authorizer callbacks #65156.
- sqlite: prevent reentrant statement finalization from callbacks #64795 —
deserialize()is unguarded: it callsFinalizeStatements()atsrc/node_sqlite.cc:1909with no callback-depth check, whileclose()alongside it is now covered. Fix in sqlite: reject deserialize() while in a callback #64796. - Unchecked sqlite3 API calls #63311 — unchecked
sqlite3_step()/sqlite3_reset()return values. Fix in sqlite: check sqlite3_step() and sqlite3_reset() results #63319. - sqlite: fix undefined behaviour in
Session::Changeset()#63637 — UB inSession::Changeset(). - sqlite: manage sqlite3_stmt lifetime with RAII #62419 — RAII ownership for
sqlite3_stmt. - sqlite: authorizer callback can modify invoking connection despite SQLite contract #63207 — authorizer callback can reenter the connection, contrary to the
sqlite3_set_authorizer()contract. Fix in sqlite: reject connection access from authorizer callbacks #65156. - sqlite: two reentrancy guard gaps let a callback free the object SQLite is still using #65428 - authorizer callback can free memory SQLite is using.
3. Semantics that become semver-major if deferred
These value-conversion behaviors need to be pinned down:
- sqlite: inconsistent undefined bind to null #61824 —
undefinedbinding is inconsistent as implemented today. sqlite: bind undefined to null #62008 has a fix. - sqlite integer parameters should not be bound as doubles #63826 — integer parameters bound as
double. Currently labeledquestion; worth a decision either way. - sqlite: bind Boolean #62001 (boolean binding,
author ready), sqlite: bind ArrayBuffer #62061 (ArrayBuffer), sqlite: support reading NULL as undefined in result sets #61472 / sqlite:statement.setReadNullAsUndefined()#59457 (NULLasundefined). Each of these adds a conversion path, so nailing down the intended type-mapping table as a whole may be more productive than deciding them one at a time.
Reacted by Trivikram Kamat, Tobias Petry, Henrik Gaßmann and Michael Kriese- Settle whether an async API gets a separate class or shares
w.r.t.
DatabaseSyncI recall that the destructor closes the DB connection if the user forgot to do so. Given thatFileHandlemoved in the opposite direction withDEP0137, it might be worth to align their behaviour in that regard before stabilizing the API.If I have time, I might rebase the async Database PR to the current main commit this weekend.
What is the problem this feature will solve?
I couldn't find any issues tracking the stabalization of the node:sqlite module. Built in sqlite support will remove the need for native modules which complicate build and distribution.
What is the feature you are proposing to solve the problem?
Eventually stabilizing test sqlite
What alternatives have you considered?
No response