Add async File.ReadAll, File.AppendAll and File.WriteAll #15372

JonHanna · 2017-01-21T21:40:15Z

Fixes #11220

jamesqo · 2017-01-22T15:38:39Z

src/System.IO.FileSystem/src/System/IO/File.cs

+
+        // UTF-8 without BOM and with error detection.
+        // Same as the default encoding for StreamWriter.
+        private static Encoding UTF8NoBOM => _UTF8NoBOM ?? (_UTF8NoBOM = new UTF8Encoding(false, true));


Nit: Would named parameters help here?

Well, they wouldn't hurt.

jamesqo · 2017-01-22T15:38:55Z

src/System.IO.FileSystem/src/System/IO/File.cs

+                path, FileMode.Open, FileAccess.Read, FileShare.Read, DefaultBufferSize,
+                FileOptions.Asynchronous | FileOptions.SequentialScan);
+
+            return new StreamReader(stream, encoding, true);


Would named parameters help here?

jamesqo · 2017-01-22T15:41:55Z

src/System.IO.FileSystem/src/System/IO/File.cs

+                cancellationToken.ThrowIfCancellationRequested();
+                StringBuilder sb = new StringBuilder();
+                int bufferSize = sr.CurrentEncoding.GetMaxCharCount(DefaultBufferSize);
+                char[] buffer = new char[bufferSize];


Maybe StringBuilderCache.Acquire() and ArrayPool<char>.Shared.Rent() respectively could be used here.

jamesqo · 2017-01-22T15:44:27Z

src/System.IO.FileSystem/src/System/IO/File.cs

+
+            return cancellationToken.IsCancellationRequested
+                ? Task.FromCanceled(cancellationToken)
+                : InternalWriteAllTextAsync(AsyncStreamWriter(path, false, encoding), contents, cancellationToken);


Nit: Named parameters

jamesqo · 2017-01-22T15:45:45Z

src/System.IO.FileSystem/src/System/IO/File.cs

+            using (fs)
+            {
+                int index = 0;
+                byte[] bytes = new byte[count];


Maybe use ArrayPool<byte>.Shared.Rent(count)?

Not so useful here, as that array is returned back to the client.

Ah, I missed that.

jamesqo · 2017-01-22T15:46:45Z

src/System.IO.FileSystem/src/System/IO/File.cs

+
+            return cancellationToken.IsCancellationRequested
+                ? Task.FromCanceled(cancellationToken)
+                : InternalWriteAllLinesAsync(AsyncStreamWriter(path, false, encoding), contents, cancellationToken);


Nit: Named parameters

jamesqo · 2017-01-22T15:46:54Z

src/System.IO.FileSystem/src/System/IO/File.cs

+                : InternalWriteAllLinesAsync(AsyncStreamWriter(path, false, encoding), contents, cancellationToken);
+        }
+
+        private static async Task InternalWriteAllLinesAsync(TextWriter writer, IEnumerable<String> contents, CancellationToken cancellationToken)


Nit: string

jamesqo · 2017-01-22T15:48:17Z

src/System.IO.FileSystem/src/System/IO/File.cs

+                if (!string.IsNullOrEmpty(contents))
+                {
+                    cancellationToken.ThrowIfCancellationRequested();
+                    char[] buffer = new char[DefaultBufferSize];


ArrayPool? Same for the other allocations happening.

jamesqo · 2017-01-22T15:49:01Z

src/System.IO.FileSystem/src/System/IO/File.cs

+
+            return cancellationToken.IsCancellationRequested
+                ? Task.FromCanceled(cancellationToken)
+                : InternalWriteAllTextAsync(AsyncStreamWriter(path, true, encoding), contents, cancellationToken);


Named parameters? Same for all of the other places with true, false, null literals being passed directly.

JonHanna · 2017-01-22T17:16:01Z

Thanks @jamesqo. I'm now making use of shared buffers in those cases that don't return the array to the client. I'm using the length of the buffer obtained rather than the size asked for with reading but not for writing. With reading it's up to the stream implementation whether it uses the full size or smaller, but with writing it has to use all of it, so that is more likely to be sub-optimal.

jamesqo · 2017-01-22T17:39:08Z

src/System.IO.FileSystem/src/System/IO/File.cs

+                    int read = await sr.ReadAsync(buffer, 0, buffer.Length).ConfigureAwait(false);
+                    if (read == 0)
+                    {
+                        ArrayPool<char>.Shared.Return(buffer);


I think you need to use a try/finally pattern with ArrayPool so the buffer is returned in the case of an exception. That is the general usage from what I've seen, at least.

I was wondering about that. It's non-fateful but less than ideal for such a thing to happen. I think I'll replace the using with a single try/finally and then there's no further impact.

jamesqo · 2017-01-22T17:40:29Z

src/System.IO.FileSystem/src/System/IO/File.cs

+            using (fs)
+            {
+                int index = 0;
+                byte[] bytes = new byte[count];


Ah, I missed that.

jamesqo · 2017-01-22T17:48:10Z

src/System.IO.FileSystem/src/System/IO/File.cs

+                foreach (string line in contents)
+                {
+                    cancellationToken.ThrowIfCancellationRequested();
+                    // Note that this working depends on the fix to #8662, and cannot be ported without


#8662 seems to be a closed issue

It is. This is a note in case it's ported to desktop. If this is ported and the fix for #8662 isn't then this will have a bug.

Darn it. Not 8662, but #14563. 8662 was a related PR number in coreclr.

justinvp · 2017-01-22T21:15:19Z

src/System.IO.FileSystem/src/System/IO/File.cs


 namespace System.IO
 {
    // Class for creating FileStream objects, and some basic file management
    // routines such as Delete, etc.
    public static class File
    {
+        private static Encoding _UTF8NoBOM;


Static fields should be prefixed with s_

Indeed they should. Fixed.

ianhays · 2017-01-23T21:07:55Z

src/System.IO.FileSystem/tests/File/ReadWriteAllBytesAsync.cs

+            File.Delete(path);
+        }
+
+        public Task AlreadyCanceledAsync()


You forgot a [Fact] on this one.

I'd also forgot to cancel the token!

ianhays · 2017-01-23T21:09:58Z

src/System.IO.FileSystem/tests/File/ReadWriteAllTextAsync.cs

+        public async Task NullParametersAsync()
+        {
+            string path = GetTestFilePath();
+            await Assert.ThrowsAsync<ArgumentNullException>(async () => await WriteAsync(null, "Text"));


does ThrowsAsync allow assertion of parameterNames on Argument*Exceptions?

It does, thanks.

Fixes #11220

danmoseley · 2017-02-02T01:41:41Z

@ianhays is this ready for another review?

ianhays

I took another look and this LGTM. Thanks Jon!

khellang · 2017-02-02T20:27:33Z

Nice! Will this be part of .NET Core/Standard/Framework vNext?

karelz · 2017-02-02T20:53:22Z

It will be in .NET Core 2.0.
I won't be part of .NET Standard 2.0 ... we will have to wait for other platforms (Desktop Framework, Xamarin) to adopt it first.

karelz · 2017-02-02T20:53:42Z

Thanks @JonHanna for your help here!

Add async File.ReadAll*, File.AppendAll* and File.WriteAll Commit migrated from dotnet/corefx@2b4e1e4

dnfclas added the cla-already-signed label Jan 21, 2017

JonHanna force-pushed the fix_11220_redux branch 4 times, most recently from dcdd69d to da20360 Compare January 22, 2017 00:01

danmoseley requested review from JeremyKuhne and ianhays January 22, 2017 03:23

jamesqo reviewed Jan 22, 2017

View reviewed changes

JonHanna force-pushed the fix_11220_redux branch from da20360 to 662dc6e Compare January 22, 2017 17:09

jamesqo reviewed Jan 22, 2017

View reviewed changes

JonHanna force-pushed the fix_11220_redux branch from 662dc6e to 369cc91 Compare January 22, 2017 18:57

justinvp reviewed Jan 22, 2017

View reviewed changes

JonHanna force-pushed the fix_11220_redux branch 2 times, most recently from 2a49b66 to 241fc4e Compare January 23, 2017 10:14

karelz added the area-System.IO label Jan 23, 2017

karelz assigned JonHanna, ianhays and JeremyKuhne Jan 23, 2017

ianhays reviewed Jan 23, 2017

View reviewed changes

Add async File.ReadAll*, File.AppendAll* and File.WriteAll

8afa432

Fixes #11220

JonHanna force-pushed the fix_11220_redux branch from 241fc4e to 8afa432 Compare January 23, 2017 21:55

ianhays approved these changes Feb 2, 2017

View reviewed changes

ianhays merged commit 2b4e1e4 into dotnet:master Feb 2, 2017

JonHanna deleted the fix_11220_redux branch February 2, 2017 18:59

karelz added this to the 2.0.0 milestone Feb 2, 2017

picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022

Merge pull request dotnet/corefx#15372 from JonHanna/fix_11220_redux

84343cb

Add async File.ReadAll*, File.AppendAll* and File.WriteAll Commit migrated from dotnet/corefx@2b4e1e4

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add async File.ReadAll, File.AppendAll and File.WriteAll #15372

Add async File.ReadAll, File.AppendAll and File.WriteAll #15372

JonHanna commented Jan 21, 2017

jamesqo Jan 22, 2017

JonHanna Jan 22, 2017

jamesqo Jan 22, 2017

jamesqo Jan 22, 2017

jamesqo Jan 22, 2017

jamesqo Jan 22, 2017

JonHanna Jan 22, 2017

jamesqo Jan 22, 2017

jamesqo Jan 22, 2017

jamesqo Jan 22, 2017

jamesqo Jan 22, 2017

jamesqo Jan 22, 2017

JonHanna commented Jan 22, 2017

jamesqo Jan 22, 2017

JonHanna Jan 22, 2017

jamesqo Jan 22, 2017

jamesqo Jan 22, 2017

JonHanna Jan 22, 2017

JonHanna Jan 23, 2017

justinvp Jan 22, 2017

JonHanna Jan 22, 2017

ianhays Jan 23, 2017

JonHanna Jan 23, 2017

ianhays Jan 23, 2017

JonHanna Jan 23, 2017

danmoseley commented Feb 2, 2017

ianhays left a comment

khellang commented Feb 2, 2017

karelz commented Feb 2, 2017

karelz commented Feb 2, 2017

Add async File.ReadAll*, File.AppendAll* and File.WriteAll #15372

Add async File.ReadAll*, File.AppendAll* and File.WriteAll #15372

Conversation

JonHanna commented Jan 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JonHanna commented Jan 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danmoseley commented Feb 2, 2017

ianhays left a comment

Choose a reason for hiding this comment

khellang commented Feb 2, 2017

karelz commented Feb 2, 2017

karelz commented Feb 2, 2017

Add async File.ReadAll, File.AppendAll and File.WriteAll #15372

Add async File.ReadAll, File.AppendAll and File.WriteAll #15372