DapperLib / DapperLib/Dapper

SqlMapper.GridReader.ReadUnbufferedAsync possibly hiding exceptions

Open
#2,092 0 comments 1 reaction 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
C#
Stars
18.4k
Forks
3.7k
Avg merge
5h 8m
Merged PRs (30d)
1

Description

We're currently facing issues with querying multiple results and reading each of them unbuffered.

After investigating the Dapper source code I think we may have some exception that is being hidden by Dapper. Our stack trace reveals an exception being thrown in OnAfterGridAsync which is being called in a finally block. I understand that this block is necessary.

async Task Main()
{
    await foreach (var record in ReadUnbufferedAsync())
    {
        // ...
    }
}

private async Task OnAfterGridAsync()
{
    await Task.Yield();
    throw new InvalidOperationException("OnAfterGridAsync");
}

private async Task<bool> ReadAsync()
{
    await Task.Yield();
    throw new InvalidOperationException("ReadAsync"); // this exception will be hidden by the other
}

private string ConvertTo()
{
    return "Record";
}

public async IAsyncEnumerable<string> ReadUnbufferedAsync()
{
    try
    {
        while (await ReadAsync())
        {
            yield return ConvertTo();
        }
    }
    finally
    {
        await OnAfterGridAsync();
    }
}

However, wouldn't it be possible to apply refactoring to properly propagate exceptions to the call-site?

async Task Main()
{
    await foreach (var record in ReadUnbufferedAsync())
    {
        // ...
    }
}

private async Task OnAfterGridAsync()
{
    await Task.Yield();
    throw new InvalidOperationException("OnAfterGridAsync");
}

private async Task<bool> ReadAsync()
{
    await Task.Yield();
    throw new InvalidOperationException("ReadAsync");
}

private string ConvertTo()
{
    return "Record";
}

public async IAsyncEnumerable<string> ReadUnbufferedAsync()
{
    ExceptionDispatchInfo? exceptionCaught = null;
    
    try
    {
        while (true)
        {
            string record;
            
            try
            {
                var hasNext = await ReadAsync();
                
                if (!hasNext)
                {
                    break;
                }
                
                record = ConvertTo();
            }
            catch (Exception ex)
            {
                exceptionCaught = ExceptionDispatchInfo.Capture(ex);
                
                break;
            }
            
            yield return record;
        }
    }
    finally
    {
        try
        {
            await OnAfterGridAsync();
        }
        catch
        {
            if (exceptionCaught == null)
            {
                throw;
            } // else swallow this exception and propagate the other
        }
        
        exceptionCaught?.Throw();
    }
}

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start in Dapper/SqlMapper.GridReader.Async.cs at the linked ReadUnbufferedAsync implementation, focusing on how OnAfterGridAsync is called during iterator cleanup. Reproduce the provided multiple-exception scenario and determine how the call site should observe the original read failure; done means exceptions are propagated without being unexpectedly hidden.

Written by the indexing model from the issue text.

Assessment

Tech stack
csharp
Domain
database
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
42/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.