WiseLibs
Make transaction work with async callbacks.
Came across this problem when using Drizzle, and found transactions were not actually rolling back.
The issue is that wrapTransaction always assumes the callback is sync, in this day of JS that's often not going to be the case, eg. during the transaction you might want to read a file, make a http request etc, without using blocking methods.
One solution is check if the result is a Promise, if use then & fail to handle the commit / rollback. This should then mean sync requests continue as is, and Promise version is then correctly handled too.
eg.
// Return a new transaction function by wrapping the given function
const wrapTransaction = (apply, fn, db, { begin, commit, rollback, savepoint, release, rollbackTo }) => function sqliteTransaction() {
let before, after, undo;
if (db.inTransaction) {
before = savepoint;
after = release;
undo = rollbackTo;
} else {
before = begin;
after = commit;
undo = rollback;
}
const ok = (r) => {
after.run();
return r;
}
const fail = (ex) => {
if (db.inTransaction) {
undo.run();
if (undo !== rollback) after.run();
}
throw ex;
}
before.run();
try {
const result = apply.call(fn, this, arguments);
if (result instanceof Promise) return result.then(ok).catch(fail)
return ok(result);
} catch (ex) {
fail(ex);
}
};
This has already been discussed and the decision made not to provide such support in:
- #319
- #476
@winghouchan Your kidding right?.
I'm fully aware of the issues of keeping transactions short, this is the case for any ACID db. Lets take another example, let's say you had records, eg. like Invoices and you wanted to clone from 1 DB to another, maybe Posgress etc. You have to make sure during each copy of an Invoice that both DB have a lock while copying (Invoice / lines) etc. Using your transaction, this is not possible, because the other DB uses async.
Of course I can get around this by just manually using my own transaction and avoid yours,. But really?, the mod I proposed has Zero effect on sync callbacks, but the cases were async is a must it will then still work.
Nobody is kidding here. By using async operations within a SQLite transaction, you're locking your entire database for a long time, ruining the performance of anything else that might be going on in your application (e.g., other requests being handled concurrently). Therefore, 99% of the time, it's better to do the necessary async operations before the transaction, if possible.
Most people are not experts in this stuff, so I designed the API of better-sqlite3 to encourage good patterns and discourage patterns that can unexpected kill your application if you don't know what you're doing. On the other hand, if you do know what you're doing, you're totally free to manually handle your own transactions, or write your own helper functions (as you already did in your first post above).
@JoshuaWise
I know how transactions work, anyway it's your project, and it kind of breaks Drizzle integration as it kind of assumes async. It's kind of made me switch to turso now anyway, the blocking nature of this project always bothered me to some extent, and using a HTTP backend that's not good.
@KpjComp to be fair when this project was created almost no one was using sqlite for servers. This practice only started with turso and cloudflare d1 so yes when you're doing an API where you expect more than 10 simultaneous users at once hitting the API, you should use @libsql/client by all means
@capaj Yeah, no problem. The conversion took seconds as Drizzle handles the rest, and the bonus for me it's seems faster, as the Server can now breath. (very important).
One bit of advice on the NPM docs it say's "When is this library not appropriate?", maybe when used in a server environment, as blocking the event loop is a bad idea, even if the queries are only short.
Maybe better-sqlite3 shines for batch console utilities etc.
Well, if you get in trouble, because your transactions / statements keep locking up the event loop, switching to asynchronous code may seem as "the solution", but to me this looks more like a cover up. SQLite3 is fast by nature. And if your tables are fine, your indices are well defined and you know your SQL, it stays fast even wrapped into a NodeJS context. But if you adding more and more layers (or frameworks) that take these things away from you .. well.. it's not a single library to blame.
I can understand that you might not be satisfied with the result of your initial request, but I cannot understand, why you are trying to run down better-sqlite3 just because you could not solve your issues. Now stating this lib might only "shine" in console utilities is just shows some lack of understanding. Might be, that it's not the best to be used within a web server with a high amount of concurrent requests - but maybe SQLite per se and NodeJS are not the best picks here, if things like data migrations are triggered by a simple request.
If you are really doing blocking actions, like reading files or waiting for responses from another server, while holding an expensive resource like a database connection / transaction, there is the issue. No matter in which environment. This is bad design. Especially if the database is SQLite3, which will eventually lock the whole database on certain operations anyway and force somewhat of blocking, serialized operations.
@neoxpert
Again I know how ACID works, and the risks of keep transactions open for a long period, just because something is async doesn't mean you have to keep the transaction open for a long time.
The server environment problem is rather simple, the blocking nature of better-sqlite3 is fine for sync projects, but by definition a Server is not. Even a short SQLite query blocking the event loop for 100ms is a performance hit for a Server. Of course you can mitigate this by spawning a workers but was overkill for my use case. And moving to async version did indeed make my Server more responsive, especially with more users hitting the server.
why you are trying to run down better-sqlite3
I think the project is a good project, and I'm sure it has use cases that it shines in, eg. where the event loop is not a big issue. I suppose I got a bit miffed, because I spent time tracking down the issue I was having, there was no error to indicate transactions were failing, apart from looking at the data. Maybe a check on the callback to make sure it's not a Promise would have helped here. Maybe a if (isPromise(result)) throw ... might have saved me some time, or maybe the Drizzle plugin docs have a warning about this issue. In both cases this is maybe a Drizzle concern anyway, and keeping as is, I agree is maybe the best option, but failing transactions are of course a big problem so hopefully some protection for users in the future certainly would be a good idea. I assume we can agree on that.. :) On a side note, one suggestion is also maybe a transactionCTX variant, so you can be 100% sure a db.* actions are called inside a valid transaction, as it's sometimes easy to drop out of a transaction without noticing, especially new users.
After reviewing all the different issues / discussions related to async vs sync nature of better-sqlite3, I definitely agree with the philosophy of better-sqlite3 generally.
But I am curious what the "best practice" is for handling nested transactions for the purpose of rolling back when "in between" the nested child transactions we may need some async code (not directly related to SQLite transactions)?
Here is some pseudo code:
Wrap in a top level Transaction so we can rollback everything happening within here on error () => {
const resultId = syncronousSqliteTransactionInsert();
someFunctionThatUsesInsertedId(resultId);
await doSomethingAsyncLikeWriteSomethingToACsv();
// Error gets thrown in the following transaction
anotherSyncTransactionInsertThatUsesResultIdFromAboveAndThrowsAnError(resultId);
// Because error was just thrown in previous transaction, I want the first "resultId" transaction to also be rolled back
}
Hopefully this makes sense! But let me know if you need clarification with my weird pseudo code ;-)
Basically I have several transactions that need to happen (synchronously is fine) but I want them all rolled up into a single "parent" transaction so that if anything Errors, I rollback everything. But the problem is "between" the child transactions, I have some async code that needs to run for various reasons (typically for node I/O operations like reading/writing files and/or usage of other libraries that have async modules and I have no control over whether async / sync).
Is really the only option to "truly" stick to the sync api / nature of better-sqlite is to "manually" track for example the resultId from my first transaction above and if another transaction errors, I need to catch and then "manually" undo whatever resultId was?
Thanks in advance!
@GitMurf
typically for node I/O operations like reading/writing files
Unfortunately better-sqlite3, say this cannot happen, end-off no debate. And so fair enough!!. The biggest problem I have with it though, is that it won't catch / throw error if you do break said rule. And that's dangerous, so I moved away from using it, because database integrity for me is my primary concern. Because it was Drizzle, moving over to using libsql was just a few lines of code changes anyway, and it seems faster as blocking code is not that great for nodejs when doing other async operations.
@KpjComp Thanks for your detailed comments. I will also have to switch to another library because of this issue.
Maybe a if (isPromise(result)) throw ... might have saved me some time
Looks like this has been implemented now. I saw the error, and was immediately able to find this issue.
To link it, the relevant code change that disallows Promises in transactions is here: #1364
