Repository navigation
Fix MultiMapAsync disposing the reader before unbuffered results are enumerated - #2229
jinseojang0903 wants to merge 2 commits into
Conversation
…enumerated Both the fixed-arity and Type[]-based MultiMapAsync overloads wrapped the DbDataReader in a `using` that disposed it as soon as the async method returned. For buffered:false calls the returned IEnumerable<TReturn> is a lazy (yield-based) sequence that hasn't started executing yet, so the reader was already closed by the time the caller enumerated it, throwing ObjectDisposedException / "reader is closed" on first access. Mirrors the disposal-deferral pattern already used by the single-type QueryAsync<T> unbuffered path: transfer reader ownership into the returned sequence instead of disposing it unconditionally. Fixes DapperLib#2099 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PPzJgxak5nZ4u3j88ui5xX
fathiaomar
left a comment
There was a problem hiding this comment.
Left a question on line 950 regarding reader disposal and potential connection leaks during partial iteration
| } | ||
| wasClosed = false; // handing back open reader; rely on command-behavior | ||
| var deferred = ExecuteReaderSync(reader, results); | ||
| reader = null; // to prevent it being disposed before the caller gets to see it |
There was a problem hiding this comment.
Setting reader = null prevents immediate disposal so unbuffered results can be streamed, but if the caller abandons or partially enumerates the returned IEnumerable, the reader and underlying connection won't be closed. How are connection leaks handled if iteration stops early?
There was a problem hiding this comment.
Thanks for the question! Early termination is covered: ExecuteReaderSync wraps the reader in a using inside an iterator, so the redder is disposed whenever the enumerator is disposed. That includes break or an exception inside a foreach, and LINQ operators like First(), Take() and Any(). If Dapper opened the connection itself, the reader is created with CommandBehavior.CloseConnection, so disposing the reader also closes the connection.
The only uncovered case is a caller who never enumerates the result, or who calls GetEnumerator() manually and never disposes it. That's the existing contract for buffered: false, and the single-type QueryAsync unbuffered path behaves the same way. This change follows that path exactly. Before this fix, the multi-map unbuffered path threw as soon as you enumerated it, so no new leak is introduced.
The previous GitHub Actions run expired while awaiting workflow approval. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Fixes #2099.
Problem
When calling any multi-map
QueryAsyncoverload (e.g.QueryAsync<TFirst, TSecond, TReturn>or theType[]-basedQueryAsync<TReturn>)with
buffered: false, enumerating the returned sequence throws once the reader has already been disposed:System.InvalidOperationException: Invalid attempt to call FieldCount when reader is closed.
at Dapper.SqlMapper.GetColumnHash(...)
at Dapper.SqlMapper.MultiMapImpl[...]+MoveNext()
Root cause
Both
MultiMapAsync<TFirst,...,TSeventh,TReturn>andMultiMapAsync<TReturn>(Type[] overload) inSqlMapper.Async.cswrapped theDbDataReaderin ausingblock.MultiMapImplbuilds its result as a lazy,yield return-basedIEnumerable<TReturn>that doesn'tstart executing until the caller enumerates it. For
buffered: false, the async method returns that un-enumerated sequence directly — sothe
usingdisposes the reader immediately on return, before the caller ever gets a chance to read from it.The single-type
QueryAsync<T>path already avoids this by deferring disposal: it hands the reader into a small iterator(
ExecuteReaderSync) that only disposes it once the caller finishes enumerating. This PR applies the same pattern to both multi-mapoverloads, adding a matching
IEnumerable<TReturn>-based overload ofExecuteReaderSyncfor them to share.Fix
SqlMapper.Async.cs: transfer reader ownership into the deferred sequence instead of disposing it unconditionally whenbuffered: false.buffered: truepath (already worked correctly, since.ToList()fully consumes the reader before the method returns).Tests
Added two regression tests in
AsyncTests.cs, following the existingTestMultiMapWithSplitAsync/TestMultiMapArbitraryWithSplitAsyncstyle:
TestMultiMapWithSplitUnbufferedAsyncTestMultiMapArbitraryWithSplitUnbufferedAsyncBoth fail with the reported exception before this fix and pass after it. Full suite run locally (net8.0 + net10.0, SQL
Server/MySQL/Postgres) with no regressions.