Skip to content

Commit 89a2c0f

Browse files
committed
chore: fix some potential race conditions
1 parent ebf57ae commit 89a2c0f

3 files changed

Lines changed: 25 additions & 7 deletions

File tree

src/context.jl

Lines changed: 13 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -122,16 +122,23 @@ from the pool), ensure all registered sources are applied to it, call `f`,
122122
and release the connection back to the pool if needed.
123123
"""
124124
function _with_conn(f::Function, ctx::QueryContext)
125-
ctx._closed && throw(QueryError("QueryContext has been closed."))
126-
if ctx._pool !== nothing
125+
# Read _closed, _pool, and _conn atomically under the same lock that
126+
# close!(ctx) holds when it nulls them out. This eliminates the TOCTOU
127+
# window where _closed passes but _conn is set to nothing before f(conn).
128+
pool, conn = lock(ctx._lock) do
129+
ctx._closed && throw(QueryError("QueryContext has been closed."))
130+
(ctx._pool, ctx._conn)
131+
end
132+
if pool !== nothing
127133
# Pool path: acquire! already calls _ensure_sources_applied!, which
128134
# reads pool.sources — a mirror of ctx.sources kept in sync by
129135
# register!/deregister!. No secondary source check is needed here.
130-
with_connection(ctx._pool) do conn
131-
f(conn)
136+
with_connection(pool) do c
137+
f(c)
132138
end
133139
else
134-
# Single connection path
135-
f(ctx._conn)
140+
# Single connection path: conn is a local reference captured before
141+
# the lock was released, so a concurrent close! cannot null it out.
142+
f(conn)
136143
end
137144
end

src/query.jl

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -132,7 +132,7 @@ function query(ctx::QueryContext, sql::String, args...; kwargs...)::QueryResult
132132
processed_sql, params = normalise_params(sql, args, kwargs)
133133
_with_conn(ctx) do conn
134134
df, elapsed = _run(conn, ctx.config, processed_sql, params)
135-
QueryResult(df, elapsed, sql)
135+
QueryResult(df, elapsed, processed_sql)
136136
end
137137
end
138138

@@ -241,6 +241,7 @@ end
241241
```
242242
"""
243243
function stream(ctx::QueryContext, sql::String, args...; batch_size::Int=10_000, kwargs...)::Channel{DataFrame}
244+
batch_size > 0 || throw(ArgumentError("batch_size must be a positive integer, got $batch_size"))
244245
processed_sql, params = normalise_params(sql, args, kwargs)
245246
Channel{DataFrame}(2) do ch
246247
_with_conn(ctx) do conn

test/runtests.jl

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,9 @@ global_logger(ConsoleLogger(stderr, Logging.Warn))
5858
@test r.elapsed_ns > 0
5959
@test elapsed_ms(r) >= 0.0
6060
@test DataFrame(r) isa DataFrame
61+
# Named params: stored SQL must use ? placeholders, not :name tokens
62+
r2 = query(ctx, "SELECT :val AS n", val=42)
63+
@test r2.sql == "SELECT ? AS n"
6164
close!(ctx)
6265
end
6366

@@ -217,6 +220,13 @@ global_logger(ConsoleLogger(stderr, Logging.Warn))
217220
end
218221

219222
# ── Streaming ──────────────────────────────────────────────────────────────
223+
@testset "stream batch_size validation" begin
224+
ctx = QueryContext()
225+
@test_throws ArgumentError stream(ctx, "SELECT 1"; batch_size=0)
226+
@test_throws ArgumentError stream(ctx, "SELECT 1"; batch_size=-1)
227+
close!(ctx)
228+
end
229+
220230
@testset "stream in batches" begin
221231
ctx = QueryContext()
222232
execute!(ctx, "CREATE TABLE big AS SELECT generate_series AS n FROM generate_series(1, 100)")

0 commit comments

Comments
 (0)