Implement SqlConnection.GetSchemaAsync and SqlDataReader.GetSchemaTableAsync #3005
Conversation
| cancellationToken.ThrowIfCancellationRequested(); | ||
| #if NET | ||
| DataTable schemaTable = isAsync ? await reader.GetSchemaTableAsync(cancellationToken).ConfigureAwait(false) : reader.GetSchemaTable(); | ||
| #else |
There was a problem hiding this comment.
Would it make sense to have the .NET Framework path also use an async API here? Unless I'm missing something, the exact same methods/code could be define for both, with the sole extension that in .NET Framework it wouldn't override?
There was a problem hiding this comment.
I can do that; I thought it might look a little clumsy for the .NET Framework version of Microsoft.Data.SqlClient to have a dedicated async method which isn't present on System.Data.SqlClient and which doesn't actually run asynchronously.
(GetSchemaTable just constructs a DataTable from the cached metadata - there's nothing which we can make async.)
There was a problem hiding this comment.
Running this through CI now...
The IDbColumnSchemaGenerator method will need to be done after (or possibly as part of) #2967.
There was a problem hiding this comment.
GetSchemaTable just constructs a DataTable from the cached metadata - there's nothing which we can make async
Oh I must be misunderstanding what this is about then - I thought the idea here is to actually get the schema information (asynchronously).
In any case, the idea is to have the least amount of separate code paths here for .NET Framework vs. modern .NET...
There was a problem hiding this comment.
I implemented this, but on re-checking, we access a property within SqlDataReader which performs a read from the network. I'm going to revert the SqlDataReader changes tomorrow so that I don't introduce async-over-sync within the new API. I'll submit the new SqlDataReader API in a subsequent PR, which will need more review since it'll involve touching the state machine.
SqlConnection.GetSchemaAsync is much simpler, since it just layers atop SqlCommand. This PR can still introduce that safely.
| @@ -107,7 +109,7 @@ protected virtual void Dispose(bool disposing) | |||
| } | |||
| } | |||
|
|
|||
| private DataTable ExecuteCommand(DataRow requestedCollectionRow, string[] restrictions, DbConnection connection) | |||
| private async ValueTask<DataTable> ExecuteCommandAsync(DataRow requestedCollectionRow, string[] restrictions, DbConnection connection, bool isAsync, CancellationToken cancellationToken) | |||
There was a problem hiding this comment.
Really nice to be seeing proper async/await in SqlClient!
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3005 +/- ##
==========================================
+ Coverage 72.48% 72.53% +0.04%
==========================================
Files 288 288
Lines 59493 59519 +26
==========================================
+ Hits 43123 43170 +47
+ Misses 16370 16349 -21
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
edwardneal
changed the title
This introduces async-over-sync, removing such requires work with the state machine and separate review.
Contributes to #646, adding
SqlConnection.GetSchemaAsyncandSqlDataReader.GetSchemaTableAsync.SqlConnection.GetSchemaAsyncis the only .NET 5.0+ method which would benefit from end-to-end async plumbing to remove the async-over-sync which is currently present. The other two schema methods on DbDataReader and IDbColumnSchemaGenerator already operate from the cached metadata. I've copied the .NET 5.0 base implementation to SqlDataReader to harmonise this part of the API surface between .NET Framework and .NET Core.I can't add the IDbColumnSchemaGenerator method because the .NET Framework version of SqlDataReader doesn't yet implement that interface. #2967 tracks this.
I've added some basic documentation, but this effectively follows the
SqlCommand.[Something]Asyncpattern of "An asynchronous version of [Something], which <etc.>"I've also moved the netfx GetSchema methods from SqlConnectionHelper.cs to SqlConnection.cs, to align that part of it with netcore. This contributes to the merge slightly.