From 399555790cd86cebaae2203d1b02b03a438a2b7e Mon Sep 17 00:00:00 2001 From: Dallas Hoffman Date: Sun, 15 Sep 2024 22:45:45 -0400 Subject: [PATCH] Fix deadlock when transaction starts during another transaction --- src/processor.ts | 63 +++++++++++++++------------- test/get-database-file.test.ts | 2 +- test/init.test.ts | 2 +- test/overwrite-database-file.test.ts | 4 +- test/sql.test.ts | 2 +- test/transaction.test.ts | 50 +++++++++++++++++++++- vite.config.ts | 3 ++ 7 files changed, 91 insertions(+), 35 deletions(-) diff --git a/src/processor.ts b/src/processor.ts index e432d94..f820b78 100644 --- a/src/processor.ts +++ b/src/processor.ts @@ -73,6 +73,8 @@ export class SQLocalProcessor { `The origin private file system is not available, so ${databasePath} will not be persisted. Make sure your web server is configured to use the correct HTTP response headers (See https://sqlocal.dallashoffman.com/guide/setup#cross-origin-isolation).` ); } + + this.userFunctions.forEach(this.initUserFunction); } catch (error) { this.emitMessage({ type: 'error', @@ -81,11 +83,9 @@ export class SQLocalProcessor { }); this.destroy(); - return; + } finally { + await this.initMutex.unlock(); } - - this.userFunctions.forEach(this.initUserFunction); - await this.initMutex.unlock(); }; postMessage = async ( @@ -142,17 +142,6 @@ export class SQLocalProcessor { ): Promise => { if (!this.db) return; - const partOfTransaction = - (message.type === 'transaction' && - (this.transactionKey === null || - message.transactionKey === this.transactionKey)) || - (message.type === 'query' && - message.transactionKey === this.transactionKey); - - if (!partOfTransaction) { - await this.transactionMutex.lock(); - } - try { const response: DataMessage = { type: 'data', @@ -162,17 +151,35 @@ export class SQLocalProcessor { switch (message.type) { case 'query': - const statementData = execOnDb(this.db, message); - response.data.push(statementData); + const partOfTransaction = + this.transactionKey !== null && + this.transactionKey === message.transactionKey; + + try { + if (!partOfTransaction) { + await this.transactionMutex.lock(); + } + const statementData = execOnDb(this.db, message); + response.data.push(statementData); + } finally { + if (!partOfTransaction) { + await this.transactionMutex.unlock(); + } + } break; case 'batch': - this.db.transaction((tx: Sqlite3Db) => { - for (let statement of message.statements) { - const statementData = execOnDb(tx, statement); - response.data.push(statementData); - } - }); + try { + await this.transactionMutex.lock(); + this.db.transaction((tx: Sqlite3Db) => { + for (let statement of message.statements) { + const statementData = execOnDb(tx, statement); + response.data.push(statementData); + } + }); + } finally { + await this.transactionMutex.unlock(); + } break; case 'transaction': @@ -182,7 +189,11 @@ export class SQLocalProcessor { this.db.exec({ sql: 'BEGIN' }); } - if (message.action === 'commit' || message.action === 'rollback') { + if ( + (message.action === 'commit' || message.action === 'rollback') && + this.transactionKey !== null && + this.transactionKey === message.transactionKey + ) { const sql = message.action === 'commit' ? 'COMMIT' : 'ROLLBACK'; this.db.exec({ sql }); this.transactionKey = null; @@ -199,10 +210,6 @@ export class SQLocalProcessor { queryKey: message.queryKey, }); } - - if (!partOfTransaction) { - await this.transactionMutex.unlock(); - } }; protected getDatabaseInfo = async ( diff --git a/test/get-database-file.test.ts b/test/get-database-file.test.ts index 2acf1a4..01ae0e7 100644 --- a/test/get-database-file.test.ts +++ b/test/get-database-file.test.ts @@ -33,6 +33,6 @@ describe('getDatabaseFile', () => { it('should not throw when requested database has not been created', async () => { const { getDatabaseFile } = new SQLocal('blank.sqlite3'); - expect(getDatabaseFile()).resolves.not.toThrow(); + await expect(getDatabaseFile()).resolves.not.toThrow(); }); }); diff --git a/test/init.test.ts b/test/init.test.ts index 1e69b08..1b93677 100644 --- a/test/init.test.ts +++ b/test/init.test.ts @@ -39,7 +39,7 @@ describe('init', () => { const write = async () => { await sql`INSERT INTO nums (num) VALUES (1)`; }; - expect(write).rejects.toThrowError( + await expect(write).rejects.toThrowError( 'SQLITE_IOERR_WRITE: sqlite3 result code 778: disk I/O error' ); diff --git a/test/overwrite-database-file.test.ts b/test/overwrite-database-file.test.ts index 7410756..c449755 100644 --- a/test/overwrite-database-file.test.ts +++ b/test/overwrite-database-file.test.ts @@ -22,8 +22,8 @@ describe('overwriteDatabaseFile', async () => { await db1.overwriteDatabaseFile(db2File); const letters = db1.sql`SELECT * FROM letters`; - expect(letters).rejects.toThrow(); + await expect(letters).rejects.toThrow(); const nums = db1.sql`SELECT * FROM nums`; - expect(nums).resolves.toEqual([{ num: 1 }, { num: 2 }, { num: 3 }]); + await expect(nums).resolves.toEqual([{ num: 1 }, { num: 2 }, { num: 3 }]); }); }); diff --git a/test/sql.test.ts b/test/sql.test.ts index f7e42ef..ee6c564 100644 --- a/test/sql.test.ts +++ b/test/sql.test.ts @@ -33,7 +33,7 @@ describe('sql', () => { const multiSelect2 = async () => await sql`SELECT * FROM groceries WHERE id = ${3}; SELECT * FROM groceries WHERE id = ${2};`; - expect(multiSelect2).rejects.toThrow(); + await expect(multiSelect2).rejects.toThrow(); const delete1 = await sql`DELETE FROM groceries WHERE id = 2 RETURNING *`; expect(delete1).toEqual([{ id: 2, name: 'milk' }]); diff --git a/test/transaction.test.ts b/test/transaction.test.ts index 5ac41f7..bf2d11e 100644 --- a/test/transaction.test.ts +++ b/test/transaction.test.ts @@ -3,7 +3,7 @@ import { SQLocal } from '../src/index.js'; import { sleep } from './test-utils/sleep.js'; describe('transaction', () => { - const { sql, transaction } = new SQLocal('transaction-test.sqlite3'); + const { sql, batch, transaction } = new SQLocal('transaction-test.sqlite3'); beforeEach(async () => { await sql`CREATE TABLE groceries (id INTEGER PRIMARY KEY AUTOINCREMENT, name TEXT NOT NULL)`; @@ -50,7 +50,7 @@ describe('transaction', () => { expect(selectData.length).toBe(0); }); - it('should isolate transaction mutations', async () => { + it('should isolate transaction mutations from outside queries', async () => { const order: number[] = []; await Promise.all([ @@ -73,4 +73,50 @@ describe('transaction', () => { expect(data).toEqual([{ name: 'x' }, { name: 'x' }]); expect(order).toEqual([1, 2, 3]); }); + + it('should isolate transaction mutations from outside batch queries', async () => { + const order: number[] = []; + + await Promise.all([ + transaction(async (tx) => { + order.push(1); + await tx.sql`INSERT INTO groceries (name) VALUES ('a')`; + await sleep(200); + order.push(3); + await tx.sql`INSERT INTO groceries (name) VALUES ('b')`; + }), + (async () => { + await sleep(100); + order.push(2); + await batch((sql) => [sql`UPDATE groceries SET name = 'x'`]); + })(), + ]); + + const data = await sql`SELECT name FROM groceries`; + + expect(data).toEqual([{ name: 'x' }, { name: 'x' }]); + expect(order).toEqual([1, 2, 3]); + }); + + it('should complete concurrent transactions', async () => { + const transactions = Promise.all([ + transaction(async (tx) => { + await tx.sql`INSERT INTO groceries (name) VALUES ('a') RETURNING name`; + await sleep(100); + return await tx.sql`SELECT name FROM groceries`; + }), + transaction(async (tx) => { + await sleep(50); + return await tx.sql`INSERT INTO groceries (name) VALUES ('b') RETURNING name`; + }), + ]); + + await expect(transactions).resolves.toEqual([ + [{ name: 'a' }], + [{ name: 'b' }], + ]); + + const data = await sql`SELECT name FROM groceries`; + expect(data).toEqual([{ name: 'a' }, { name: 'b' }]); + }); }); diff --git a/vite.config.ts b/vite.config.ts index cf1c91a..d573010 100644 --- a/vite.config.ts +++ b/vite.config.ts @@ -3,6 +3,9 @@ import { defineConfig } from 'vite'; export default defineConfig({ test: { + testTimeout: 1000, + hookTimeout: 1000, + teardownTimeout: 1000, includeTaskLocation: true, browser: { enabled: true,