Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement new async methods coming in .NET Core 3.0 #642

Closed
roji opened this issue May 23, 2019 · 12 comments
Closed

Implement new async methods coming in .NET Core 3.0 #642

roji opened this issue May 23, 2019 · 12 comments
Assignees

Comments

@roji
Copy link

roji commented May 23, 2019

.NET Core 3.0 (and hopefully .NET Standard 2.1) will fill in some missing async APIs (e.g. transaction management), these should be implemented in MySqlConnector at some point.

dotnet/corefx#35012

@bgrainger
Copy link
Member

bgrainger commented May 23, 2019

  • Add override ValueTask<DbTransaction> BeginDbTransactionAsync
  • Add MySqlConnection.CloseAsync (Implement MySqlConnection.CloseAsync #467), MySqlConnection.DisposeAsync
  • Add MySqlTransaction.DisposeAsync
  • Add MySqlCommand.DisposeAsync
  • Add MySqlDataReader.CloseAsync, MySqlDataReader.DisposeAsync

Open questions:

  • Should new MySqlConnection.BeginTransactionAsync change to return ValueTask<MySqlTransaction>, or keep the return type as Task<MySqlTransaction>? It will always perform I/O.

@roji
Copy link
Author

roji commented May 23, 2019

Should new MySqlConnection.BeginTransactionAsync change to return ValueTask, or keep the return type as Task? It will always perform I/O.

Since this is a provider-specific method, it ultimately depends on you.

First, are you sure the roundtrip-reducing optimization I described here is absolutely impossible with MySQL, and will never be possible? Because if the possibility exists it's definitely better to choose ValueTask now to avoid a breaking change later.

Even without this optimization, I'd lean towards leaving it to return ValueTask for consistency with the ADO.NET API. I suspect the extra overhead from ValueTask is going to be quite negligible for this specific method, which does a roundtrip - it may be best to simply benchmark this.

@bgrainger
Copy link
Member

bgrainger commented May 23, 2019

The (unstated) compatibility concern is with Connector/NET, which has exposed a (sync) Task<MySqlTransaction> MySqlConnection.BeginTransactionAsync() method for years, and which MySqlConnector currently clones.

The optimisation you mention may be possible by implicitly batching the START TRANSACTION; statement with the next statement the client executes. There would be some amount of complexity with doing that so I'm not planning to implement it soon.

@roji
Copy link
Author

roji commented May 23, 2019

The (unstated) compatibility concern is with Connector/NET, which has exposed a (sync) Task MySqlConnection.BeginTransactionAsync() method for years, and which MySqlConnector currently clones.

Oh, I see, that makes some sense. If you're doing a major release and are willing to break this, it may be worth it to change to ValueTask - I'm not sure how important it is for you to be totally compatible with the Oracle provider. In any case the ValueTask/Task difference isn't huge for more scenarios - unless users are doing something more than awaiting (which may be rare with BeginTransactionAsync() specifically) there's no real difference - just a binary break that requires a recompile.

But if you really don't think you'll be doing the optimization it's just as well to leave it as is, I guess.

@bgrainger
Copy link
Member

bgrainger commented Jun 23, 2019

I'm not sure how important it is for you to be totally compatible with the Oracle provider.

Porting any code from Connector/NET to MySqlConnector requires (at minimum) a recompile, so a binary-breaking but source-compatible change is fine. After more thought, it seems best to align with ADO.NET here (even if it's not technically an override on most frameworks).

@bgrainger bgrainger self-assigned this Jun 28, 2019
@bgrainger
Copy link
Member

bgrainger commented Jun 29, 2019

After all that, I didn't actually change the return type of BeginTransactionAsync (or add the necessary override).

@bgrainger bgrainger reopened this Jun 29, 2019
@bgrainger
Copy link
Member

bgrainger commented Jun 29, 2019

The (unstated) compatibility concern is with Connector/NET, which has exposed a (sync) Task<MySqlTransaction> MySqlConnection.BeginTransactionAsync() method for years, and which MySqlConnector currently clones.

However, Connector/NET does not provide MySqlTransaction.CommitAsync or MySqlTransaction.RollbackAsync (not even with synchronous implementations), so continuing to be API-compatible with Connector/NET seems much less important.

@jadanah
Copy link

jadanah commented Jul 3, 2019

Just as a heads up the changes breaks compatibility with Pomelo.EntityFrameworkCore.MySql (v2.2.0) and will throw a runtime exception when SaveAsync is called on the ef core context.

MySqlConnector 0.57.0-beta3

MissingMethodException: Method not found: 'System.Threading.Tasks.Task`1<MySql.Data.MySqlClient.MySqlTransaction> MySql.Data.MySqlClient.MySqlConnection.BeginTransactionAsync(System.Data.IsolationLevel, System.Threading.CancellationToken)'.

@bgrainger
Copy link
Member

bgrainger commented Jul 3, 2019

I knew this would probably happen, since it is a binary breaking change. However, I hadn't coordinated with Pomelo yet. Just opened PomeloFoundation/Pomelo.EntityFrameworkCore.MySql#794 to track it.

@bgrainger
Copy link
Member

bgrainger commented Aug 8, 2019

@JoJo2406 I've pushed a preview release of Pomelo.EFCore that's compatible with the 0.57.0-beta series.

@jadanah
Copy link

jadanah commented Aug 9, 2019

Thanks Bradley,

I will test and let you know if I come across any issues.

@bgrainger
Copy link
Member

bgrainger commented Sep 24, 2019

Added in 0.57.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

3 participants