Skip to content

Commit

Permalink
Make transaction handling compatible with node-sqlite3-wasm
Browse files Browse the repository at this point in the history
https://github.com/tndrle/node-sqlite3-wasm doesn't have transaction support so I've tried to implement it using SQL statements directly.

@hoelzro do you think this is right? Should we be rolling back the transaction in the finally clause? It would be nice to have tests in this area...

I looked at better-sqlite3's implementation - https://github.com/WiseLibs/better-sqlite3/blob/master/lib/methods/transaction.js
  • Loading branch information
Jermolene committed Feb 22, 2024
1 parent cfbd8ab commit 81b8c91
Showing 1 changed file with 14 additions and 9 deletions.
23 changes: 14 additions & 9 deletions plugins/tiddlywiki/multiwikiserver/modules/sql-tiddler-database.js
Original file line number Diff line number Diff line change
Expand Up @@ -500,16 +500,21 @@ Execute the given function in a transaction, committing if successful but rollin
Calls to this function can be safely nested, but only the top-most call will actually take place in a transaction.
*/
SqlTiddlerDatabase.prototype.transaction = function(fn) {
try {
const alreadyInTransaction = this.transactionDepth > 0;
this.transactionDepth++;
if(alreadyInTransaction) {
return fn();
} else {
return this.db.transaction(fn)();
const alreadyInTransaction = this.transactionDepth > 0;
this.transactionDepth++;
if(alreadyInTransaction) {
return fn();
} else {
try {
this.runStatement(`BEGIN TRANSACTION`);

This comment has been minimized.

Copy link
@hoelzro

hoelzro Feb 24, 2024

Contributor

I think that if we wanted to be super pedantic about it, the BEGIN TRANSACTION should be above the try, since if that statement throws an exception no transaction should be active

This comment has been minimized.

Copy link
@Jermolene

Jermolene Feb 24, 2024

Author Member

Thanks @hoelzro

var result = fn();
this.runStatement(`COMMIT TRANSACTION`);
} catch(e) {
this.runStatement(`ROLLBACK TRANSACTION`);

This comment has been minimized.

Copy link
@hoelzro

hoelzro Feb 24, 2024

Contributor

@Jermolene This seems mostly right to me - but I think you'll want to throw e; after the rollback here!

This comment has been minimized.

Copy link
@Jermolene

Jermolene Feb 24, 2024

Author Member

Thanks @hoelzro

} finally {
this.transactionDepth--;
}
} finally {
this.transactionDepth--;
return result;
}
};

Expand Down

0 comments on commit 81b8c91

Please sign in to comment.