SQLite Async API

[geeksilva97]

Closes #54307

This PR implements an async API for node:sqlite module. So far, it contains a very minimal implementation of exec method, misses some tests, docs and refactoring but it is good enough to share the whole theory I have for it; with that, anybody can share thoughts about it.

• Docs
• Tests

---

Design

On C++ land, I plan to have the Database class determine whether the operations will be asynchronous.

Public API

For the public API, I plan to have classes such as Database, Statement, etc., as counterparts to' DatabaseSync', ' StatementSync', and so on.

[nodejs-github-bot]

Review requested:

• @nodejs/sqlite

[geeksilva97]

Concurrency Control

For concurrency control, SQLite provides a few options. Multi-threaded seems to be a good fit.

Multi-thread. In this mode, SQLite can be safely used by multiple threads provided that no single database connection nor any object derived from database connection, such as a prepared statement, is used in two or more threads at the same time.

We just need a way to guarantee that no two threads will be using connection or statements at the same time. Since we have a threadpool, hence no control over which thread a work will be executed, seems like we need a mutex. Please share your thoughts.

[geeksilva97]

We just need a way to guarantee that no two threads will be using connection or statements at the same time. Since we have a threadpool, hence no control over which thread a work will be executed, seems like we need a mutex. Please share your thoughts.

I wonder if, for the first version of this API if the serialized mode is good enough

Serialized. In serialized mode, API calls to affect or use any SQLite database connection or any object derived from such a database connection can be made safely from multiple threads. The effect on an individual object is the same as if the API calls had all been made in the same order from a single thread. The name "serialized" arises from the fact that SQLite uses mutexes to serialize access to each object.

[codecov]

Codecov Report

❌ Patch coverage is 81.73258% with 97 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.50%. Comparing base (08d966c) to head (5bec78e).
⚠️ Report is 21 commits behind head on main.

Files with missing lines	Patch %	Lines
src/node_sqlite.cc	82.53%	49 Missing and 42 partials ⚠️
src/node_sqlite.h	40.00%	6 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #59109      +/-   ##
==========================================
- Coverage   88.54%   88.50%   -0.05%     
==========================================
  Files         703      703              
  Lines      208291   208690     +399     
  Branches    40170    40244      +74     
==========================================
+ Hits       184430   184691     +261     
- Misses      15865    15961      +96     
- Partials     7996     8038      +42     

Files with missing lines	Coverage Δ
src/node_sqlite.h	81.81% <40.00%> (+1.42%)	⬆️
src/node_sqlite.cc	79.56% <82.53%> (-0.37%)	⬇️

... and 41 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[geeksilva97]

It turns out I was very naive while conceiving this implementation. My approach was to utilize libuv's thread pool for running SQLite operations, as we do for backups.

In a situation where a query calls a user-defined function, authorizer, or aggregation, execution on the thread pool fails because it doesn't have access to an Isolate.

My first idea was to track the defined functions and identify when they are in the query; if functions are present, that query would run in the main thread. Another option would be using worker threads that have their own isolates.

@nodejs/sqlite would you have any clue? Thanks in advance.

[geeksilva97]

It turns out I was very naive while conceiving this implementation. My approach was to utilize libuv's thread pool for running SQLite operations, as we do for backups.

In a situation where a query calls a user-defined function, authorizer, or aggregation, execution on the thread pool fails because it doesn't have access to an Isolate.

My first idea was to track the defined functions and identify when they are in the query; if functions are present, that query would run in the main thread. Another option would be using worker threads that have their own isolates.

@nodejs/sqlite would you have any clue? Thanks in advance.

better-sqlite lets the developer decide, suggesting worker threads https://github.com/WiseLibs/better-sqlite3/blob/master/docs/threads.md

[louwers]

I would consider not adding any async APIs, and instead

1. Allow passing threading mode on DatabaseSync construction.
2. Allow setting the threading mode at runtime.
3. Add an example to the documentation how to use SQLite with worker threads.

The overhead of serializing queries and results probably makes this somewhat of an advanced use case. Defaulting to async for all queries is (probably) a mistake for most applications. Instead it is something you might do for a few expensive queries. I could be wrong though.

[geeksilva97]

I would consider not adding any async APIs, and instead

1. Allow passing threading mode on DatabaseSync construction.
2. Allow setting the threading mode at runtime.
3. Add an example to the documentation how to use SQLite with worker threads.

The overhead of serializing queries and results probably makes this somewhat of an advanced use case. Defaulting to async for all queries is (probably) a mistake for most applications. Instead it is something you might do for a few expensive queries. I could be wrong though.

From the beginning, the idea is not to default all queries to run asynchronously. It's about to add the possibility for async. Like fs and fs/promises

[PhilipTrauner]

having implemented worker thread offloading i can state that there's a significant amount of gnarly lifecycle plumbing involved, especially if one wants to support transactions
if some of that plumbing could live within node.js itself it would be much appreciated
not sure where a line would have to be drawn be to support more advanced use cases (e.g. 1 writer, <n> reader setups) but i'm eager to test stuff out!

[geeksilva97]

having implemented worker thread offloading i can state that there's a significant amount of gnarly lifecycle plumbing involved, especially if one wants to support transactions if some of that plumbing could live within node.js itself it would be much appreciated not sure where a line would have to be drawn be to support more advanced use cases (e.g. 1 writer, <n> reader setups) but i'm eager to test stuff out!

Yeah. I will build a solution based on the authorizer, and let's see what you think about it.

[louwers]

From the beginning, the idea is not to default all queries to run asynchronously. It's about to add the possibility for async.

I think the API should reflect this. If it is named Database then I think a lot of people will just go for that one, thinking fewer keystrokes! And async is better/faster! For reading files, async often makes most sense. Correct me if I am wrong, but I think for SQLite queries, most of the time, the overhead makes async the worse option, even measured in wall clock time on the main thread.

Maybe calling it something like DatabaseAsync, StatementAsync is an option?

Or maybe DatabaseSync can be renamed to Database and we can have Database.execAsync?

[louwers]

Is there a performance penalty for purely single-thread use cases?

If so, would it make sense to allow setting the threading mode at runtime?

[geeksilva97]

I don't know for sure about any performance penalti. But as discusses in the issue, due to the node.js nature, this will work fine for sync. Better-sqlite uses it.

[louwers]

Understood. Thanks! This can be closed.

If anything I think this will speed up single-threaded use cases, because apparently by default SQLite uses internal serialization. Did not test it though.

[mceachen]

(I was going to post this on the main thread, but this is the context that matters, so I'll add it here) (edit: I rewrote for brevity):

Synchronous node:sqlite operations can run while your new async operations are executing. This will cause concurrent access to the same SQLite connection from multiple threads. This violates SQLITE_THREADSAFE=2 requirements and can cause db corruption undefined behavior (I could have sworn I read about this on the how-to-corrupt page but I don't see it now.)

SQLITE_THREADSAFE=2 requires no connection be used by multiple threads simultaneously (docs). Currently, nothing prevents sync methods (main thread) from running while async operations are on the thread pool.

Suggested fix: throw from sync methods when has_running_task_ is true:

diff --git a/src/node_sqlite.cc b/src/node_sqlite.cc
--- a/src/node_sqlite.cc
+++ b/src/node_sqlite.cc
@@ -1257,6 +1257,8 @@ void Database::Prepare(const FunctionCallbackInfo<Value>& args) {
   ASSIGN_OR_RETURN_UNWRAP(&db, args.This());
   Environment* env = Environment::GetCurrent(args);
   THROW_AND_RETURN_ON_BAD_STATE(env, !db->IsOpen(), "database is not open");
+  THROW_AND_RETURN_ON_BAD_STATE(env, db->has_running_task_,
+    "Cannot call sync methods while async operation is running");

Same check needed in Exec() (sync path), Function(), Aggregate(), SetAuthorizer(), and LoadExtension(). Tested locally — works as expected.

(SQLITE_THREADSAFE=1 would also prevent undefined behavior, but the main thread would silently block on SQLite's internal mutex until the async op completes, defeating the purpose of async.)

[BurningEnlightenment]

Thank you for the in-depth examples. I'd like to mention another option (which I thought has been discussed in the issue, but maybe we were talking past each other):
Have one sqlite connection for synchronous access and maintain a pool of (seperate) sqlite3 db connections for asynchronous access. For each asynchronous access one connection from the pool would be exclusively used. The main difficulty with this approach is obviously db.prepare(). One would have to track which db connection owns the prepared statement instance and only dispatch operations from that prepared statement object to its owning db connection. Another drawback of that design is the inherent potential of SQLITE_BUSY failures for concurrent write access.

[mceachen]

Yeah, if you don’t want to cross the streams, you need multiple database instances opened. Rather than transparently managing this magically begins the scenes in a pool, though, I think it may be less surprising to actually push the complexity of managing per -database handles to the user—they can figure out what makes sense in their app, and when one query wedges another, it’s their fault.

[mcollina]

It turns out I was very naive while conceiving this implementation. My approach was to utilize libuv's thread pool for running SQLite operations, as we do for backups.

In a situation where a query calls a user-defined function, authorizer, or aggregation, execution on the thread pool fails because it doesn't have access to an Isolate.

My first idea was to track the defined functions and identify when they are in the query; if functions are present, that query would run in the main thread. Another option would be using worker threads that have their own isolates.

@nodejs/sqlite would you have any clue? Thanks in advance.

The only viable solution is not support custom functions with the async mode. Basically you would need to move the computation back to the main thread, causing a possible deadlock situation.

[geeksilva97]

It turns out I was very naive while conceiving this implementation. My approach was to utilize libuv's thread pool for running SQLite operations, as we do for backups.
In a situation where a query calls a user-defined function, authorizer, or aggregation, execution on the thread pool fails because it doesn't have access to an Isolate.
My first idea was to track the defined functions and identify when they are in the query; if functions are present, that query would run in the main thread. Another option would be using worker threads that have their own isolates.
@nodejs/sqlite would you have any clue? Thanks in advance.

The only viable solution is not support custom functions with the async mode. Basically you would need to move the computation back to the main thread, causing a possible deadlock situation.

That sounds good to me

[geeksilva97]

Here's the state of this PR by now.

• Async API delegates work to the Thread pool
• Classes don't have a Sync postfix (like zlib), this is what is causing most of the conflicts.
• Custom functions, aggregates and authorizer are not allowed, those functions require to be executed in the main thread.
• Each database instance has a queue for async tasks. With that we ensure only no connection or statement will be used in multiple threads at the same time.
• Iterators remains the same as async iterator helpers are still on stage 2 (https://github.com/tc39/proposal-async-iterator-helpers)

cc @mcollina @cjihrig

[geeksilva97]

From the beginning, the idea is not to default all queries to run asynchronously. It's about to add the possibility for async.

I think the API should reflect this. If it is named Database then I think a lot of people will just go for that one, thinking fewer keystrokes! And async is better/faster! For reading files, async often makes most sense. Correct me if I am wrong, but I think for SQLite queries, most of the time, the overhead makes async the worse option, even measured in wall clock time on the main thread.

Maybe calling it something like DatabaseAsync, StatementAsync is an option?

Or maybe DatabaseSync can be renamed to Database and we can have Database.execAsync?

All of them are good options but the argument of consistency is very strong. Looking at other modules with the perspective of what is exported. Zlib exports functions with sync prefix with the function is sync and without it otherwise.

Maybe this can be a turning point?

I saw that @mcollina added the tsc-agenda tag to the PR #61262. Maybe this naming thing can be discusses as well?

[louwers]

I don't find the consistency argument very strong, because as far as I am aware there is no precedent of postfixing a class with *Sync (versus leaving it out for async). It's only functions.

I also don't think creating a separate Database class that allows asynchronous operations is a particularly good API. It makes it harder to switch between async and sync. Due to the overhead workers incur, I expect many applications will want to only use async for a subset of queries (and ideally be able to switch between one or the other without too much fuss).

In addition, we are staying quite close to the SQLite API, which I think it a good thing to do. The current DatabaseSync class has a single SQLite database connection handle, wheras the proposed (async) Database wraps complicated machinery to juggle multiple database connection handles on different threads. I think the naming should reflect this (call it a database pool or something similar) and offer a more constrained API that makes more sense for the underlying multi-threaded implementation.

I wonder what you think about this API and if it would be feasible.

[geeksilva97]

I don't find the consistency argument very strong, because as far as I am aware there is no precedent of postfixing a class with *Sync (versus leaving it out for async). It's only functions.

That's why I mentioned the perspective. If you look at what is exported, they follow this rule.

I wonder what you think about #54307 (comment) and if it would be feasible.

I like your proposal. It brings flexibility, with the async stuff under the pool attribute. I don't think we need a threadingMode though. We can keep all async under pool anyways.

If we all agree we can drop the Sync postfix - as Bun - I think we can do this.

The machinery will still have complexity because that's how it is. Every work must be splitted to run part on thread pool (SQLite stuff) part on main thread (object creation like arrays and objects, and promise resolving).

[louwers]

I don't think we need a threadingMode though.

Agreed. I thought this would make it possible to use single threaded mode for individual database connections, but this is not possible as pointed out by @BurningEnlightenment.

Glad you like it though!

The machinery will still have complexity because that's how it is.

Yes, but this API makes it clear that it not 'just' a SQLite database handle.

[addaleax]

As mentioned before, setting the thread safety build flag to 1/serializable is pretty much guaranteed to be at least as efficient as building our own scheduling mechanism, so I'd remove this code and instead do that

[geeksilva97]

Appreciate your help, @addaleax . I will revisit this implementation and will take your suggestion in consideration as well @louwers 🚀

[mceachen]

If I'm reading this correctly, FinalizeStatements() and DeleteSessions() run before the has_running_task_ check at line 1238. Both call into SQLite (sqlite3_finalize, sqlite3session_delete) accessing connection_ on the main thread, while the thread pool may be executing sqlite3_exec(connection_).

The sqlite3_close_v2 is correctly deferred, but these cleanup calls should be deferred too - either move them after the has_running_task_ check, or into MaybeCloseConnection().