[API Proposal]: `DbDataReaderEnumerator` with `MoveNextAsync`

[wessleym]

Background and motivation

I would like to use DbEnumerator.MoveNextAsync, which does not exist. (MoveNext does exist, but again, I want to read asynchronously.) DbEnumerator accepts IDataReader as a parameter, and since IDataReader does not support ReadAsync, DbEnumerator cannot be readily given async abilities without breaking changes.
I have made a DbDataReaderEnumerator class which requires DbDataReader as a parameter. DbDataReader has a ReadAsync method, so adding DbDataReaderEnumerator.MoveNextAsync is trivial. This class just reuses MoveNext code to make a MoveNextAsync with async enhancements.
This is my suggested file:
https://github.com/wessleym/runtime/blob/d49c266b80dcc36566e55b72d93da2fe1fc98509/src/libraries/System.Data.Common/src/System/Data/Common/DbDataReaderEnumerator.cs

API Proposal

namespace System.Data.Common;

public class DbDataReaderEnumerator : IEnumerator<DbDataRecord>, IAsyncEnumerator<DbDataRecord>
{
    public DbDataReaderEnumerator(DbDataReader reader);
    public DbDataRecord Current { get; }
    public bool MoveNext();
    public async ValueTask<bool> MoveNextAsync();
}

API Usage

var readerEnumerator = new DbDataReaderEnumerator(reader);
var anyResults = await readerEnumerator.MoveNextAsync().ConfigureAwait(false);

Alternative Designs

An alternative would be to allow DbEnumerator to accept DbDataReader as a parameter, but that would introduce breaking changes to callers who were passing an IDataReader as a parameter.

Risks

I am aware of no risks since DbDataReaderEnumerator is a new class.

Tagging subscribers to this area: @roji, @ajcvickers
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Background and motivation

I would like to use DbEnumerator.MoveNextAsync, which does not exist. (MoveNext does exist, but again, I want to read asynchronously.) DbEnumerator accepts IDataReader as a parameter, and since IDataReader does not support ReadAsync, DbEnumerator cannot be readily given async abilities without breaking changes.
I have made a DbDataReaderEnumerator class which requires DbDataReader as a parameter. DbDataReader has a MoveNextAsync method, so adding DbDataReaderEnumerator.MoveNextAsync is trivial. This class just reuses MoveNext code to make a MoveNextAsync with async enhancements.
I plan to create a pull request in the next few minutes since this was a quick change to make.

API Proposal

namespace System.Data.Common;

public class DbDataReaderEnumerator : IEnumerator<DbDataRecord>, IAsyncEnumerator<DbDataRecord>
{
    public DbDataReaderEnumerator(DbDataReader reader);
    public DbDataRecord Current { get; }
    public bool MoveNext();
    public async ValueTask<bool> MoveNextAsync();
}

API Usage

var readerEnumerator = new DbDataReaderEnumerator(reader);
var anyResults = await readerEnumerator.MoveNextAsync().ConfigureAwait(false);

Alternative Designs

An alternative would be to allow DbEnumerator to accept DbDataReader as a parameter, but that would introduce breaking changes to callers who were passing an IDataReader as a parameter.

Risks

I am aware of no risks since DbDataReaderEnumerator is a new class. Please note that class does not seem to be me to be a new API. It's just a new class within a larger API. But it's not clear to me what qualifies a change as being a new API.

Author:	wessleym
Assignees:	-
Labels:	api-suggestion, area-System.Data
Milestone:	-

[roji]

Some initial thoughts:

• First, adding an enumerator type - as opposed to an IAsyncEnumerable - is less useful; for example, one won't be able to do await foreach on it, or use System.Linq.Async to compose LINQ operators on top of it. Is there any particular reason you're proposing adding an enumerator rather than an enumerable?
• While this would allow async moving from record to record (which is a good thing), DbDataRecord itself still has no async methods on it (unlike DbDataReader which does have GetFieldValueAsync and IsDBNullAsync), so this is proposal is incomplete in that sense.
• DbDataReader already implements IEnumerable (with GetEnumerator as abstract), and implementing types typically returning DbEnumerator (e.g. SqlClient's GetEnumerator). Your proposed DbDataReaderEnumerator seems to be conceptually very similar to this existing DbEnumerator - I'm not sure why it would make sense for a new type to be introduced.

Stepping back, the "enumerability" story of DbDataReader generally isn't great; as I wrote above, DbDataReader implement IEnumerable, but the non-generic IEnumerable only. And once again, the enumerated type - DbDataRecord - hasn't evolved with the rest of ADO.NET and e.g. doesn't support async, as well as probably other APIs. I think that if we want to evolve this enumerability story, we'd need to consider the design more holistically and think of all these problems together, rather than adding a new type such as DbDataReaderEnumerator which addresses only a single concern, etc.

At the same time, this is the first time I'm seeing a complaint about DbDataReader not providing an async enumerator - typical usage is done via DbDataReader.MoveNextAsync() directly. Is there any particular reason you require an actual enumerator here, where MoveNextAsync() isn't sufficient?

[wessleym]

1. I agree about an enumerable vs. an enumerator. But DbEnumerator allows for the row's data to be saved into the record with a DataRecordInternal. When using DbDataReader.ReadAsync(), if the reader gets disposed followed by an attempt to read data from the record, an exception will be thrown. So let's say my code reads the records, disposes of the reader, and then tries to read the record values later on. DbDataReader.ReadAsync() doesn't allow for that, but DbEnumerator does since it caches the data.
2. Since I would prefer that my data get loaded into the record initially like with DbEnumerator, I wouldn't need GetFieldValueAsync or IsDBNullAsync. I would already have a dictionary of all record objects for fast value retrieval and DBNull checks. (Interesting, SqlDataReader.GetFieldValueAsync doesn't seem to be an async method anyway, despite the name.)
3. The reason why I would think a new type would be necessary is because DbEnumerator requires just an IDataReader. DbDataReaderEnumerator requires a DbDataReader. DbDataReader is needed for ReadAsync(). I think you would want to allow DbEnumerator to continue to accept just an IDataReader, but an IDataReader is not sufficient for my purposes in this new class.

Regarding your second-to-last paragraph, that sounds great, but it also sounds like a lot of work, and I'm not sure when that would be done. My suggestion just addresses my specific need.

Regarding your last paragraph, please see my point 1 above in this comment.

Thank you for your time and consideration.

[roji]

DbDataReader.ReadAsync() doesn't allow for that, but DbEnumerator does since it caches the data.

I think there's some confusion around this; DbEnumerator doesn't do any caching whatsoever (source code) - it's merely an implementation of the IEnumerator interface over DbDataReader. That is, it internally saves a DbDataReader, and calls MoveNext() on it as you enumerate it. Neither does your proposed DbDataReaderEnumerator in #98891.

So in both types, if you dispose the DbDataReader before fully enumerating, you would get an ObjectDisposeException just as if you had accessed DbEnumerator directly. The point of the IEnumerable (and IEnumerator) APIs is not to cache the entire data (which would take up lots of memory), but to the contrary, to enumerate (without any caching). In fact, if DbEnumerator/DbDataReaderEnumerator cached the entire resultset as you seem to be expecting, there would no longer be a need for async methods to enumerate it, since all data would be in memory anyway.

If you're simply looking to cache the entire resultset so that you can dispose DbDataReader early, there are various ways to do that: the easiest would probably to do a small loop over the rows with ReadAsync(), and read each one with DbDataReader.GetValues(). Or you can use something like Dapper or EF, which simply materializes the rows into objects for you.

Interesting, SqlDataReader.GetFieldValueAsync doesn't seem to be an async method anyway, despite the name

That might be a problem in the SqlClient implementation of that method (SqlClient has various issues with its async support) - but of course System.Data is implemented by other drivers of other databases (the Npgsql implementation definitely is fully async).

The reason why I would think a new type would be necessary is because DbEnumerator requires just an IDataReader.

It may be possible to retro-fit DbDataReader with DbDataReader support, for the async support. But in any case, we need to consider having DbDataReader implement IAsyncEnumerable, with a default implementation that either returns the new enumerator type (or the retro-fitted DbEnumerator), or throws, or possibly returns a sync enumerator - as most System.Data APIs do in the async default implementations. This was my point about having to consider this holistically rather than just adding a new public type to fix your specific, narrow need.

Regarding your second-to-last paragraph, that sounds great, but it also sounds like a lot of work, and I'm not sure when that would be done. My suggestion just addresses my specific need.

Understood. I've opened #98903, summarizing some of the points made in this issue, and considering the larger DbDataReader enumerability story. I'll go ahead and close this issue for now - but please feel free to continue the conversation here (e.g. with regards to the caching question above). In any case, if we see more user interest we can definitely look at improving things in this area.

[wessleym]

Thank you again for your time and for opening that new issue. I agree with everything you said except for your initial paragraphs about caching. Please see the below example, and please note I understand this isn't the most ideal way to retrieve the information shown. It's just an example of what I mean by caching.

First, I declare two methods for retrieving records. One caches row data via DbEnumerator. The other just calls ReadAsync directly without using DbEnumerator. (Please note that the outcome of this experiment would be the same whether I used DbDataReader.Read() or DbDataReader.ReadAsync().)

public static IEnumerable<IDataRecord> GetRecords(string connectionString, SqlParameter[] parameters, string commandText)
{
    using (SqlConnection connection = new SqlConnection(connectionString))
    {
        connection.Open();
        using (SqlCommand command = new SqlCommand(commandText, connection))
        {
            command.Parameters.AddRange(parameters);
            using (var reader = command.ExecuteReader())
            {
                foreach (IDataRecord record in reader)
                {//Uses DbEnumerator
                    yield return record;
                }
            }
        }
    }
}

public static async IAsyncEnumerable<IDataRecord> GetRecordsAsync(string connectionString, SqlParameter[] parameters, string commandText, [EnumeratorCancellation] CancellationToken cancellationToken)
{
    using (SqlConnection connection = new SqlConnection(connectionString))
    {
        await connection.OpenAsync(cancellationToken).ConfigureAwait(false);
        using (SqlCommand command = new SqlCommand(commandText, connection))
        {
            command.Parameters.AddRange(parameters);
            using (var reader = await command.ExecuteReaderAsync().ConfigureAwait(false))
            {
                while (await reader.ReadAsync().ConfigureAwait(false))
                {//Does not use DbEnumerator
                    yield return reader;
                }
            }
        }
    }
}

Now see the code that calls each method:

string userName1 = (string)GetRecords("Some Connection String", [], "select UserName from [User]").First()["UserName"];
string userName2 = (string)(await GetRecordsAsync("Some Connection String", [], "select UserName from [User]", CancellationToken.None).FirstAsync().ConfigureAwait(false))["UserName"];

userName1 is retrieved successfully, despite the reader being disposed since the row information was cached prior to the reader's disposal. userName2 cannot be retrieved, and an InvalidOperationException is thrown: "Invalid attempt to call MetaData when reader is closed." The reader is disposed, and the information was not cached.

[roji]

Re caching, have you tried this with more than one row (and replacing First() with e.g. ToList())? DbEnumerator is probably caching the row it's already one (the first row), but once you call MoveNext() on it, it should just call MoveNext() on the underlying reader and you'll get the same exception.

[wessleym]

Yes.

var records1 = GetRecords("Some Connection String", [], "select UserName from [User]").ToArray();
var records2 = await GetRecordsAsync("Some Connection String", [], "select UserName from [User]", CancellationToken.None).ToArrayAsync().ConfigureAwait(false);
_ = records1[anyIndex]["UserName"];//works
_ = records2[anyIndex]["UserName"];//fails with same InvalidOperationException: "Invalid attempt to call MetaData when reader is closed."

I contend the synchronous example works because DbEnumerator is saving each record's data into a DataRecordInternal:

runtime/src/libraries/System.Data.Common/src/System/Data/Common/DbEnumerator.cs

Line 68 in 66aebed

_current = new DataRecordInternal(_schemaInfo, values, _descriptors, _fieldNameLookup!);

[roji]

I'm not sure exactly what's going on in your test code, but to be extra precise, DbEnumerator caches the row - by calling DbDataReader.GetValues(), but not the resultset - that's very clear from its source code. That means that as you enumerate DbEnumerator (MoveNext()), it simply calls Read() on its wrapped DbDataReader; so any attempt to call enumerate DbEnumerator after disposing its DbDataReader will cause an ObjectDisposedException.

In other words, there's really nothing going on in DbEnumerator that you can't do yourself by just calling GetValues() and Read() on DbDataReader directly.