API Review: should IResettableService.ResetStateAsync() return ValueTask

[ajcvickers]

Currently IResettableService.ResetStateAsync() returns ValueTask since the one place we need an async path is in RelationalConnection where we use it to async dispose the DbConnection and the DbTransaction:

public virtual async ValueTask DisposeAsync()
{
    if (CurrentTransaction != null)
    {
        await CurrentTransaction.DisposeAsync();
    }
    ClearTransactions(clearAmbient: true);
    if (_connectionOwned
        && _connection != null)
    {
        await DbConnection.DisposeAsync();
        _connection = null;
        _openedCount = 0;
    }
}

So there are no async paths that this uses that call any Task returning async methods.

[ajcvickers]

Make it Task

[roji]

Note that I checked, and ValueTask.AsTask() doesn't allocate if the ValueTask is completed (it just returned Task.CompletedTask. So there's really no loss of perf in returning Task here instead of ValueTask, and a better API being exposed.

[roji]

Note: #16761 also adds a cancellation token to IResettableService - resetting isn't the same as disposing, so it seems like the general logic of having a token should hold (and it also calms down our API consistency tests).

/cc @divega

[divega]

@roji is cancelling the reset meaningful?

[roji]

I admit I only added this after the API consistency tests raised an error, and I hesitated about this for a while. The general practice seems to be to always allow async operations to be cancellable by default, except for some very specific cases, such as the DisposeAsync pit of failure. That specific pit of failure doesn't seem to exist here, so there's at least no strong reason not to make it cancellable.

Here's a (very) theoretical case where this could be useful. We currently reset DbContext's services sequentially, one by one. This in theory could be done in parallel with Task.WhenAll (although if services are expected to use the same connection resource, that would be a big bug). In this scenario, if one of the services throws an exception (i.e. failed to reset), we should issue cancellations on the others before discarding the poolable context.

Basically it's about IResettableService being a public abstraction, so we're not sure exactly where it will end up. If you have strong feelings against this (especially if you see some pit of failure) we can revert.