Add async methods in System.Data.Common, implement IAsyncDisposable

[roji]

DO NOT MERGE until dotnet/corefx#39070 is merged

---

This brings over https://github.com/dotnet/corefx/issues/35012 (already merged for .NET Core 3.0) to .NET Standard 2.1.

One thing I noticed, is that in the corefx implementation the types have been updated to implement IAsyncDisposable, but in the ref they have not (although DisposeAsync() has been added). Can someone please confirm whether the interface should get added there?

[roji]

/cc @divega @ajcvickers @terrajobst

[divega]

LGTM. I assume we need @terrajobst to approve/merge.

[terrajobst]

LGTM. I assume we need @terrajobst to approve/merge.

We need all board representatives to sign off.

Tagging @marek-safar @agocke

[agocke]

One note: it seems a little weird to accept a cancellation token here. The reasoning for not including a CancellationToken in DisposeAsync is as follows:

DisposeAsync accepting a CancellationToken: while in theory it makes sense that anything async can be canceled, disposal is about cleanup, closing things out, free'ing resources, etc., which is generally not something that should be canceled; cleanup is still important for work that's canceled. The same CancellationToken that caused the actual work to be canceled would typically be the same token passed to DisposeAsync, making DisposeAsync worthless because cancellation of the work would cause DisposeAsync to be a nop. If someone wants to avoid being blocked waiting for disposal, they can avoid waiting on the resulting ValueTask, or wait on it only for some period of time.

[roji]

Thanks for the comment! It seems like there may be cause to treat CloseAsync a bit differently from DisposeAsync.

First, it's less likely that the same CancellationToken that caused the work to be canceled would end up being passed to CloseAsync (assuming the standard async using pattern etc.).

Also, at least for DbConnection, the same object can closed and opened several times, so closing isn't really about terminal cleanup (object continues to be usable afterwards), but possibly a bit more similar to a standard I/O operation. That's admittedly less true for DbDataReader which can't be reopened once closed - although some properties on it remain alive (and in some cases even only accessible) after close and before dispose.

@divega @terrajobst what do you think? Will hold off on merging this for a bit.

[divega]

@roji, I came to the exact same conclusions as you without reading your comment 😄 In particular about the difference between CloseAsync and DisposeAsync.

But then I looked at some of the code we have already put together in EF Core in preparation for this PR to see if I could find any interesting anecdotal information. And it turns out that we have a few different cases that made me change my mind completely.

TL;DR: I think I found proof that @agocke is right, and having CancellationToken on CloseAsync is actually a pit of failure.

Here are the examples:

• In MigrationCommandExecutor.ExecuteNonQueryAsync, we are using DbConnection.CloseAsync(CancellationToken) as part of finally block, and we are effectively passing the same CancellationToken we passed before to DbCommand.ExecuteNonQueryAsync.

• In RelationalTransaction.ClearTransactionAsync we call CloseAsync(CancellationToken) on the connection, also passing the same cancellation token we pass to the previous call that does some work.

• In BatchExecutor.ExecuteAsync same problem.

• In all the execute methods in RelationalCommand same problem.

• In SqlServerDatabaseCreator.ExistsAsync we call IRelationalConnection.CloseAsync without passing the cancellation token! I am not sure if we should call it an oversight 😄

All in all, it seems that we are using CloseAsync for cleanup (e.g. in finally blocks) and it is hard to figure why passing the same cancellation token that you are passing to everything else is the wrong thing to do.

@roji, @ajcvickers I will create an issue in EF Core to track removing the cancellation token. If you agree, can we get this changes into .NET Core and this PR?

[divega]

Related: dotnet/efcore#16353.

[roji]

On DbConnection, it seems a little problematic to have OpenAsync accept a token but not CloseAsync - but I do agree that the pit of failure here outweighs that. Am fine with removing the token from CloseAsync, will wait to hear @ajcvickers's opinion.

[ajcvickers]

I'm on the fence, but I'm fine with removing it.

[divega]

On DbConnection, it seems a little problematic to have OpenAsync accept a token but not CloseAsync

Do you have in mind any specific case in which this is problematic?

I haven't come up with any case in which because of hoe OpenAsync() is called, we would need CloseAsync() to accept a CancellationToken to achieve the correct behavior.

Example 1:

try
{
    await connection.OpenAsync(ct);
    command.Connection = connection;
    await command.ExecuteNonQueryAsync(ct);
}
finally
{
    await connection.CloseAsync();
}

• If cancellation is triggered after OpenAsync() executes but before ExecuteNoQueryAsync() executes. The latter should then throw, causing the finally block to be executed. Since the connection is open, CloseAsync should close the connection.

• If cancellation is triggered before OpenAsync() executes, the latter will not open the connection and throw, causing the finally block to be executed. Since the connection isn't open, CloseAsync should then be a no-op.

Example 2:

await connection.OpenAsync(ct);
try
{
    command.Connection = connection;
    await command.ExecuteNonQueryAsync(ct);
}
finally
{
    await connection.CloseAsync();
}

If cancellation is triggered before OpenAsync(), the connection should not be opened and the exception should be handled elsewhere. This is the pattern in https://github.com/aspnet/EntityFrameworkCore/blob/master/src/EFCore.Relational/Migrations/Internal/MigrationCommandExecutor.cs#L142.

If OpenAsync() implementation is buggy and does not throw if its cancelled, then it becomes equivalent to one of the other cases.

[roji]

The scenarios I have in mind are where DbConnection.CloseAsync isn't used for cleanup (in a finally block), but as a close operation where the connection is expected to be reopened. It's also worth noting that this is valid for non-pooled connections - with pooled connections (the common case) CloseAsync is expected to return synchronously in all but some rare uncommon cases (since it's putting the connection back in the pool).

But I do agree that scenarios where close cancellation is important are niche, and see more potential damage in keeping the token (i.e. when the same cancelled token is accidentally in passed for cleanup). Since @ajcvickers is OK with removing, I can submit a PR to corefx and see whether @stephentoub or @terrajobst have any objections.

[roji]

Submitted PR to corefx for token removal: dotnet/corefx#39070

[terrajobst]

@roji @divega

OK, I'll wait until dotnet/corefx#39070 is being merged.

[wtgodbe]

Approved for preview7

[roji]

Seems like this was merged before the cancellation token was removed from CloseAsync - note that dotnet/corefx#39070 has already been merged, removing it from corefx.

Have just submitted #1304 to remove the token from the two CloseAsync methods.