Skip to content

Fix MultiMapAsync disposing the reader before unbuffered results are enumerated - #2229

Open
jinseojang0903 wants to merge 2 commits into
DapperLib:mainfrom
jinseojang0903:fix/multimapasync-unbuffered-reader-dispose
Open

jinseojang0903 wants to merge 2 commits into
DapperLib:mainfrom
jinseojang0903:fix/multimapasync-unbuffered-reader-dispose

Conversation

@jinseojang0903

Copy link
Copy Markdown

Fixes #2099.

Problem

When calling any multi-map QueryAsync overload (e.g. QueryAsync<TFirst, TSecond, TReturn> or the Type[]-based QueryAsync<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> and MultiMapAsync<TReturn> (Type[] overload) in SqlMapper.Async.cs wrapped the
DbDataReader in a using block. MultiMapImpl builds its result as a lazy, yield return-based IEnumerable<TReturn> that doesn't
start executing until the caller enumerates it. For buffered: false, the async method returns that un-enumerated sequence directly — so
the using disposes 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-map
overloads, adding a matching IEnumerable<TReturn>-based overload of ExecuteReaderSync for them to share.

Fix

  • SqlMapper.Async.cs: transfer reader ownership into the deferred sequence instead of disposing it unconditionally when buffered: false.
  • No change to the buffered: true path (already worked correctly, since .ToList() fully consumes the reader before the method returns).

Tests

Added two regression tests in AsyncTests.cs, following the existing TestMultiMapWithSplitAsync / TestMultiMapArbitraryWithSplitAsync
style:

  • TestMultiMapWithSplitUnbufferedAsync
  • TestMultiMapArbitraryWithSplitUnbufferedAsync

Both 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.

…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 fathiaomar left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a question on line 950 regarding reader disposal and potential connection leaks during partial iteration

Comment thread Dapper/SqlMapper.Async.cs
}
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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

MultiMapAsync disposes reader in unbuffered query

2 participants