TryGhost / node-sqlite3

SQLite3 bindings for Node.js
BSD 3-Clause "New" or "Revised" License
6.23k stars 817 forks source link

Not safe for use concurrently from multiple async scopes #1761

Open segevfiner opened 9 months ago

segevfiner commented 9 months ago

Issue Summary

If you use a single Database from multiple different async contexts, such as handling two requests concurrently in a web server, and both use transactions, the transactions will get interleaved, meaning you might end up trying to start another transaction while one is already ongoing in the connection, which will error, or you will end up throwing in a random query into an unrelated transaction started by a different request.

To use the library safely concurrently with one Database, there needs to be a mechanism to "lock" the database while one context is performing a transaction, or any other action that effects the connection state, which will block other queries/transaction from other contexts while that lock is held. (Might be possible to achieve using the builtin db lock of SQLite3, the API level one sqlite_db_mutex, not the database level one), and of course, such a lock needs to use callbacks/async to not actually block Node.js.

serialize is not enough, as other concurrent code can still use the DB, that is, end up queueing queries.

This even tripped up TypeORM https://github.com/typeorm/typeorm/issues/1884, and IMHO makes the library unusable like this without connection pooling or some other external locking mechanism.

Steps to Reproduce

Run multiple transactions concurrently, e.g. From multiple web requests.

Version

5.1.7

Node.js Version

18.19.0

How did you install the library?

pnpm 8 on macOS 14.3.1

thebinarysearchtree commented 9 months ago

My ORM handles this properly because I actually know how sqlite works. Each transaction must have its own database connection.

https://github.com/thebinarysearchtree/flyweight#transactions-and-concurrency

segevfiner commented 8 months ago

Qn API like https://github.com/WiseLibs/better-sqlite3/blob/master/docs/api.md#transactionfunction---function can help, by correctly locking the DB around the execution of the async callback inside.

thebinarysearchtree commented 8 months ago

SQLite is designed to allow you to continue querying the database during a transaction as long as you use a different connection.

segevfiner commented 8 months ago

Yes. I know. SQlite3 has file locking in place. Including handling for multiple connections in the same process with the weird semantics of fcntl.

But it can sometimes be more convenient to just use a single connection when concurrency isn't high, rather than a full blown connection pool, especially when using a memory database, where opening multiple connections to one is more involved.