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

An ExecuteReaderAsync that returns MySqlDataReader #822

Closed
taspeotis opened this issue May 27, 2020 · 6 comments
Closed

An ExecuteReaderAsync that returns MySqlDataReader #822

taspeotis opened this issue May 27, 2020 · 6 comments
Assignees

Comments

@taspeotis
Copy link

Hi,

MySqlCommand has its ExecuteReaderAsync methods inherited from DbCommand. These return DbDataReader which is fine for the most part, except sometimes you want the underlying MySqlDataReader.

For example MySqlDataReader has a GetTimeSpan method that DbDataReader does not provide.

Could there please be MySqlDataReader-returning equivalents of ExecuteReaderAsync? For now I am explicitly casting the return value.

Thank you.

@bgrainger
Copy link
Member

I haven't added this in the past because it would add extra overhead to convert Task<DbDataReader> (that has to be returned from the overridden virtual method) to Task<MySqlDataReader> , because Task<T> is not covariant.

That is, if this method were added:

public new async Task<MySqlDataReader> ExecuteReaderAsync(CancellationToken cancellationToken = default) =>
	(MySqlDataReader) await base.ExecuteReaderAsync(cancellationToken).ConfigureAwait(false);

Then all code calling ExecuteReaderAsync on a MySqlCommand object would have an extra async state machine created (for this async method) just to implement a single cast. The overhead would dwarf the work that is performed.

Or, alternatively, if the default code path did return Task<MySqlDataReader>, then the virtual method would have to contain an extra "thunk" that upcast the result to DbDataReader, and consumers that use the DbCommand ADO.NET classes directly would suffer a performance penalty.

I think you could write an extension method to do this work (or continue casting the return value as you currently are) but I think the performance penalty that it would incur currently makes it infeasible to include on this API by default. (Hmm, maybe MySqlConnector could provide it as an extension method in a separate namespace so you have to explicit write using MySqlConnector.SlowConvenienceMethods to get it?)

I don't think C# 9.0 covariant return types (https://github.com/dotnet/csharplang/blob/master/proposals/covariant-returns.md) will help this situation either.

@bgrainger
Copy link
Member

It looks like Npgsql chooses to implement new Task<NpgsqlReader> ExecuteReaderAsync efficiently, and "penalise" the virtual DbCommand implementation with an extra await.

Microsoft.Data.SqlClient is similar (new Task<SqlDataReader> ExecuteReaderAsync is efficient) and the DbCommand method is implemented with ContinueWith to perform the cast.

It looks like generally an easier-to-use API is preferred over a slight performance boost. (I should benchmark the alternatives and see how they perform.)

@taspeotis
Copy link
Author

Thanks for the commentary. And regarding Microsoft.Data.SqlClient I was just looking at the exact same place as you :)

For reference: https://github.com/dotnet/SqlClient/blob/df35d18f869ac2daa410ce12ee5264b7759944cd/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlCommand.cs#L2304-L2314

@bgrainger
Copy link
Member

Benchmarks show that await should be preferred over ContinueWith on both platforms:

Method Job Runtime Mean Error StdDev Median Gen 0 Gen 1 Gen 2 Allocated
Direct .NET 4.8 .NET 4.8 71.48 ns 0.694 ns 0.649 ns 71.42 ns 0.0221 - - 104 B
Await .NET 4.8 .NET 4.8 134.81 ns 1.520 ns 1.422 ns 135.18 ns 0.0391 - - 185 B
Continue .NET 4.8 .NET 4.8 560.63 ns 84.097 ns 243.982 ns 443.87 ns 0.0491 - - 233 B
Direct .NET Core 3.0 .NET Core 3.0 67.35 ns 8.013 ns 22.992 ns 62.28 ns 0.0204 - - 96 B
Await .NET Core 3.0 .NET Core 3.0 80.02 ns 5.052 ns 14.738 ns 74.57 ns 0.0356 - - 168 B
Continue .NET Core 3.0 .NET Core 3.0 212.00 ns 9.979 ns 29.110 ns 202.49 ns 0.0458 - - 216 B

@taspeotis
Copy link
Author

Thank you!

@bgrainger
Copy link
Member

This is implemented in 0.67.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

2 participants