subframe7536 / kysely-sqlite-tools

SQLite tools for Kysely, include auto serialize plugin, sqlite wasm / sqlite worker / Tauri sqlite plugin / bun:sqlite dialects
https://subframe7536.github.io/kysely-sqlite-tools/
MIT License
54 stars 2 forks source link

dialect-wasqlite-worker transactions break concurrent requests #10

Open davidisaaclee opened 2 hours ago

davidisaaclee commented 2 hours ago
Patch to demonstrate the issue in the playground (updated to use Kysely directly) ```diff diff --git a/playground/src/modules/utils.ts b/playground/src/modules/utils.ts index 8735016..7ddecd2 100644 --- a/playground/src/modules/utils.ts +++ b/playground/src/modules/utils.ts @@ -1,8 +1,27 @@ -import type { Dialect } from 'kysely' +import { Dialect, Kysely } from 'kysely' import type { InferDatabase } from 'kysely-sqlite-builder/schema' import { SqliteBuilder } from 'kysely-sqlite-builder' import { column, defineTable, useSchema } from 'kysely-sqlite-builder/schema' +// helper to manually resolve promises to test concurrency +export function simpleDeferred(): { + promise: Promise + resolve: (value: T) => void + reject: (reason: any) => void +} { + let resolve: (value: T) => void + let reject: (reason: any) => void + const promise = new Promise((res, rej) => { + resolve = res + reject = rej + }) + return { + promise, + resolve: resolve!, + reject: reject!, + } +} + const tables = { test: defineTable({ columns: { @@ -52,9 +71,69 @@ export async function testDB(dialect: Dialect) { console.error(error) } + await checkTransactionIsolation(dialect) + return db.selectFrom('test').selectAll().execute().then(async (data) => { await db.destroy() console.log(data) return data }) } + +const rollback = Symbol('rollback') + +async function checkTransactionIsolation(dialect: Dialect) { + const db = new Kysely({ dialect }) + + // Check that requests concurrent with transaction do not interfere with the transaction + const innerInsertCompleted = simpleDeferred() // will resolve when inner insert is done + const outerSelectCompleted = simpleDeferred() + const transactionPromise = db.transaction().execute(async (tx) => { + await tx.insertInto('test') + .values({ name: 'from transaction' }) + .execute() + innerInsertCompleted.resolve() + + // Wait for the concurrent SELECT to complete, then rollback + await timeout(outerSelectCompleted.promise) + + throw rollback + }).catch((err) => { + // swallow rollback error + if (err !== rollback) { + throw err + } + }) + + // Wait for the transaction's insert to complete, then SELECT from outside the transaction. + // This should not "see" the row inserted by the transaction. + await timeout(innerInsertCompleted.promise) + const outerSelectPromise = db.selectFrom('test') + .selectAll() + .where('name', '=', 'from transaction') + .execute() + + // I'd expect either: + // - transactionPromise resolves first & rolls back changes; then + // outerSelectPromise resolves finding no rows; or... + // - transactionPromise and outerSelectPromise resolve concurrently (like + // if you used multiple connections? idk), but outerSelectPromise still + // finds no rows because the transaction is isolated + const [outerSelect] = await timeout(Promise.all([ + outerSelectPromise, + transactionPromise, + ])) + + outerSelectCompleted.resolve() + + console.log('Concurrency check result (should be empty):', outerSelect) +} + +function timeout(x: Promise, ms: number = 800): Promise { + return new Promise((resolve, reject) => { + const timer = setTimeout(() => { + reject(new Error('timeout')) + }, ms) + x.then(resolve, reject).finally(() => clearTimeout(timer)) + }) +} ``` When run, this times out at the `Promise.all` – specifically, the `outerSelectPromise` seems to be timing out (i.e. somehow the
simple diagram of tested behavior because reading async code is hard ![Screenshot 2024-11-14 at 4 02 26 PM](https://github.com/user-attachments/assets/efc1eac2-baaa-4f0b-98cf-9ce04d1d2b9b)

When a transaction is in-progress, any other requests (outside of the transaction) performed concurrently never resolve.

I have suspicions that also:

  1. Concurrent requests outside of the transaction end up being part of the transaction (I thought I saw this in a bigger project, but I'm not seeing it in the small repro – this does seem to happen with kysely-sqlite-builder if you look at the edit history of this issue)
  2. Concurrent requests can cause responses to get out of sync with requests in the WaSqliteWorkerDriver – this manifested in my bigger project as an INSERT INTO ... RETURNING complaining about no results, but when I traced the request / response, the response (1) arrived at the worker driver after the Kysely query execution completed, and (2) did include results – hypothesis is that the no results error was raised because the INSERT INTO request thought that some other query response contained the results to its query.

I'm going to continue trying to reproduce these issues in the playground, but the issue in the snippet above seems real enough 🤷

I haven't used any of the other packages in this repo, so I don't know if this is an issue in the other drivers.

I'm motivated to fix this for my own project, but I haven't tried fixing it yet – will PR if I'm able to fix it.

davidisaaclee commented 2 hours ago

I didn't see that the playground uses kysely-sqlite-builder instead of Kysely directly – not sure what the difference in behavior is, so I'm working to update my example to use Kysely.

Updated snippet to use Kysely directly.

subframe7536 commented 1 hour ago

Thanks for reporting and your detailed reproduction. I will look into this issue.