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

[roji]

Provider implementation tracking issues

• SqlClient: Implement async transaction begin/commit/rollback methods SqlClient#113
• Npgsql: Override new async methods introduced in corefx npgsql/npgsql#2481
• MySqlConnector: Implement new async methods coming in .NET Core 3.0 mysql-net/MySqlConnector#642

Specification

This issue consolidates #18225, #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 #19858, #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. #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.

[benaadams]

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

Don't think default(ValueTask) has any advatange over Task.CompletedTask in this scenario?

/cc @stephentoub

[bgrainger]

public virtual Task ChangeDatabase(string databaseName, CancellationToken cancellationToken = default);

ChangeDatabaseAsync ?

[bgrainger]

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

For most providers, that support pooling, it seems like DbConnection.Close[Async] would by default just synchronously return the connection to the pool (and not perform any I/O). Perhaps the ability to return Task.CompletedTask from a (non-asynchronous) implementation means this isn't a problem in practice? Cf. mysql-net/MySqlConnector#467.

For MySQL, DbCommand.Cancel[Async] is always (lots of) network I/O, to send a "kill query" command to the server (and wait for acknowledgement). I would put it firmly in the "likely to be dominated by networking etc." category.

[roji]

@benaadams,

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

Don't think default(ValueTask) has any advatange over Task.CompletedTask in this scenario?

I agree.. 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. I agree that it probably doesn't make much sense.

@bgrainger,

ChangeDatabaseAsync?

Yep, corrected...

For most providers, that support pooling, it seems like DbConnection.Close(Async) would by default just synchronously return the connection to the pool (and not perform any I/O). Perhaps the ability to return Task.CompletedTask from a (non-asynchronous) implementation means this isn't a problem in practice? Cf. mysql-net/MySqlConnector#467.

I agree, and like with OpenAsync() it makes sense to me for CloseAsync() to return a Task. It also fits squarely with the idea that calls involving a network roundtrip should return Task (because the Task vs. ValueTask difference is negligible anyway).

But just as a note, the fact that an operation is usually expected to return synchronously doesn't necessarily seem decisive - both Task- and ValueTask- returning methods can have zero overhead in this case. I think it matters more what happens when the operation does return asynchronously, and how often that's expected to occur. Compare with NpgsqlDataReader.Read(), which is also expected synchronously in most cases because rows would typically be buffered in memory - it still seems to make sense to have a version returning ValueTask (not a database roundtrip, but rather continuously parsing results).

For MySQL, DbCommand.Cancel(Async) is always (lots of) network I/O, to send a "kill query" command to the server (and wait for acknowledgement). I would put it firmly in the "likely to be dominated by networking etc." category.

I agree...

[benaadams]

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.

[roji]

Note: rather than providing an empty virtual implementation of DisposeAsync(), we probably need to provide the async analogue of the dispose pattern (Dispose(bool) called by both Dispose() and the finalizer). Need to confirm.

After thinking about this again (and not at 3AM), this doesn't seem to make any sense - finalizers shouldn't be doing I/O or calling async methods, and the classes in question call into the synchronous Dispose(bool).

[MaceWindu]

Task BeginTransactionAsync: shouldn't it have Task<DbTransaction> return type?

[roji]

Task BeginTransactionAsync: shouldn't it have Task<DbTransaction> return type?

Thanks, yes - corrected.

[terrajobst]

Video

@roji @divega

• What does it mean to cancel CancelAsync()? Also, how is calling CancelAsync() different from cancelling the cancellation token that was passed to the operation that CancelAsync() would cancel?
• Is there a reason why CloseAsync() returns ValueTask? It doesn't seem to on the perf critical path?
• Should we really add DbDataReader.CloseAsync() seems odd to add an API that is known to be not useful.

[roji]

What does it mean to cancel CancelAsync()?

Unlike some other async cancellations, cancelling an ongoing database operation typically involves a (potentially) heavy networking operation (e.g. establish a new connection to the database). For this reason it may make sense to treat the cancellation as its own async operation that may need to be cancelled (although I admit it may be a bit contrived).

Assuming we decide to keep CancelAsync() (see below), we could drop the cancellation token (although this would be the first case I've heard of where an async method doesn't accept one).

Also, how is calling CancelAsync() different from cancelling the cancellation token that was passed to the operation that CancelAsync() would cancel?

1. There's at least the possibility of two different cancellation types. simply passing the token down to the socket layer (which now has actual value since https://github.com/dotnet/corefx/issues/24430), vs. triggering the database-specific cancellation mechanism I described above (e.g. establishing a new connection to the database). It's true that there's a lot of ambiguity here though.
2. It's possible for CancelAsync() to be called on a synchronous operation, in which case there's no token (although it may be contrived for an application to sometimes use sync, sometimes async).
3. DbCommand already has a synchronous Cancel(), so this adds its async counterpart for consistency.

But I do admit it could make sense to not have CancelAsync() at all, and say that Cancel() is there for sync only, whereas sync has the original cancellation token - that does seem to be a simpler API surface around cancellation. @divega what do you think?

Is there a reason why CloseAsync() returns ValueTask? It doesn't seem to on the perf critical path?

For DbDataReader.CloseAsync() - assuming we actually add it (see next point) - it returns ValueTask for consistency with DisposeAsync(), to which it's very similar. Note that unlike with DbConnection, there's no way to reopen a DbDataReader, so disposing and closing really mean the same thing (unless some provider decides to implement them differently). Since DisposeAsync() must return ValueTask (because of IDisposableAsync), it seems to make sense to return the same from CloseAsync().

For DbConnection.CloseAsync(), I think you're right - will update the proposal.

Should we really add DbDataReader.CloseAsync() seems odd to add an API that is known to be not useful.

It's definitely not absolutely necessary, but:

1. The sync counterpart Close() already exists, and it seems like it would be inconsistent/surprising to not have it.
2. There's the possibility that some provider out there decided to do something different for Close()/Dispose(), so they would need CloseAsync() to maintain the same distinction with async.
3. There doesn't seem to be a big disadvantage in adding CloseAsync() specifically (apart from the general added API surface). Just like with Close()/Dispose(), providers would typically implement one by calling the other.

[roji]

Just to clarify one of my points from above: in the current state of things, some providers already implement DbCommand.Cancel() via a database-specific mechanism. It should be possible to trigger that same mechanism asynchronously, but if we do that when the cancellation token is triggered we lose the option to pass it down into the socket layer.

[roji]

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

/cc @divega