Consider adding FillAsync and async APIs on data adapters

[spudcud]

Core 2.0 preview 1 now supports SqlDataAdapter and DataTable which is great and the best way to fill a DataSet from a stored procedure is by using SqlDataAdapter.Fill(DataSet). This works perfectly when not used in an async environment. There is currently no way in Core or full .NET to do a FillAsync. Without a FillAsync which would take a single line of code now requires 20 lines of much more complicated code including using SqlDataReader.GetSchemaTable() which is currently broken preview 1. Based on other posts I believe it will be working in Preview 2 but have not seen a definitive on that.

This is not something for 2.0 but is a clear deficiency in truly being able to support a fully async back-end and would be a much needed improvement moving forward.

[saurabh500]

Moving to future because of lack of resources

[saurabh500]

Alternatively if someone from the community would like to drive this, the effort would be welcome.

[danmoseley]

This needs a formal API proposal written up. @spudcud do you want to do this - updating your top post is clearest? See https://github.com/dotnet/corefx/blob/master/Documentation/project-docs/api-review-process.md

[jonreis]

Not sure if this helps, but someone has implemented this.

https://github.com/voloda/AsyncDataAdapter/blob/master/AsyncDataAdapter/DataAdapter.cs

[Misiu]

@danmosemsft any chance this can be added to Core 3.0? I've added package suggested by @jonreis and it looks like it works fine 😄 Ideally this should be added into Core.

[effyteva]

+1 on this, Perhaps it about time to take care of it?

[Misiu]

@danmosemsft @saurabh500 any updates on this?

[brendanalexdr]

+1

[danmoseley]

@saurabh500?

[danmoseley]

@divega

[divega]

Moving to out of System.Data.SqlClient and into System.Data because I believe the methods should be added to the base DbDataAdapter, before any provider implements it.

I am also keeping this issue in the Future milestone because we need a much better understanding of its impact before we decide to pick it up and implement it in a specific release. For example:

1. How many modern applications that take advantage of async would start using async APIs on DataSet/DataAdapter if they were available? Why don't they use other more modern data access technologies that already support async? I appreciate that a few people have voted for this issue and have been asking for updates on it. This adds up to a bit of data, but we need more, especially given that there are many other things in the backlog that seem to have much more demand.

2. Is the main goal of using async in this case mainly achieving better scalability on the server or more responsiveness on the client?

3. What other API that perform I/O does Fill use, that would need to become async in order for FillAsync to avoid every blocking?

4. What about the async Update story for DataSet/DataTable?

On the flip side, If someone from the community wanted to drive this, I believe the API design part, and possibly the implementation of FillAsync on SqlClient, would be relatively simple. The job is mostly to duplicate the code of the sync version into async methods, and also make sure that all potentially blocking calls in the implementation use async APIs as well.

For the SqlCient implementation of Update, currently batching is internal and synchronous. I suspect it would make sense to leverage the new public batching APIs from https://github.com/dotnet/corefx/issues/35135 once they are in place.

[divega]

cc @roji @ajcvickers

[roji]

Similar to dotnet/corefx#35012, which adds missing async APIs elsewhere in ADO.NET.

Adding these APIs in the System.Data base classes is probably easy enough (with them delegating by default to the sync APIs, as usual with ADO.NET), allowing providers to implement later.

[effyteva]

Similar to dotnet/corefx#35012, which adds missing async APIs elsewhere in ADO.NET.

Adding these APIs in the System.Data base classes is probably easy enough (with them delegating by default to the sync APIs, as usual with ADO.NET), allowing providers to implement later.

Would it help if I publish a PR for that? It seems pretty straightforward.

Effy

[uprightbass360]

Is this still under review?

[roji]

@uprightbass360 this issue is currently in the backlog, which means we don't have immediate plans to implement it priority-wise. In general, investment in DbDataAdapter is mostly limited to maintenance at this point, but given enough user feedback we would definitely consider it. This could be a good issue for the community to look into.

[Misiu]

@maxle5 you added this feature to your fork. Any chance to create a PR?

[roji]

One small note - I haven't reviewed @maxle5's commit, but we'd want to add async across all operations performed by DbDataAdapter, and not just for some (FillAsync). We don't want to end up with a type that only partially supports async.

[Misiu]

@roji I think @maxle5's commit creates all the necessary methods: https://github.com/maxle5/runtime/commit/5d1f61d6c6322d351cbb02069b04eed0c04d208f#diff-fbc8b4fa5b891e40cb60fd8fd075cc81R1236-R1240 https://github.com/maxle5/runtime/commit/5d1f61d6c6322d351cbb02069b04eed0c04d208f#diff-fbc8b4fa5b891e40cb60fd8fd075cc81R1630-R1636 https://github.com/maxle5/runtime/commit/5d1f61d6c6322d351cbb02069b04eed0c04d208f#diff-fbc8b4fa5b891e40cb60fd8fd075cc81R2044-R2056 I haven't tried it, but it looks complete

[roji]

In that case a PR would be good to have!

[maxle5]

@Misiu, @roji , I can definitely create a PR, I had just gotten side-tracked on a different project and never got around to it. I believe there is probably still some work required though; if someone could help point out what is missing, that would be great!

[stephentoub]

In that case a PR would be good to have!

@roji, we don't accept PRs for new APIs until those APIs have been reviewed and approved. Have these? I don't see a formal API proposal here. Thanks.

[roji]

@stephentoub this is only about adding async counterparts to already-existing sync APIs on DbDataAdapter. But you're right of course... @maxle5, what we'd need is an list of public APIs being added.

[maxle5]

Full list of public APIs that were added (async counterparts to already-existing sync APIs):

namespace System.Data.Common
{
    public partial class DataAdapter
    {
        public virtual Task<int> FillAsync(DataSet dataSet);
        protected virtual Task<int> FillAsync(DataSet dataSet, string srcTable, IDataReader dataReader, int startRecord, int maxRecords);
        protected virtual Task<int> FillAsync(DataTable dataTable, IDataReader dataReader);
        protected virtual Task<int> FillAsync(DataTable[] dataTables, IDataReader dataReader, int startRecord, int maxRecords);
        public virtual Task<DataTable[]> FillSchemaAsync(DataSet dataSet, SchemaType schemaType);
        protected virtual Task<DataTable[]> FillSchemaAsync(DataSet dataSet, SchemaType schemaType, string srcTable, IDataReader dataReader);
        protected virtual Task<DataTable> FillSchemaAsync(DataTable dataTable, SchemaType schemaType, IDataReader dataReader);
    }
    public partial class DbDataAdapter
    {
        public override Task<int> FillAsync(DataSet dataSet);
        public Task<int> FillAsync(DataSet dataSet, int startRecord, int maxRecords, string srcTable);
        protected virtual Task<int> FillAsync(DataSet dataSet, int startRecord, int maxRecords, string srcTable, IDbCommand command, CommandBehavior behavior);
        public Task<int> FillAsync(DataSet dataSet, string srcTable);
        public Task<int> FillAsync(DataTable dataTable);
        protected virtual Task<int> FillAsync(DataTable dataTable, IDbCommand command, CommandBehavior behavior);
        protected virtual Task<int> FillAsync(DataTable[] dataTables, int startRecord, int maxRecords, IDbCommand command, CommandBehavior behavior);
        public Task<int> FillAsync(int startRecord, int maxRecords, params DataTable[] dataTables);
        public override Task<DataTable[]> FillSchemaAsync(DataSet dataSet, SchemaType schemaType);
        protected virtual Task<DataTable[]> FillSchemaAsync(DataSet dataSet, SchemaType schemaType, IDbCommand command, string srcTable, CommandBehavior behavior);
        public Task<DataTable[]> FillSchemaAsync(DataSet dataSet, SchemaType schemaType, string srcTable);
        public Task<DataTable> FillSchemaAsync(DataTable dataTable, SchemaType schemaType);
        protected virtual Task<DataTable> FillSchemaAsync(DataTable dataTable, SchemaType schemaType, IDbCommand command, CommandBehavior behavior);        
    }
    public partial interface IDbDataAdapter
    {
        Task PrepareAsync(CancellationToken cancellationToken);
        Task<int> ExecuteNonQueryAsync();
        Task<IDataReader> ExecuteReaderAsync();
        Task<IDataReader> ExecuteReaderAsync(CommandBehavior behavior);
        Task<object> ExecuteScalarAsync();
    }
    public partial interface IDbDataAdapter
    {
        Task CloseAsync();
        Task OpenAsync(CancellationToken cancellationToken);
    }
}

[terrajobst]

@roji we'll need you in the review, I'll send an invite

[GrabYourPitchforks]

I've temporarily removed the ready for review label so that this doesn't show up in the backlog until we're ready to tackle it.

[terrajobst]

Thanks!

[elachlan]

This would be a really fantastic addition, since .Net already has async functions against DbCommand. It would round out this effort. Additionally, as .NET 5 merges both framework and core; There will be a lot of developers migrating applications. This feature gives a performance boost with little effort. I wrote a test based on an application I maintain, there was a 2.5x performance improvement.

So I am here advocating for this to be added to the API, so that the code written by the community can be merged and improved upon.

Thank you everyone for all your efforts.

[elachlan]

What is the hold up on the API review for this? What additional information is required?

A summarisation would be greatly appreciated.

Thanks!

[ajcvickers]

@elachlan We're having some wider discussions around this area. It's looking like these won't be resolved in time to get this into .NET 5, but we're still considering doing it for .NET 6.

[elachlan]

@ajcvickers Thanks for the response. Is there an issue I can follow for those discussions or are they internal?

[ajcvickers]

@elachlan Internal at the moment.

[elachlan]

@ajcvickers Thank you for the explanation it is greatly appreciated. It helps developers and associated companies/organisations plan accordingly.

It's a shame we have to wait for November 2021 for something that seems essentially solved and has such a massive benefit. But I imagine it is not as simple as adding in the new methods.

If these changes are targeted at .NET 6, should we move this issue to that milestone?

Again, everyone's efforts are much appreciated.

[maxle5]

@ajcvickers I would love to have the opportunity to continue contributing to this issue (whether that is the current API proposal or a future one). Please let me know if there is anything I can do once the internal discussions have been had. Thanks.

[Misiu]

@ajcvickers won't there be anything between .NET 5 and .NET 6? Shame we must wait at least till November 2021 for this. There are massive benefits from this.

[roji]

For everyone looking at this, we'll review this issue again when we do the planning for .NET 6 (everyone is currently focused on stabilizing the .NET 5 release). If it does go into .NET 6, this will show up in one of the previews for that release.

[Misiu]

@roji thanks for the update. I hope this does go into .NET 6.

[Misiu]

@roji I think it's time to get back to this. NET 5 is released. Can this be added to 6.0?

[ChristineBoersen]

I'd love to see it in the main release,

For now I'm using Vladimir Kloz's version (Thanks Vlad!!!) https://github.com/voloda/AsyncDataAdapter/blob/master/AsyncDataAdapter/DataAdapter.cs for my data adapter until this is available, since I only need it for fill/read to then use bulk copy async to get it back to sql.

I'm a week from going to testing for migration of our main application from framework to .NET 5.0 and can't wait a year :(

[cincuranet]

Out of curiosity, wouldn't it make sense to also add IAsyncDisposable for data adapter?

[roji]

@cincuranet it doesn't implement IDisposable... what do you have in mind, which resources need to be disposed?

[cincuranet]

it doesn't implement IDisposable

DbDataAdapter > DataAdapter > Component > IDisposable. I'm asking because i.e. DbConnection shares similar path and it has explicit IAsyncDisposable.

what do you have in mind, which resources need to be disposed?

I was more about feature parity with i.e. DbConnection or DbCommand. But also, maybe this select command should be disposed (in general, not sure whether NpgsqlCommand needs that). I have the same in FbDataAdapter (and disposing it). And given the DbCommand is IAsyncDisposable it makes sense to have it in DbDataAdapter as well.

[roji]

@cincuranet thanks, missed the Component path...

However, the idea behind DbConnection implementing IAsyncDisposable, is that closing a (physical) connection can actually involve I/O, so there should be the possibility to do that operation asynchronously. DbDataAdapter, AFAICT, is just an adapter for filling DataSets, which doesn't itself correspond to any resource that would require I/O when closing.

I'd even go further to say that if DbDataAdapter hadn't been made to inherit from Component (a design choice I don't think we'd have done today), it wouldn't have been made to implement IDisposable either...

[cincuranet]

I'd even go further to say that if DbDataAdapter hadn't been made to inherit from Component (a design choice I don't think we'd have done today), it wouldn't have been made to implement IDisposable either...

Yeah, but that ship has sailed (similarly to Close and Dispose on DbConnection).

So, if I understand it correctly, we're now not about having "feature" parity with "sync" API, rather introducing "async" API where it really makes sense, right?

[roji]

Well, yeah - the idea isn't to systematically introduce IAsyncDisposable everywhere where IDisposable is implemented, but to do it where it makes sense. I don't think it's a matter of "feature parity"; in many cases it really does make sense to have IDisposable but not IAsyncDisposable. IAsyncDisposable really is intended for cases where disposing takes a long time (i.e. involves I/O/networking), whereas IDisposable covers various other cases.

For example, say some type has some native resource, or allocates unmanaged memory. These resources need to be freed when disposing (so IDisposable), but there's no reason for the type to implement IAsyncDisposable, because there's no async involved here whatsoever.

[ajcvickers]

We had the discussion some time ago about which Db... objects are really designed to be disposable, and which are disposable just because the inherit from Component. DbDataReader landed on the disposable side; DbDataAdapter landed on the other side.

[daiplusplus]

@ChristineBoersen I've built on Vlad's work and published my own fork which is fully async (Vlad's original code missed a few spots, and didn't use ConfigureAwait(false) throughout), and it targets .NET Standard 2.0 and it works under .NET Core 3.1 and 5.0 in my testing (and it supports UpdateAsync etc)

https://www.nuget.org/packages/Jehoel.AsyncDataAdapter/

https://github.com/Jehoel/AsyncDataAdapter/

Discussion (especially w.r.t. testing): https://github.com/dotnet/runtime/discussions/50402

[elachlan]

So last time there was discussion on this issue there was mentions of a possible inclusion in .NET 6.

Has it been added to the planning?

Does the work done by Jehoel make that easier to merge? (Thanks Jehoel!)

I'd there still a need for an API change proposal?

[ChristineBoersen]

I'll definitely have to take a look. :)

Thanks, Christine

---

From: Dai @.> Sent: Tuesday, April 6, 2021 9:33:17 AM To: dotnet/runtime @.> Cc: Christine Boersen @.>; Mention @.> Subject: Re: [dotnet/runtime] Consider adding FillAsync and async APIs on data adapters (#22109)

@ChristineBoersenhttps://github.com/ChristineBoersen I've built on Vlad's work and published my own fork which is fully async (Vlad's original code missed a few spots, and didn't use ConfigureAwait(false) throughout), and it targets .NET Standard 2.0 and it works under .NET Core 3.1 and 5.0 in my testing (and it supports UpdateAsync etc)

https://www.nuget.org/packages/Jehoel.AsyncDataAdapter/

https://github.com/Jehoel/AsyncDataAdapter/

Discussion (especially w.r.t. testing): #50402https://github.com/dotnet/runtime/discussions/50402

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/dotnet/runtime/issues/22109#issuecomment-814123099, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ADIK7XNY3XOYI7CWFYHU453THMER3ANCNFSM4NESDNLA.

[IVTore]

DbDataAdapter has a very versatile, solution oriented beautiful architecture. I am really waiting for FillAsync with fingers crossed. Thanks to all people involved. Jumping and yelling in my mind ("Why wait until .net 6.0?") XD