2

In my repository code, I am getting a disposable instance of Database. I am then using this to return an IAsyncEnumable of my object type. The problem I'm running into is that the database object is being disposed before the enumeration happens at all -- so the connection is closed from under it. What is the pattern to solve this? (if it matters -- which it shouldn't -- this is NPoco).

I am editing the problem to say that it is specific to IAsyncEnumerable such that the awaited row-by-row fetch is more appropriate in this context, as opposed to assembling an entire List of results and returning that at once.

    public IAsyncEnumerable<T_AccountViewProperty> RetrieveManyAsync(AccountViewId input)
    {
        var database = GetDatabase();
        return database.Query<T_AccountViewProperty>()
            .Where(x => x.AccountViewId == (int)input)
            .ToEnumerableAsync()
            .DisposeWhenCompleted(database);     // <-- is this a thing?
    }
Bryan Boettcher
  • 4,412
  • 1
  • 28
  • 49
  • 1
    Does this answer your question? [Why do I need a ToList() to avoid disposed context errors?](https://stackoverflow.com/questions/27510430/why-do-i-need-a-tolist-to-avoid-disposed-context-errors) – gunr2171 Oct 15 '20 at 22:25
  • Also see https://stackoverflow.com/questions/21212822/the-operation-cannot-be-completed-because-the-dbcontext-has-been-disposed-using – gunr2171 Oct 15 '20 at 22:27
  • Maybe this is back to being specific to IAsyncEnumerable then. – Bryan Boettcher Oct 15 '20 at 22:29
  • I don't think it's specific to IAsyncEnumerable - in both cases you're deferring execution of the query, but the execution is happening _after_ you've disposed the database context. – gunr2171 Oct 15 '20 at 22:30
  • It's more the fix has to respect the runtime behavior of IAsyncEnumerable, which has distinct behavior from ToList / IEnumerable prior to introduction of IAsyncEnumerable. – Bryan Boettcher Oct 15 '20 at 22:32
  • There are two important things to keep in mind here: 1. A `using` statement tells the runtime to call `Dispose` on the object as soon as the following statement/block returns control,. 2. `async` methods return immediately. – Powerlord Oct 15 '20 at 22:38
  • @Powerlord -- correct. In older code, we had the ability to buffer with ToList() (not perfect), and inside of bigger frameworks like ASP.NET we can RegisterForDispose instead of using. What can I do at this method level, without having to foreach my own query to `yield return` it inside the RetrieveManyAsync method? – Bryan Boettcher Oct 15 '20 at 22:42
  • I would say, using patterns with service or repositories would solve this by disposing the database connection when disposing the service instance for example... – kapsiR Oct 15 '20 at 22:51
  • see this `GetDatabase()` dont do it. Dont try to cache a database instance otherwise you hit problems you have been getting and more – TheGeneral Oct 15 '20 at 23:02
  • @TheGeneral: I'm not caching the database. That invokes a factory method to get a new instance. – Bryan Boettcher Oct 15 '20 at 23:31

2 Answers2

3

No, DisposeWhenCompleted(database) isn't a thing. But it could be, if you write an extension method for it. For example:

public static async IAsyncEnumerable<T> DisposeWhenCompleted<T>(
    this IAsyncEnumerable<T> source,
    IDisposable disposable)
{
    using (disposable)
    {
        await foreach (T t in source)
        {
            yield return t;
        }
    }
}

That would do just what you want. That said, I find that the using statement is better when used explicitly. The above construct isn't nearly as clear, IMHO, as just putting the using in the method itself, even though the latter is more verbose:

public IAsyncEnumerable<T_AccountViewProperty> RetrieveManyAsync(AccountViewId input)
{
    using (var database = GetDatabase())
    {
        var result = database.Query<T_AccountViewProperty>()
            .Where(x => x.AccountViewId == (int)input)
            .ToEnumerableAsync()

        await foreach (T_AccountViewProperty x in result)
        {
            yield return x;
        }
    }
}

Another nice thing about doing it as above is that it's more efficient, especially when you want to dispose more than one item (if you have multiple items, you can just chain the DisposeWhenCompleted() method calls, but that creates a new iterator for each item that needs disposing).

Just like with regular iterator methods, the C# compiler will ensure when it rewrites the code as a state machine that the implicit finally that the using block creates isn't executed until the iteration has actually completed. And this is in fact also true for regular async methods as well, which like both the original iterator methods and the new async iterator methods, have the same property that the method returns to the caller before the code in the method is actually done.

Which makes sense, since both regular async methods and their iterator counterparts are essentially progeny of the original iterator methods. All three kinds of methods share the same basic concept of rewriting the method into a state machine.

Peter Duniho
  • 68,759
  • 7
  • 102
  • 136
-1

The strategy I ended up going with, from Peter Duniho's answer, was an extension method like this:

public static class TaskExtensions
{
    public static Task<TResult> DisposeAfter<TResult>(this Task<TResult> input, IDisposable disposable)
    {
        TResult ContinuationFunction(Task<TResult> t)
        {
            using (disposable) return t.Result;
        }

        return input.ContinueWith(ContinuationFunction, TaskContinuationOptions.OnlyOnRanToCompletion);
    }
}

which gets used like this:

    public Task<T_Account> RetrieveAsync(AccountId input)
    {
        var database = GetDatabase();
        return database.Query<T_Account>()
            .SingleAsync(x => x.AccountId == (int) input)
            .DisposeAfter(database);
    }

Solving whether "OnlyOnRanToCompletion" is needed is a problem for Future Bryan, but this does work for what I want.

Bryan Boettcher
  • 4,412
  • 1
  • 28
  • 49
  • 1
    You do not want to only dispose of a resource when you successfully complete the operation. That leaks it if there is a failure or cancellation. Also this doesn't properly propagate error/cancellation from the input task to the output task. You need to be *real* careful with `ContinueWith`. It's *very* hard to use properly and you're almost always better off using `await`, and that hold to this instance. – Servy Oct 19 '20 at 16:31