From 4db66fd301cf10cc225b48d17488354175cba9f7 Mon Sep 17 00:00:00 2001 From: Matthew Johnston Date: Tue, 29 Sep 2026 08:21:55 -0500 Subject: [PATCH 1/5] fix: do not block statement cleanup on a busy connection statement_type_destructor took the connection mutex and called sqlite3_finalize. A connection in the busy handler holds that mutex, and SQLite's db mutex, until busy_timeout expires. Cleanup then stalled the process that held the write lock, so the waiting BEGIN IMMEDIATE failed with "database is locked". The destructor now finalizes only when the connection mutex is free. Otherwise it queues the statement. The next locked call, or connection close, finalizes it. Fixes: #360 --- CHANGELOG.md | 2 + c_src/sqlite3_nif.c | 120 +++++++++++++++++++++++++++++++--- mix.exs | 2 +- test/exqlite/sqlite3_test.exs | 101 ++++++++++++++++++++++++++++ 4 files changed, 216 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 66de568..d311545 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,8 @@ ## Unreleased +- fixed: statement cleanup no longer waits on a connection that is inside the busy handler. See [#360](https://github.com/elixir-sqlite/exqlite/issues/360). + ## v0.41.0 - added: `:authorizer` connection option to deny SQL actions on `DBConnection` managed connections via `Exqlite.Sqlite3.set_authorizer/2`. diff --git a/c_src/sqlite3_nif.c b/c_src/sqlite3_nif.c index ff06098..cc1fcd5 100644 --- a/c_src/sqlite3_nif.c +++ b/c_src/sqlite3_nif.c @@ -56,11 +56,25 @@ ErlNifMutex* log_hook_mutex = NULL; // currently defined SQLite action code is SQLITE_RECURSIVE (33). #define AUTHORIZER_DENY_SIZE 64 +// Node for a sqlite3_stmt whose Erlang resource is already gone. +// Allocated at prepare so the statement destructor never allocates. +typedef struct deferred_finalize +{ + sqlite3_stmt* statement; + struct deferred_finalize* next; +} deferred_finalize_t; + typedef struct connection { sqlite3* db; ErlNifMutex* mutex; ErlNifMutex* interrupt_mutex; + + // Guards finalize_head only. + // NOTE: Never hold this across a SQLite call. + ErlNifMutex* finalize_mutex; + deferred_finalize_t* finalize_head; + ErlNifPid update_hook_pid; int authorizer_deny[AUTHORIZER_DENY_SIZE]; @@ -76,8 +90,11 @@ typedef struct statement { connection_t* conn; sqlite3_stmt* statement; + deferred_finalize_t* slot; } statement_t; +static void connection_drain_deferred(connection_t* conn); + static int exqlite_progress_handler(void* arg); static void* @@ -282,6 +299,12 @@ connection_acquire_lock(connection_t* conn) { assert(conn); enif_mutex_lock(conn->mutex); + + // Dropped statements may have been queued while this connection was + // inside a SQLite call. + // + // NOTE: Finalize them before the next SQLite call. + connection_drain_deferred(conn); } static inline void @@ -305,6 +328,43 @@ statement_release_lock(statement_t* statement) connection_release_lock(statement->conn); } +// Caller must hold conn->mutex, unless the connection is not reachable +// from another thread. sqlite3_finalize runs after finalize_mutex is released +// so a statement destructor can enqueue without waiting for finalize. +static void +connection_drain_deferred(connection_t* conn) +{ + deferred_finalize_t* node; + deferred_finalize_t* next; + + if (!conn->finalize_mutex) { + return; + } + + enif_mutex_lock(conn->finalize_mutex); + node = conn->finalize_head; + conn->finalize_head = NULL; + enif_mutex_unlock(conn->finalize_mutex); + + while (node) { + next = node->next; + if (node->statement) { + sqlite3_finalize(node->statement); + } + enif_free(node); + node = next; + } +} + +static void +connection_defer_finalize(connection_t* conn, deferred_finalize_t* slot) +{ + enif_mutex_lock(conn->finalize_mutex); + slot->next = conn->finalize_head; + conn->finalize_head = slot; + enif_mutex_unlock(conn->finalize_mutex); +} + static inline void connection_configure_progress_handler(connection_t* conn) { @@ -484,6 +544,8 @@ exqlite_open(ErlNifEnv* env, int argc, const ERL_NIF_TERM argv[]) conn->db = db; conn->mutex = mutex; conn->interrupt_mutex = NULL; + conn->finalize_mutex = NULL; + conn->finalize_head = NULL; memset(conn->authorizer_deny, 0, sizeof(conn->authorizer_deny)); // Initialize busy handler fields @@ -492,6 +554,12 @@ exqlite_open(ErlNifEnv* env, int argc, const ERL_NIF_TERM argv[]) conn->progress_handler_steps = 1000; conn->callback_env = NULL; + conn->finalize_mutex = enif_mutex_create("exqlite:finalize"); + if (conn->finalize_mutex == NULL) { + enif_release_resource(conn); + return make_error_tuple(env, am_failed_to_create_mutex); + } + conn->interrupt_mutex = enif_mutex_create("exqlite:interrupt"); if (conn->interrupt_mutex == NULL) { enif_release_resource(conn); @@ -683,6 +751,14 @@ exqlite_prepare(ErlNifEnv* env, int argc, const ERL_NIF_TERM argv[]) return make_error_tuple(env, am_out_of_memory); } statement->statement = NULL; + statement->conn = NULL; + statement->slot = enif_alloc(sizeof(deferred_finalize_t)); + if (!statement->slot) { + enif_release_resource(statement); + return make_error_tuple(env, am_out_of_memory); + } + statement->slot->statement = NULL; + statement->slot->next = NULL; enif_keep_resource(conn); statement->conn = conn; @@ -1399,6 +1475,11 @@ connection_type_destructor(ErlNifEnv* env, void* arg) conn->mutex = NULL; } + if (conn->finalize_mutex) { + enif_mutex_destroy(conn->finalize_mutex); + conn->finalize_mutex = NULL; + } + if (conn->interrupt_mutex) { enif_mutex_destroy(conn->interrupt_mutex); conn->interrupt_mutex = NULL; @@ -1411,17 +1492,40 @@ statement_type_destructor(ErlNifEnv* env, void* arg) assert(env); assert(arg); - statement_t* statement = (statement_t*)arg; - statement_acquire_lock(statement); + // Runs on whichever normal scheduler drops the last reference, not as a + // dirty NIF. A connection inside the busy handler holds conn->mutex and + // SQLite's db mutex until busy_timeout expires. Taking either mutex here + // stalls this scheduler and the writer the handler is waiting for. + statement_t* statement = (statement_t*)arg; + connection_t* conn = statement->conn; + deferred_finalize_t* slot = statement->slot; + sqlite3_stmt* sqlite_stmt = statement->statement; + + statement->conn = NULL; + statement->slot = NULL; + statement->statement = NULL; - if (statement->statement) { - sqlite3_finalize(statement->statement); - statement->statement = NULL; + if (!sqlite_stmt) { + if (slot) { + enif_free(slot); + } + } else if (conn && conn->mutex && enif_mutex_trylock(conn->mutex) == 0) { + connection_drain_deferred(conn); + sqlite3_finalize(sqlite_stmt); + enif_mutex_unlock(conn->mutex); + if (slot) { + enif_free(slot); + } + } else if (conn && conn->finalize_mutex && slot) { + slot->statement = sqlite_stmt; + connection_defer_finalize(conn, slot); + } else if (slot) { + enif_free(slot); } - statement_release_lock(statement); - enif_release_resource(statement->conn); - statement->conn = NULL; + if (conn) { + enif_release_resource(conn); + } } int diff --git a/mix.exs b/mix.exs index 9a1441f..b102e4a 100644 --- a/mix.exs +++ b/mix.exs @@ -1,7 +1,7 @@ defmodule Exqlite.MixProject do use Mix.Project - @version "0.41.0" + @version "0.42.0-dev" def project do [ diff --git a/test/exqlite/sqlite3_test.exs b/test/exqlite/sqlite3_test.exs index 4615e90..a1d1afb 100644 --- a/test/exqlite/sqlite3_test.exs +++ b/test/exqlite/sqlite3_test.exs @@ -3,6 +3,7 @@ defmodule Exqlite.Sqlite3Test do alias Exqlite.Sqlite3 alias Exqlite.Sqlite3NIF + doctest Exqlite.Sqlite3 describe ".open/1" do @@ -1648,4 +1649,104 @@ defmodule Exqlite.Sqlite3Test do assert :ok = Sqlite3.cancel(conn) end end + + describe "statement cleanup while another connection is busy" do + test "dropping a statement does not block on the owning connection's busy handler" do + path = Temp.path!() + + try do + {:ok, holder} = Sqlite3.open(path) + {:ok, waiter} = Sqlite3.open(path) + + :ok = Sqlite3.execute(holder, "PRAGMA journal_mode=WAL") + :ok = Sqlite3.execute(holder, "CREATE TABLE t (id INTEGER PRIMARY KEY)") + :ok = Sqlite3.set_busy_timeout(waiter, 3_000) + + parent = self() + + owner = + spawn(fn -> + {:ok, stmt} = Sqlite3.prepare(waiter, "SELECT 1") + send(parent, :prepared) + + receive do + :drop -> :ok + end + + # Exit drops the last reference. The destructor must not wait out + # waiter's busy_timeout. + _ = stmt + end) + + assert_receive :prepared, 1_000 + + :ok = Sqlite3.execute(holder, "BEGIN IMMEDIATE") + + waiter_pid = + spawn(fn -> + result = Sqlite3.execute(waiter, "BEGIN IMMEDIATE") + send(parent, {:waiter, result}) + end) + + assert await_busy(waiter_pid) == :blocked + + drop_ref = Process.monitor(owner) + started = System.monotonic_time(:millisecond) + send(owner, :drop) + + assert_receive {:DOWN, ^drop_ref, :process, ^owner, reason}, 8_000 + drop_ms = System.monotonic_time(:millisecond) - started + + assert reason == :normal + assert drop_ms < 500, "statement cleanup blocked for #{drop_ms}ms" + + :ok = Sqlite3.execute(holder, "COMMIT") + + assert_receive {:waiter, :ok}, 2_000 + + assert :ok = Sqlite3.execute(waiter, "SELECT 1") + assert :ok = Sqlite3.close(waiter) + assert :ok = Sqlite3.close(holder) + after + File.rm(path) + File.rm(path <> "-wal") + File.rm(path <> "-shm") + end + end + end + + defp await_busy(pid) do + deadline = System.monotonic_time(:millisecond) + 2_000 + + Stream.repeatedly(fn -> + Process.sleep(20) + + cond do + System.monotonic_time(:millisecond) > deadline -> + :timeout + + waiter_finished?() -> + :finished + + Process.info(pid, :status) == {:status, :running} -> + Process.sleep(150) + + if Process.info(pid, :status) == {:status, :running} and + not waiter_finished?() do + :blocked + else + :retry + end + + true -> + :retry + end + end) + |> Enum.find(&(&1 != :retry)) + end + + defp waiter_finished? do + {:messages, messages} = Process.info(self(), :messages) + Enum.any?(messages, &match?({:waiter, _}, &1)) + end end From 9d38306036a29dae614eb02ebfad5b3c0101112a Mon Sep 17 00:00:00 2001 From: Matthew Johnston Date: Wed, 30 Sep 2026 08:20:03 -0500 Subject: [PATCH 2/5] fix: finish deferred statement cleanup without blocking `release/2` still took the owning connection mutex. A caller holding the write lock could stall a dirty scheduler for the whole `busy_timeout`, the same failure as a destructor wait. A statement queued during that wait was also left until the next locked call. If it had been stepped, it kept a WAL read mark after the busy call returned. Release now queues instead of waiting, and the unlock path finalizes queued statements before the mutex is free. Fixes: #360 --- CHANGELOG.md | 2 +- c_src/sqlite3_nif.c | 110 ++++++++++++++++++++++++++++---- test/exqlite/sqlite3_test.exs | 114 ++++++++++++++++++++++++++++++++++ 3 files changed, 213 insertions(+), 13 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d311545..8847af9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,7 +2,7 @@ ## Unreleased -- fixed: statement cleanup no longer waits on a connection that is inside the busy handler. See [#360](https://github.com/elixir-sqlite/exqlite/issues/360). +- fixed: statement cleanup no longer waits on a connection that is inside the busy handler. `release/2` defers the same way, and a deferred statement is finalized when that connection unlocks rather than waiting for a later call. See [#360](https://github.com/elixir-sqlite/exqlite/issues/360). ## v0.41.0 diff --git a/c_src/sqlite3_nif.c b/c_src/sqlite3_nif.c index cc1fcd5..5cb70cb 100644 --- a/c_src/sqlite3_nif.c +++ b/c_src/sqlite3_nif.c @@ -56,11 +56,16 @@ ErlNifMutex* log_hook_mutex = NULL; // currently defined SQLite action code is SQLITE_RECURSIVE (33). #define AUTHORIZER_DENY_SIZE 64 -// Node for a sqlite3_stmt whose Erlang resource is already gone. // Allocated at prepare so the statement destructor never allocates. +// statement is set when the Erlang resource is already gone. +// owner is set when release/2 deferred because the connection mutex was held; +// the sqlite3_stmt stays in owner until drain, so an in-flight step can finish. +typedef struct statement statement_t; + typedef struct deferred_finalize { sqlite3_stmt* statement; + statement_t* owner; struct deferred_finalize* next; } deferred_finalize_t; @@ -86,12 +91,12 @@ typedef struct connection ErlNifPid caller_pid; } connection_t; -typedef struct statement +struct statement { connection_t* conn; sqlite3_stmt* statement; deferred_finalize_t* slot; -} statement_t; +}; static void connection_drain_deferred(connection_t* conn); @@ -311,7 +316,31 @@ static inline void connection_release_lock(connection_t* conn) { assert(conn); - enif_mutex_unlock(conn->mutex); + + // Statements may have been queued while this thread was inside SQLite. + // Finalize them before unlocking. Otherwise an idle connection keeps a + // WAL read mark until some later call takes the mutex. + // + // When the queue is empty, unlock conn->mutex before finalize_mutex. + // A destructor can enqueue only while holding finalize_mutex, then + // trylocks conn->mutex. It either observes this thread still holding + // the connection mutex, or it obtains the mutex and drains itself. + if (!conn->finalize_mutex) { + enif_mutex_unlock(conn->mutex); + return; + } + + for (;;) { + connection_drain_deferred(conn); + + enif_mutex_lock(conn->finalize_mutex); + if (conn->finalize_head == NULL) { + enif_mutex_unlock(conn->mutex); + enif_mutex_unlock(conn->finalize_mutex); + return; + } + enif_mutex_unlock(conn->finalize_mutex); + } } static inline void @@ -348,7 +377,17 @@ connection_drain_deferred(connection_t* conn) while (node) { next = node->next; - if (node->statement) { + if (node->owner) { + statement_t* owner = node->owner; + node->owner = NULL; + if (owner->statement) { + sqlite3_finalize(owner->statement); + owner->statement = NULL; + } + // Drops the keep taken when release deferred. The NIF argument + // ref, if any, keeps the destructor from running on this thread. + enif_release_resource(owner); + } else if (node->statement) { sqlite3_finalize(node->statement); } enif_free(node); @@ -758,6 +797,7 @@ exqlite_prepare(ErlNifEnv* env, int argc, const ERL_NIF_TERM argv[]) return make_error_tuple(env, am_out_of_memory); } statement->slot->statement = NULL; + statement->slot->owner = NULL; statement->slot->next = NULL; enif_keep_resource(conn); @@ -1426,14 +1466,56 @@ exqlite_release(ErlNifEnv* env, int argc, const ERL_NIF_TERM argv[]) return make_error_tuple(env, am_invalid_statement); } - statement_acquire_lock(statement); + // Lock the statement's connection, not argv[0]. A caller can pass another + // connection while this one is inside the busy handler. Blocking here stalls + // the dirty scheduler that holds the write lock the handler is waiting for. + connection_t* owner = statement->conn; + if (!owner || !owner->mutex) { + return am_ok; + } + + if (enif_mutex_trylock(owner->mutex) == 0) { + connection_drain_deferred(owner); + if (statement->statement) { + sqlite3_finalize(statement->statement); + statement->statement = NULL; + } + connection_release_lock(owner); + return am_ok; + } - if (statement->statement) { - sqlite3_finalize(statement->statement); - statement->statement = NULL; + // Contended. Do not take statement->statement; the holder may be stepping it + // and will re-read that field. Drain finalizes it after those calls return. + if (!owner->finalize_mutex) { + statement_acquire_lock(statement); + if (statement->statement) { + sqlite3_finalize(statement->statement); + statement->statement = NULL; + } + statement_release_lock(statement); + return am_ok; } - statement_release_lock(statement); + enif_keep_resource(statement); + enif_mutex_lock(owner->finalize_mutex); + if (!statement->statement || !statement->slot) { + enif_mutex_unlock(owner->finalize_mutex); + enif_release_resource(statement); + return am_ok; + } + + deferred_finalize_t* slot = statement->slot; + statement->slot = NULL; + slot->statement = NULL; + slot->owner = statement; + slot->next = owner->finalize_head; + owner->finalize_head = slot; + enif_mutex_unlock(owner->finalize_mutex); + + // The holder may have unlocked after our first trylock. Drain if so. + if (enif_mutex_trylock(owner->mutex) == 0) { + connection_release_lock(owner); + } return am_ok; } @@ -1510,15 +1592,19 @@ statement_type_destructor(ErlNifEnv* env, void* arg) enif_free(slot); } } else if (conn && conn->mutex && enif_mutex_trylock(conn->mutex) == 0) { - connection_drain_deferred(conn); sqlite3_finalize(sqlite_stmt); - enif_mutex_unlock(conn->mutex); if (slot) { enif_free(slot); } + connection_release_lock(conn); } else if (conn && conn->finalize_mutex && slot) { slot->statement = sqlite_stmt; + slot->owner = NULL; connection_defer_finalize(conn, slot); + // If the holder unlocked after our trylock failed, finish the queue now. + if (conn->mutex && enif_mutex_trylock(conn->mutex) == 0) { + connection_release_lock(conn); + } } else if (slot) { enif_free(slot); } diff --git a/test/exqlite/sqlite3_test.exs b/test/exqlite/sqlite3_test.exs index a1d1afb..1734867 100644 --- a/test/exqlite/sqlite3_test.exs +++ b/test/exqlite/sqlite3_test.exs @@ -1713,6 +1713,120 @@ defmodule Exqlite.Sqlite3Test do File.rm(path <> "-shm") end end + + test "releasing a statement does not block on the owning connection's busy handler" do + path = Temp.path!() + + try do + {:ok, holder} = Sqlite3.open(path) + {:ok, waiter} = Sqlite3.open(path) + + :ok = Sqlite3.execute(holder, "PRAGMA journal_mode=WAL") + :ok = Sqlite3.execute(holder, "CREATE TABLE t (id INTEGER PRIMARY KEY)") + :ok = Sqlite3.set_busy_timeout(waiter, 3_000) + + # Prepared on the waiter, released from the lock holder. The connection + # argument must not choose a different mutex. + {:ok, stmt} = Sqlite3.prepare(waiter, "SELECT 1") + + :ok = Sqlite3.execute(holder, "BEGIN IMMEDIATE") + + parent = self() + + waiter_pid = + spawn(fn -> + result = Sqlite3.execute(waiter, "BEGIN IMMEDIATE") + send(parent, {:waiter, result}) + end) + + assert await_busy(waiter_pid) == :blocked + + started = System.monotonic_time(:millisecond) + assert :ok = Sqlite3.release(holder, stmt) + release_ms = System.monotonic_time(:millisecond) - started + + assert release_ms < 500, "statement release blocked for #{release_ms}ms" + + :ok = Sqlite3.execute(holder, "COMMIT") + + assert_receive {:waiter, :ok}, 2_000 + + assert :ok = Sqlite3.execute(waiter, "SELECT 1") + assert :ok = Sqlite3.close(waiter) + assert :ok = Sqlite3.close(holder) + after + File.rm(path) + File.rm(path <> "-wal") + File.rm(path <> "-shm") + end + end + + test "a statement dropped during a busy call does not keep a WAL read mark" do + path = Temp.path!() + + try do + {:ok, holder} = Sqlite3.open(path) + {:ok, waiter} = Sqlite3.open(path) + + :ok = Sqlite3.execute(holder, "PRAGMA journal_mode=WAL") + :ok = Sqlite3.execute(holder, "CREATE TABLE t (id INTEGER PRIMARY KEY)") + :ok = Sqlite3.execute(holder, "INSERT INTO t VALUES (1)") + + parent = self() + + reader = + spawn(fn -> + {:ok, stmt} = Sqlite3.prepare(waiter, "SELECT id FROM t") + {:row, [1]} = Sqlite3.step(waiter, stmt) + send(parent, :stepped) + + receive do + :drop -> :ok + end + + _ = stmt + end) + + assert_receive :stepped, 1_000 + + # A write upgrade of this open read returns SQLITE_BUSY_SNAPSHOT and + # does not enter the busy handler. Hold the connection mutex with a + # long read instead, so the dropped statement has to be queued. + waiter_pid = + spawn(fn -> + result = + Sqlite3.execute( + waiter, + "WITH RECURSIVE c(x) AS (SELECT 1 UNION ALL SELECT x + 1 FROM c WHERE x < 5000000) SELECT max(x) FROM c" + ) + + send(parent, {:waiter, result}) + end) + + assert await_busy(waiter_pid) == :blocked + + drop_ref = Process.monitor(reader) + send(reader, :drop) + assert_receive {:DOWN, ^drop_ref, :process, ^reader, :normal}, 1_000 + + assert_receive {:waiter, :ok}, 5_000 + + # No further call on waiter. Finalize has to have run when the long + # read released the connection mutex. RESTART, unlike PASSIVE, returns + # SQLITE_BUSY while any reader still holds a WAL read mark. + :ok = Sqlite3.set_busy_timeout(holder, 0) + {:ok, checkpoint} = Sqlite3.prepare(holder, "PRAGMA wal_checkpoint(RESTART)") + assert {:row, [0, _, _]} = Sqlite3.step(holder, checkpoint) + :ok = Sqlite3.release(holder, checkpoint) + + assert :ok = Sqlite3.close(waiter) + assert :ok = Sqlite3.close(holder) + after + File.rm(path) + File.rm(path <> "-wal") + File.rm(path <> "-shm") + end + end end defp await_busy(pid) do From 329a94fa14900521f1446d29e1cb20f06ba0ab87 Mon Sep 17 00:00:00 2001 From: Matthew Johnston Date: Wed, 30 Sep 2026 08:30:21 -0500 Subject: [PATCH 3/5] style: flatten await_busy nesting for credo Credo allows two levels of nesting. The poll's cond contained an if, which failed mix lint. --- test/exqlite/sqlite3_test.exs | 44 +++++++++++++++++++---------------- 1 file changed, 24 insertions(+), 20 deletions(-) diff --git a/test/exqlite/sqlite3_test.exs b/test/exqlite/sqlite3_test.exs index 1734867..d4b8696 100644 --- a/test/exqlite/sqlite3_test.exs +++ b/test/exqlite/sqlite3_test.exs @@ -1831,32 +1831,36 @@ defmodule Exqlite.Sqlite3Test do defp await_busy(pid) do deadline = System.monotonic_time(:millisecond) + 2_000 + stream = Stream.repeatedly(fn -> poll_busy(pid, deadline) end) + Enum.find(stream, &(&1 != :retry)) + end - Stream.repeatedly(fn -> - Process.sleep(20) + defp poll_busy(pid, deadline) do + Process.sleep(20) - cond do - System.monotonic_time(:millisecond) > deadline -> - :timeout + cond do + System.monotonic_time(:millisecond) > deadline -> + :timeout - waiter_finished?() -> - :finished + waiter_finished?() -> + :finished - Process.info(pid, :status) == {:status, :running} -> - Process.sleep(150) + waiter_blocked?(pid) -> + :blocked - if Process.info(pid, :status) == {:status, :running} and - not waiter_finished?() do - :blocked - else - :retry - end + true -> + :retry + end + end - true -> - :retry - end - end) - |> Enum.find(&(&1 != :retry)) + defp waiter_blocked?(pid) do + Process.info(pid, :status) == {:status, :running} and still_blocked?(pid) + end + + defp still_blocked?(pid) do + Process.sleep(150) + + Process.info(pid, :status) == {:status, :running} and not waiter_finished?() end defp waiter_finished? do From 470be7c4cd23f4c652bad255cdbc7cfc01464a1b Mon Sep 17 00:00:00 2001 From: Matthew Johnston Date: Fri, 2 Oct 2026 08:45:13 -0500 Subject: [PATCH 4/5] fix: avoid a data race in contended statement release The contended `release/2` path read `statement->statement` while holding `finalize_mutex`. Other releases write that field under `conn->mutex`. Two overlapping releases are enough for the race. `slot` is the field this mutex publishes. The queue decision now uses only that. Drain still finalizes the SQLite statement after the holder returns. --- c_src/sqlite3_nif.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/c_src/sqlite3_nif.c b/c_src/sqlite3_nif.c index 5cb70cb..d9af89c 100644 --- a/c_src/sqlite3_nif.c +++ b/c_src/sqlite3_nif.c @@ -1484,8 +1484,8 @@ exqlite_release(ErlNifEnv* env, int argc, const ERL_NIF_TERM argv[]) return am_ok; } - // Contended. Do not take statement->statement; the holder may be stepping it - // and will re-read that field. Drain finalizes it after those calls return. + // Contended. Blocking here would stall the dirty scheduler that holds + // the write lock. Without a finalize queue, waiting is the only option. if (!owner->finalize_mutex) { statement_acquire_lock(statement); if (statement->statement) { @@ -1496,9 +1496,12 @@ exqlite_release(ErlNifEnv* env, int argc, const ERL_NIF_TERM argv[]) return am_ok; } + // Do not read statement->statement. Another release writes it under + // conn->mutex, and the holder may be stepping it. slot is stolen only + // under finalize_mutex. Drain finalizes after those calls return. enif_keep_resource(statement); enif_mutex_lock(owner->finalize_mutex); - if (!statement->statement || !statement->slot) { + if (!statement->slot) { enif_mutex_unlock(owner->finalize_mutex); enif_release_resource(statement); return am_ok; From 914ff5a146cdff47c739017efd2446e326bc15e2 Mon Sep 17 00:00:00 2001 From: Matthew Johnston Date: Fri, 2 Oct 2026 08:57:32 -0500 Subject: [PATCH 5/5] fix: copy sqlite errors before deferred statement cleanup Unlock finalizes queued statements. Finalizing one left mid-result replaces the connection error with "not an error". These NIFs built that tuple after unlock, so a failed execute could lose the busy message callers match on. Copy the message first, as `step` and `prepare` already do. --- c_src/sqlite3_nif.c | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/c_src/sqlite3_nif.c b/c_src/sqlite3_nif.c index d9af89c..27f36b0 100644 --- a/c_src/sqlite3_nif.c +++ b/c_src/sqlite3_nif.c @@ -651,9 +651,10 @@ exqlite_close(ErlNifEnv* env, int argc, const ERL_NIF_TERM argv[]) if (autocommit == 0) { rc = sqlite3_exec(conn->db, "ROLLBACK;", NULL, NULL, NULL); if (rc != SQLITE_OK) { + ERL_NIF_TERM error = make_sqlite3_error_tuple(env, rc, conn->db); connection_clear_caller(conn); connection_release_lock(conn); - return make_sqlite3_error_tuple(env, rc, conn->db); + return error; } } @@ -670,9 +671,10 @@ exqlite_close(ErlNifEnv* env, int argc, const ERL_NIF_TERM argv[]) enif_mutex_lock(conn->interrupt_mutex); rc = sqlite3_close_v2(conn->db); if (rc != SQLITE_OK) { + ERL_NIF_TERM error = make_sqlite3_error_tuple(env, rc, conn->db); enif_mutex_unlock(conn->interrupt_mutex); connection_release_lock(conn); - return make_sqlite3_error_tuple(env, rc, conn->db); + return error; } conn->db = NULL; enif_mutex_unlock(conn->interrupt_mutex); @@ -719,9 +721,10 @@ exqlite_execute(ErlNifEnv* env, int argc, const ERL_NIF_TERM argv[]) rc = sqlite3_exec(conn->db, (char*)bin.data, NULL, NULL, NULL); if (rc != SQLITE_OK) { + ERL_NIF_TERM error = make_sqlite3_error_tuple(env, rc, conn->db); connection_clear_caller(conn); connection_release_lock(conn); - return make_sqlite3_error_tuple(env, rc, conn->db); + return error; } connection_clear_caller(conn); @@ -1127,11 +1130,14 @@ exqlite_multi_step(ErlNifEnv* env, int argc, const ERL_NIF_TERM argv[]) rows = enif_make_list_cell(env, row, rows); break; - default: + default: { + ERL_NIF_TERM error; sqlite3_reset(statement->statement); + error = make_sqlite3_error_tuple(env, rc, conn->db); connection_clear_caller(conn); connection_release_lock(conn); - return make_sqlite3_error_tuple(env, rc, conn->db); + return error; + } } } @@ -1433,9 +1439,10 @@ exqlite_deserialize(ErlNifEnv* env, int argc, const ERL_NIF_TERM argv[]) memcpy(buffer, serialized.data, size); rc = sqlite3_deserialize(conn->db, (const char*)database_name.data, buffer, size, size, flags); if (rc != SQLITE_OK) { + ERL_NIF_TERM error = make_sqlite3_error_tuple(env, rc, conn->db); sqlite3_free(buffer); connection_release_lock(conn); - return make_sqlite3_error_tuple(env, rc, conn->db); + return error; } connection_release_lock(conn);