Csvhelper: [Feature Request] Add WriteRecordsAsync<T> to CsvWriter

[AdamDotNet]

In Asp.Net Core 3.0 (to RTM on Sept 23, 2019), calling synchronous IO APIs on the HttpResponse stream throws an exception to help warn users of potential performance issue. One area I recently hit is calling CsvWriter.WriteRecords<T>; eventually NextRecord is called which may flush synchronously if the buffer is full. When the CsvWriter is wrapping the Asp.Net HttpResponse stream, then the following message occurs: InvalidOperationException: Synchronous operations are disallowed. Call WriteAsync or set AllowSynchronousIO to true instead. I'd prefer to be fully on async IO instead of allowing sync IO. I didn't even realize I was doing sync IO!

Demo code showing how the exception might occur can be found here.

Proposal
I propose that IWriter and CsvWriter add

• WriteRecordsAsync(IEnumerable)
• WriteRecordsAsync<T>(IEnumerable<T>)

Alternative
One could write an extension method that handles this fairly easily. Having it built in costs a breaking change to the IWriter interface, but may be worth it for the pit of success.

Branch with proposed changes
/AdamDotNet/CsvHelper
I did some refactoring to share most of the core logic, not only between the sync and async versions, but also in the IEnumerable and IEnumerable versions. Hopefully this helps to maintain yet another "nearly identical" method. The only difference from the synchronous version of the method is the call to NextRecordAsync instead of NextRecord.

Initially, I only wrote a couple of unit tests, and all other unit tests passed on my machine. Were a pull request to be made, how many synchronous tests would you want duplicated for the asynchronous version? All of the tests calling WriteRecords?

Addtional Details
While this issue references Asp.Net Core 3.0, this feature request does not require any preview bits to compile, and would benefit every TFM.

[simonholt]

The same problem occurs when reading files using CsvReader.GetRecords.

[AdamDotNet]

My branch referenced in the original post does not contain changes for this yet, but CsvReader.GetRecordsAsync could definitely be implemented too, using the new IAsyncEnumerable interface.

await foreach (...) does require the C# 8 compiler. C# 8 is technically only supported by .Net Core 3.0+, but I've personally seen await foreach work in .Net Framework as well. The type IAsyncEnumerable is available as a .Net Standard 2.0 library; it seems this particular feature out of all C# 8 features works largely based on the presence of the right types, rather than simply platform (as opposed to the default interface implementations, you definitely need platform support).

Assuming no C# 8 support, a consumer can still manually create a try/finally block.

IAsyncEnumerable<T> enumerable;
IAsyncEnumerator<T> enumerator = enumerable.GetAsyncEnumerator();
try
{
   while (await enumerable.MoveNextAsync().ConfigureAwait(false))
   {
         // do the things.
   }
}
finally
{
    await enumerable.DisposeAsync().ConfigureAwait(false);
}

[idigra]

+1, writing to files, http responses, etc. asynchronously is an important feature. I vote for publishing Pull Request of the mentioned branch.

[JoshClose]

I'll get to this eventually. For those of you who need it now, you can do this:

foreach (var record in records) 
{
    csv.WriteRecord(record);
    await csv.NextRecordAsync();
}

[JoshClose]

And

while (await csv.ReadAsync())
{
    var record = csv.GetRecord<Foo>();
}

[JoshClose]

Can you take at this and let me know what you think?

https://github.com/JoshClose/CsvHelper/commit/f1dce7ca0bf5d8c6ad5a854dc4d861df1d0bcc57

I'm pushing a breaking change version soon, and I want this to be a part of it.

[AdamDotNet]

Awesome, I'll try to find time today to review!

[AdamDotNet]

I left 2 comments on your commit, only one really matters (missing ConfigureAwait(false)). Otherwise looks good to me.

[JoshClose]

This in in 13.0.0 on NuGet.

[cypressious]

I have an IAsyncEnumerable<Foo> enumerable that I iterate over and write to CSV like so

await using var writer = new StreamWriter(Response.Body);
using var csvWriter = new CsvWriter(writer, new CsvConfiguration(CultureInfo.InvariantCulture) { Encoding = Encoding.UTF8, Delimiter = ";" });

csvWriter.Configuration.RegisterClassMap<FooClassMap>();

csvWriter.WriteHeader<Foo>();
await csvWriter.NextRecordAsync();

await foreach (var item in enumerable)
{
    csvWriter.WriteRecord(item);
    await csvWriter.NextRecordAsync();
}

await csvWriter.FlushAsync();

Can you help me understand how I can get rid of AllowSynchronousIO with 13.0.0?

[JoshClose]

Is the compiler complaining? If so, what is the output?

[cypressious]

@JoshClose It fails at runtime with

System.InvalidOperationException: Synchronous operations are disallowed. Call WriteAsync or set AllowSynchronousIO to true instead.
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http.HttpResponseStream.Write(Byte[] buffer, Int32 offset, Int32 count)
   at System.IO.StreamWriter.Flush(Boolean flushStream, Boolean flushEncoder)
   at System.IO.StreamWriter.Dispose(Boolean disposing)
   at System.IO.TextWriter.Dispose()
   at CsvHelper.WritingContext.Dispose(Boolean disposing)
   at CsvHelper.WritingContext.Dispose()
   at CsvHelper.CsvSerializer.Dispose(Boolean disposing)
   at CsvHelper.CsvSerializer.Dispose()
   at CsvHelper.CsvWriter.Dispose(Boolean disposing)
   at CsvHelper.CsvWriter.Dispose()

[AdamDotNet]

@cypressious That stacktrace says the CsvWriter is getting disposed synchronously, which is calling dispose on the underlying TextWriter. The synchronous dispose is occurring before your await using var writer triggers the StreamWriter's async dispose. Calling csvWriter.FlushAsync only writes to the underlying buffer, and does not flush the underlying buffer, which is why synchronous dispose called synchronous flush, thus triggering the exception.

CsvWriuter.FlushAsync
CsvSerizlier.WriteAsync

If CsvWriter was an IAsyncDisposable, then this problem more easily goes away. However, there's a comment on the feature request that shows an example workaround involving calling the underlying TextWriter's FlushAsync prior to disposing the CsvWriter.

[JoshClose]

Would adding IAsyncDisposable fix this? That's an easy enough change.

[AdamDotNet]

I do believe making CsvWriter (and friends down the chain) IAsyncDisposable will fix that. It may be worth noting that only .NetStandard 2.1 or .NetCore 3.0+ have BCL support for TextWriter.DisposeAsync. So I see a few options:

1. Only add IAsyncDisposable support to .NetStandard 2.1
2. Add support for all targets, by calling calling TextWriter.FlushAsync inside of your implementation of AsyncDispose. It'll still mean that synchronous dispose is called, but since the buffer will be empty, no exception should be thrown.
3. #if NETSTANDARD2_1 to use BCL support for TextWriter.DisposeAsync, but fallback to TextWriter.FlushAsync + an inevitable/harmless synchronous Dispose for other targets.

[cypressious]

Having IAsyncDisposable would be great.

[JoshClose]

@AdamDotNet Any idea what I need to do in this case?

await writer?.DisposeAsync().ConfigureAwait(false);

I get the error

'ConfiguredValueTaskAwaitable?' does not contain a definition for 'GetAwaiter' and no accessible extension method 'GetAwaiter' accepting a first argument of type 'ConfiguredValueTaskAwaitable?' could be found (are you missing a using directive or an assembly reference?)

If I remove the await it compiles, but it should be awaited.

[AdamDotNet]

@JoshClose - I couldn't find the C# Lang repo comment on it, but I don't believe it's possible to await null, so while it's really clean to do resource?.Dispose(), it's not possible to await resource?.DisposeAsync() because if resource is null, the await will try to await null and throw a NullReferenceException. Additionally, ValueTask is a struct, so that might be why ValueTask? is triggering the build error, not entirely sure.

In any case, you just need to be verbose, which stinks when ?. is otherwise so awesome.

if (writer != null)
{
   await writer.DisposeAsync().ConfigureAwait(false);
}

[JoshClose]

That is a bad compiler error message. It should say it can't await null or something.

I'm adding IAsyncDisposable to net47 and netstandard2.0 and up. For netstandard2.1 I'm calling DisposeAsync and for net47 and netstandard2.0 i'm calling TextWriter.FlushAsync then Dispose.

I will publish a beta version on NuGet once I have this going so you guys can verify it works as you expect.

[JoshClose]

@AdamDotNet Do you see a need to add IAsyncDisposable to the reading classes? TextReader doesn't implement it.

[AdamDotNet]

@JoshClose - I believe only the writers needs to be IAsyncDisposable, and not the readers.

[JoshClose]

Take a look at the code and let me know if there is anything missing. https://github.com/JoshClose/CsvHelper/commit/360e76e8422297b992815f6a98a2dff60423d2d0

I'm working on getting a pre-release build out, but GitVersion is being a PITA as usual.

[AdamDotNet]

@JoshClose - I left a couple of comments on your commit.

[JoshClose]

I just pushed the recommended changes.

[AdamDotNet]

I think it looks good.

[JoshClose]

Beta package is on NuGet. https://www.nuget.org/packages/CsvHelper/14.0.0-beta001

Can you guys give it a try? If it works and doesn't throw an error, I'll publish a full release.

[cypressious]

14 beta 1 is working without AllowSynchronousIO. Thank you very much.

[AdamDotNet]

That beta package looks to work as expected for me too.

You can close #1376 now that IAsyncDisposable is in place. Thank you!

[JoshClose]

In release 14.0.0 on NuGet.