runtime: Add missing async methods in System.Data.Common and implement IAsyncDisposable

Provider implementation tracking issues

Specification

This issue consolidates dotnet/runtime#18225, dotnet/runtime#24244.

There are several APIs in System.Data.Common which possibly involve I/O, where we’re missing async APIs altogether. This proposal adds these missing APIs, with default implementations that call the sync counterparts - much like the existing async methods on those APIs. In addition, all System.Data.Common types that implement IDisposable would be updated to implement IAsyncDisposable as well.

Note that in other cases async methods already exist but return Task, and we’d like to add overloads returning ValueTask. This is out of scope for this issue (but we’ll work on these soon, see dotnet/runtime#19858, dotnet/runtime#25297).

This would ideally go into .NET Standard 2.1.

Proposal:

class DbConnection : IAsyncDisposable
{
    protected virtual ValueTask<DbTransaction> BeginDbTransactionAsync(IsolationLevel isolationLevel, CancellationToken cancellationToken);

    // The following delegate to BeginDbTransactionAsync, like their sync counterparts
    public ValueTask<DbTransaction> BeginTransactionAsync(CancellationToken cancellationToken = default);
    public ValueTask<DbTransaction> BeginTransactionAsync(IsolationLevel isolationLevel, CancellationToken cancellationToken = default);

    public virtual Task ChangeDatabaseAsync(string databaseName, CancellationToken cancellationToken = default);
    public virtual Task CloseAsync(CancellationToken cancellationToken = default);
    public virtual ValueTask DisposeAsync();
}

class DbTransaction : IAsyncDisposable
{
    public virtual Task CommitAsync(CancellationToken cancellationToken = default);
    public virtual Task RollbackAsync(CancellationToken cancellationToken = default);
    public virtual ValueTask DisposeAsync();
}

class DbCommand : IAsyncDisposable
{
    public virtual Task PrepareAsync(CancellationToken cancellationToken = default);
    public virtual ValueTask DisposeAsync();
}

class DbDataReader : IAsyncDisposable
{
    public virtual Task CloseAsync(CancellationToken cancellationToken = default);
    public virtual ValueTask DisposeAsync();
}

Note that this is only about adding async methods where non currently exist. dotnet/runtime#25297 is about adding ValueTask overloads where Task-returning async methods already exist (and where we have to think about naming etc.).

Returning Task vs. ValueTask

For the return value of the new proposed methods, our current guiding principle has been to opt for Task when the method in question represents a full database roundtrip operation, where any perf advantages of ValueTask are likely to be dominated by networking etc. Thus, method such as CommitAsync() and RollbackAsync() would return Task (aside from them having no return value).

Methods used for processing a resultset being streamed do seem like they’d benefit from returning ValueTask; examples include DbDataReader.ReadAsync(), DbDataReader.GetFieldValueAsync<T>(), which are out of scope of this proposal.

Cases such as DbConnection.CloseAsync() and DbDataReader.CloseAsync() seem to be between the two and we could probably go either way with them.

It would definitely be good to get some input on this.

Cancellation token overloads

Existing async methods in System.Data.Common come in pairs - a virtual one accepting CancellationToken and a non-virtual one delegating to the first. For the new methods we can just add one method with cancellation token defaulting to CancellationToken.None.

/cc @divega @ajcvickers @terrajobst @stephentoub @bgrainger

Edit history

Date Modification
16/4 DbConnection.Open() returns Task instead of ValueTask as per @terrajobst’s question
18/4 Added DbDataReader.Get{Stream,TextReader}Async()
18/4 Removed DbDataReader.Get{Stream,TextReader}Async() again (GetFieldValueAsync<T>()) already provides this functionality)
18/4 Removed DbCommand.CancelAsync()
15/5 Changed BeginTransactionAsync() overloads to return ValueTask instead of Task, and changed to use the standard ADO.NET API pattern, as per design review. Changed DbDataReader.Close() to return Task instead of ValueTask.

About this issue

  • Original URL
  • State: closed
  • Created 5 years ago
  • Reactions: 4
  • Comments: 36 (35 by maintainers)

Commits related to this issue

Most upvoted comments

@stephentoub mentioned that he had a flowchart about decision making on ValueTask which might be helpful to review.

If we use ValueTask<T> in the interface definition and there are providers which can provide synchronous results (in memory databases providers like SqLite in some modes) can run fast and avoid the task allocation and it makes sense to enable that scenario where it can be of benefit.

For the rest of the big old out of process providers we’re never going to return synchronously as we’ll always end up with a .AsTask call and wasting a reference worth of copy time, but really we’re going to have to allocate a TaskCompletionSource and some sort of object to hold state and perhaps more async support machinery so we can’t get a low alloc path anyway. So do we choose to prevent the faster path being available to everyone for the sake of avoiding a struct copy in a common case?

The existing async methods were defined before ValueTask<T> was available and if it had been available at the time i think the same discussion would have occured. In this case because we can’t preclude a provider being able to give synchrnous results we shouldn’t prevent that possibility at the api layer IMO.

+2
Wraith2 on May 16, 2019

@benaadams my statement above was about DbDataReader.{Close,Dispose}(), not DbConnection.

Basically:

  1. DbConnection can be opened and closed multiple times. This makes Close() and Dispose() different, so non-passthrough. This is why I’ve modified DbConnection.Close() to return Task as @terrajobst suggested.
  2. DbDataReader can’t be reopened - it’s returned by DbCommand.ExecuteReader() and consumed by the user, until it’s disposed. Because of this, Close() seems identical to Dispose(), and the only reason to actually add CloseAsync() is for API consistency (because sync Close() already exists). We also never know what peculiar distinction between the two is already implemented by some provider out there, so it seems like a good idea to allow the same in async.
+2
roji on May 14, 2019

FYI for all those following, we’re proposing to remove the cancellation token from {DbDataReader,DbConnection}.CloseAsync: https://github.com/dotnet/corefx/pull/39070, see discussion in https://github.com/dotnet/standard/pull/1283#pullrequestreview-255383035

+1
roji on Jul 1, 2019

Opened dotnet/SqlClient#27 for the Stream/TextReader question, let’s leave it out of this issue for now.

+1
roji on Apr 19, 2019

DbDataReader.Close() seems identical to Dispose(), and the only reason to actually add CloseAsync() is for API consistency (because sync Close() already exists). We also never know what peculiar distinction between the two is already implemented by some provider out there, so it seems like a good idea to allow the same in async.

I remember one interesting distinction we discussed with @bricelam while he was working on https://github.com/aspnet/EntityFrameworkCore/pull/15001, is that Dispose() is usually implemented to not throw.

I still think that getting Stream and TextReader via GetFieldValue (and GetFieldValueAsync) makes a lot of sense - there’s no real reason for these to be different from other types like string or int. The surface API of DbDataReader is already pretty huge, I’d rather avoid adding more methods than we absolutely have to.

Agreed. Should have a new issue on SqlClient to support more stream types with GetFieldValue/GetFieldValueAsync? Should this be covered by ADO.NET specification tests?

+1
divega on Apr 19, 2019

@Wraith2 thanks for testing this.

I still think that getting Stream and TextReader via GetFieldValue (and GetFieldValueAsync) makes a lot of sense - there’s no real reason for these to be different from other types like string or int. The surface API of DbDataReader is already pretty huge, I’d rather avoid adding more methods than we absolutely have to.

+1
roji on Apr 18, 2019

Note: added DbDataReader.GetStreamAsync() and DbDataReader.GetTextReaderAsync() to the proposal above - async support seems especially important for these column-streaming methods.

/cc @divega

+1
roji on Apr 18, 2019

It’s a question of whether we expect these operations (command cancellation, physical connections) to occur intensively enough, and whether the I/O involved to be light enough, so that ValueTask would have a perf impact.

Yeah, but then if its instant, you can return a Task.CompletedTask which is no allocations an IntPtr sized to pass around.

ValueTask<T> makes a lot of sense for most scenarios; but the non-generic version ValueTask makes most sense when its IValueTaskSource backed at some point as its 2 x IntPtr in size so can’t be passed via register; so can be slower if you aren’t taking advantage of its other properties.

+1
benaadams on Feb 1, 2019
Read more comments on GitHub
← runtime: Add Enumerable.IsEmpty that judges `IEnumerable<T>` is empty.
runtime: Add missing members for System.Reflection namespace →