Skip to content

fix(sqlite3): run BEGIN, COMMIT and ROLLBACK through exec so a failed COMMIT releases its lock #362

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
just-some-random-pal wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(sqlite3): run BEGIN, COMMIT and ROLLBACK through exec so a failed COMMIT releases its lock #362

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
67 changes: 67 additions & 0 deletions packages/libsql-client/src/__tests__/client.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2048,6 +2048,73 @@ describe("timeout option (local files)", () => {
withoutTimeout.close();
}
});

// A COMMIT that fails with SQLITE_BUSY must not leave the database locked:
// once the failed transaction is rolled back and the reader is gone, other
// connections must be able to write again.
for (const [label, failCommit] of [
[
"transaction commit()",
async (c: libsql.Client) => {
const txn = await c.transaction("deferred");
await txn.execute("INSERT INTO t VALUES (1)");
await expect(txn.commit()).rejects.toBeLibsqlError(
"SQLITE_BUSY",
);
},
],
[
"batch()",
async (c: libsql.Client) => {
await expect(
c.batch(["INSERT INTO t VALUES (1)"], "deferred"),
).rejects.toBeLibsqlError("SQLITE_BUSY");
},
],
[
"migrate()",
async (c: libsql.Client) => {
await expect(
c.migrate([{ sql: "INSERT INTO t VALUES (1)", args: [] }]),
).rejects.toBeLibsqlError("SQLITE_BUSY");
},
],
] as const) {
test(`a COMMIT that fails with SQLITE_BUSY in ${label} releases its lock`, async () => {
const setup = createClient({ url: dbUrl });
await setup.execute("CREATE TABLE t (a)");
setup.close();

const writer = createClient({ url: dbUrl });
const reader = createClient({ url: dbUrl });
try {
// A read transaction holds a SHARED lock, so the writer's COMMIT
// cannot take the EXCLUSIVE lock it needs.
const read = await reader.transaction("read");
await read.execute("SELECT count(*) FROM t");

await failCommit(writer);
await read.rollback();

// Neither the writer's own pool nor a new connection may be left
// blocked by the failed COMMIT.
await writer.execute("INSERT INTO t VALUES (2)");
const other = createClient({ url: dbUrl });
try {
await other.execute("INSERT INTO t VALUES (3)");
const rs = await other.execute(
"SELECT count(*) AS n FROM t",
);
expect(rs.rows[0]["n"]).toStrictEqual(2);
} finally {
other.close();
}
} finally {
writer.close();
reader.close();
}
});
}
});

(isFile ? test : test.skip)("raw error codes", async () => {
Expand Down
49 changes: 36 additions & 13 deletions packages/libsql-client/src/sqlite3.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,7 +218,7 @@ export class ConnectionPool {
// the next borrower would silently inherit it.
if (db.open && db.inTransaction) {
try {
db.prepare("ROLLBACK").run();
executeControl(db, "ROLLBACK");
} catch {
// the connection is unusable; drop it rather than reuse it
closeQuietly(db);
Expand Down Expand Up @@ -339,7 +339,7 @@ export class Sqlite3Client implements Client {
const db = await this.#pool.acquire();
try {
this.#checkUsable(db);
executeStmt(db, transactionModeToBegin(mode), this.#intMode);
executeControl(db, transactionModeToBegin(mode));
const resultSets = [];
for (let i = 0; i < stmts.length; i++) {
try {
Expand Down Expand Up @@ -374,7 +374,7 @@ export class Sqlite3Client implements Client {
throw e;
}
}
executeStmt(db, "COMMIT", this.#intMode);
executeControl(db, "COMMIT");
return resultSets;
} finally {
// `release` rolls back anything still open before reuse
Expand All @@ -388,7 +388,7 @@ export class Sqlite3Client implements Client {
try {
this.#checkUsable(db);
executeStmt(db, "PRAGMA foreign_keys=off", this.#intMode);
executeStmt(db, transactionModeToBegin("deferred"), this.#intMode);
executeControl(db, transactionModeToBegin("deferred"));
const resultSets = [];
for (let i = 0; i < stmts.length; i++) {
try {
Expand Down Expand Up @@ -417,14 +417,21 @@ export class Sqlite3Client implements Client {
throw e;
}
}
executeStmt(db, "COMMIT", this.#intMode);
executeControl(db, "COMMIT");
return resultSets;
} finally {
if (db.inTransaction) {
executeStmt(db, "ROLLBACK", this.#intMode);
try {
if (db.open && db.inTransaction) {
executeControl(db, "ROLLBACK");
}
if (db.open) {
executeStmt(db, "PRAGMA foreign_keys=on", this.#intMode);
}
} finally {
// always give the connection back; `release` drops it if the
// rollback above failed and it is still inside a transaction
this.#pool.release(db);
}
executeStmt(db, "PRAGMA foreign_keys=on", this.#intMode);
this.#pool.release(db);
}
}

Expand All @@ -433,7 +440,7 @@ export class Sqlite3Client implements Client {
const db = await this.#pool.acquire(true);
try {
this.#checkUsable(db);
executeStmt(db, transactionModeToBegin(mode), this.#intMode);
executeControl(db, transactionModeToBegin(mode));
} catch (e) {
this.#pool.release(db);
throw e;
Expand Down Expand Up @@ -601,15 +608,15 @@ export class Sqlite3Transaction implements Transaction {
}
try {
this.#checkNotClosed();
executeStmt(db, "ROLLBACK", this.#intMode);
executeControl(db, "ROLLBACK");
} finally {
this.#settle();
}
}

async commit(): Promise<void> {
try {
executeStmt(this.#getDatabase(), "COMMIT", this.#intMode);
executeControl(this.#getDatabase(), "COMMIT");
} finally {
this.#settle();
}
Expand All @@ -624,7 +631,7 @@ export class Sqlite3Transaction implements Transaction {
// `client.close()` may have closed this connection already, and
// reading `inTransaction` on a closed database aborts the process.
if (db.open && db.inTransaction) {
executeStmt(db, "ROLLBACK", this.#intMode);
executeControl(db, "ROLLBACK");
}
} finally {
this.#settle();
Expand Down Expand Up @@ -816,6 +823,22 @@ function executeMultiple(db: Database.Database, sql: string): void {
}
}

// Runs a transaction-control statement (BEGIN, COMMIT, ROLLBACK). These go
// through `exec` rather than a prepared statement: when a prepared COMMIT
// fails, for example with SQLITE_BUSY because another connection holds a
// SHARED lock, libsql leaves the statement un-reset, and an un-reset statement
// keeps the connection's lock after a successful ROLLBACK and even after
// close(), until garbage collection finalizes it. Every later write from any
// connection then fails with SQLITE_BUSY. `exec` finalizes its statement
// before returning, whether it succeeded or not.
function executeControl(db: Database.Database, sql: string): void {
try {
db.exec(sql);
} catch (e) {
throw mapSqliteError(e);
}
}

function mapSqliteError(e: unknown): unknown {
if (e instanceof Database.SqliteError) {
const extendedCode = e.code;
Expand Down