Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add async alternatives to File.ReadAll*, File.AppendAll* and File.WriteAll* #18344

Closed
khellang opened this issue Aug 29, 2016 · 49 comments
Closed
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.IO help wanted [up-for-grabs] Good issue for external contributors
Milestone

Comments

@khellang
Copy link
Member

Is there a reason why there's no ReadAll*Async, AppendAll*Async and WriteAll*Async methods on File?

The existing methods are very convenient and I think it makes sense to have async versions available.

I propose the following additions:

public static partial class File
{
     public static void AppendAllLines(string path, IEnumerable<string> contents) { }
     public static void AppendAllLines(string path, IEnumerable<string> contents, Encoding encoding) { }
+    public static Task AppendAllLinesAsync(string path, IEnumerable<string> contents, CancellationToken cancellationToken = default(CancellationToken)) { return default(Task); }
+    public static Task AppendAllLinesAsync(string path, IEnumerable<string> contents, Encoding encoding, CancellationToken cancellationToken = default(CancellationToken)) { return default(Task); }

     public static void AppendAllText(string path, string contents) { }
     public static void AppendAllText(string path, string contents, Encoding encoding) { }
+    public static Task AppendAllTextAsync(string path, string contents, CancellationToken cancellationToken = default(CancellationToken)) { return default(Task); }
+    public static Task AppendAllTextAsync(string path, string contents, Encoding encoding, CancellationToken cancellationToken = default(CancellationToken)) { return default(Task); }

     public static byte[] ReadAllBytes(string path) { return default(byte[]); }
+    public static Task<byte[]> ReadAllBytesAsync(string path, CancellationToken cancellationToken = default(CancellationToken)) { return default(Task<byte[]>); }

     public static string[] ReadAllLines(string path) { return default(string[]); }
     public static string[] ReadAllLines(string path, System.Text.Encoding encoding) { return default(string[]); }
+    public static Task<string[]> ReadAllLinesAsync(string path, CancellationToken cancellationToken = default(CancellationToken)) { return default(Task<string[]>); }
+    public static Task<string[]> ReadAllLinesAsync(string path, Encoding encoding, CancellationToken cancellationToken = default(CancellationToken)) { return default(Task<string[]>); }

     public static string ReadAllText(string path) { return default(string); }
     public static string ReadAllText(string path, System.Text.Encoding encoding) { return default(string); }
+    public static Task<string> ReadAllTextAsync(string path, CancellationToken cancellationToken = default(CancellationToken)) { return default(Task<string>); }
+    public static Task<string> ReadAllTextAsync(string path, Encoding encoding, CancellationToken cancellationToken = default(CancellationToken)) { return default(Task<string>); }

     public static void WriteAllBytes(string path, byte[] bytes) { }
+    public static Task WriteAllBytesAsync(string path, byte[] bytes, CancellationToken cancellationToken = default(CancellationToken)) { return default(Task); }

     public static void WriteAllLines(string path, IEnumerable<string> contents) { }
     public static void WriteAllLines(string path, IEnumerable<string> contents, Encoding encoding) { }
+    public static Task WriteAllLinesAsync(string path, IEnumerable<string> contents, CancellationToken cancellationToken = default(CancellationToken)) { return default(Task); }
+    public static Task WriteAllLinesAsync(string path, IEnumerable<string> contents, Encoding encoding, CancellationToken cancellationToken = default(CancellationToken)) { return default(Task); }

     public static void WriteAllText(string path, string contents) { }
     public static void WriteAllText(string path, string contents, Encoding encoding) { }
+    public static Task WriteAllTextAsync(string path, string contents, CancellationToken cancellationToken = default(CancellationToken)) { return default(Task); }
+    public static Task WriteAllTextAsync(string path, string contents, Encoding encoding, CancellationToken cancellationToken = default(CancellationToken)) { return default(Task); }
}

I guess the ReadLines methods are candidates for this as well, but I don't know how you'd handle the async enumeration part yet.

@NickCraver
Copy link
Member

NickCraver commented Aug 29, 2016

Note: there's the relevant File.ReadLines with the async IEnumerable work...depending on how that progresses as well as all the Set___ methods, like File.SetAccessControl. All would be welcome additions, especially for network share files and such. Even File.Exists() and pretty much all others are candidates, IMO.

@khellang
Copy link
Member Author

khellang commented Aug 29, 2016

Note: there's also the relevant File.ReadLines with the async IEnumerable work... depending on how that progresses. It'd be a welcome addition as well, especially for network share files and such.

Yup, hence the comment:

I guess the ReadLines methods are candidates for this as well, but I don't know how you'd handle the async enumeration part yet.

😉

@NickCraver
Copy link
Member

Here's the async sequences discussion for those that want to follow: dotnet/roslyn#261

@khellang
Copy link
Member Author

khellang commented Sep 2, 2016

What does "api-needs-work" mean? Are you interested in a PR for this? Does it need a review first? I'd love to submit a PR, if you can walk me through the process 😄

@stephentoub
Copy link
Member

@terrajobst can correct me if I misapplied it; my understanding is that it essentially means "an API proposal that's still being discussed". Some point when the owner (aka @JeremyKuhne) deems it ready for review, the label changes to api-ready-for-review. The process is outlined here: https://github.com/dotnet/corefx/blob/master/Documentation/project-docs/api-review-process.md

@khellang
Copy link
Member Author

khellang commented Sep 2, 2016

@stephentoub Thanks. Yeah, I spoke too soon. Found it in the process document right after I asked 😝 I'll await (no pun intended) @JeremyKuhne's response 😄

@JeremyKuhne
Copy link
Member

I think it's reasonable to pursue this. Can you flesh out with usage examples and error conditions? I'll mark it for review after that.

@khellang
Copy link
Member Author

khellang commented Sep 7, 2016

Can you flesh out with usage examples and error conditions?

You mean show some sample code calling some of these methods?

These are async counterparts to the existing sync methods, so the error conditions would be the same as those, I assume.

@JeremyKuhne
Copy link
Member

@khellang, yes, sample code. Nothing super complicated, just something to help clearly illustrate the value here. And it is fine to say that we follow the error conditions in the sync methods with a callout to what we do with the CancellationToken.

@khellang
Copy link
Member Author

OK, an example. Deserializing JSON configuration:

public async Task<TConfiguration> ReadConfigurationAsync<TConfiguration>(string path)
{
    var json = await File.ReadAllTextAsync(path);
    return JsonConvert.DeserializeObject<TConfiguration>(json);
}

If you need more examples on usage of File.ReadAllText or File.ReadAllLines etc., you can also do a quick GitHub search:

The usage would be exactly the same, only in an async world.

@khellang
Copy link
Member Author

khellang commented Sep 23, 2016

Also, FYI; the reason I filed this is that I'm getting upvotes on an answer I gave on Stack Overflow almost weekly. The question currently has 7,753 views.

@khellang
Copy link
Member Author

khellang commented Sep 23, 2016

I amended the proposal to remove the CancellationToken arguments for the [Read|Write|Append]AllTextAsync APIs, cause I couldn't see a good way to handle cancellation for those (the underlying StreamReader.ReadToEndAsync doesn't take a CancellationToken).

If you want to look at my initial stab at an implementation, see dotnet/corefx@master...khellang:async-file-apis~1.

For the underlying (typically StreamReader) APIs accepting a CancellationToken, I've just passed the token along. For the others (typically those that loop through lines) I've just called ThrowIfCancellationRequested.

@karelz
Copy link
Member

karelz commented Nov 12, 2016

@JeremyKuhne what else do we need to move this API approval forward? @khellang has implementation ready, waiting just on us ...

@karelz
Copy link
Member

karelz commented Nov 15, 2016

@JeremyKuhne ping?
cc: @ianhays

@karelz
Copy link
Member

karelz commented Nov 16, 2016

@JeremyKuhne @ianhays ping?

@ianhays
Copy link
Contributor

ianhays commented Nov 16, 2016

I like these. There are a few places where we have helpers like this that lack async equivalents e.g. ZipFile. I doubt we'll face much resistance to adding these, so lets take it to the next API review session.

@ianhays ianhays self-assigned this Nov 16, 2016
@stephentoub
Copy link
Member

@khellang, how did you decide whether to have an overload that does / does not take a CancellationToken?

@khellang
Copy link
Member Author

khellang commented Nov 16, 2016

how did you decide whether to have an overload that does / does not take a CancellationToken?

First I wanted all of them to take a CancellationToken, but since most of the methods deal with FileStream and a Stream[Reader|Writer], I was restricted to either checking cancellation in a loop, or pass the token along to the reader/writer API and let it handle cancellation.

Unfortunately, the StreamReader.ReadToEndAsync and StreamWriter.WriteAsync methods doesn't take a ´CancellationToken´.

@stephentoub
Copy link
Member

@khellang, thanks.

Unfortunately, the StreamReader.ReadToEndAsync and StreamWriter.WriteAsync methods doesn't take a ´CancellationToken´.

We don't need to be constrained by what's currently available on StreamReader/StreamWriter... we don't even need to use StreamReader/Writer in the implementation if we don't want to, it's an implementation detail. Let's design the APIs we want and then figure out how to implement them.

since most of the methods deal with FileStream and a Stream[Reader|Writer]

What's the limitation with FileStream?

First I wanted all of them to take a CancellationToken

I think it makes sense for there to be an overload for each that takes a CancellationToken. Can you revise it accordingly, based on what you think the ideal is?

@khellang
Copy link
Member Author

khellang commented Nov 16, 2016

we don't even need to use StreamReader/Writer in the implementation if we don't want to, it's an implementation detail

Yeah, I just followed the implementation of what was already there (the sync overloads). Is there a reason why those reader/writer methods don't take a CancellationToken in the first place?

What's the limitation with FileStream?

Nothing. Where I've used the FileStream directly, I've used methods that take a CancellationToken.

I think it makes sense for there to be an overload for each that takes a CancellationToken. Can you revise it accordingly, based on what you think the ideal is?

Alright. I'll change it back to include cancellation tokens for the [Read|Write|Append]AllTextAsync APIs 😄

@stephentoub
Copy link
Member

Is there a reason why those reader/writer methods don't take a CancellationToken in the first place?

Why the synchronous ones don't? There are very few synchronous methods in the framework that take a CancellationToken; the only ones I can think of are on synchronization primitives, e.g. Wait(CancellationToken). While there is some value in adding sync methods that take CancellationToken, it's generally much less valuable than for the async ones, because of usage patterns but also because of the underlying mechanisms being used and whether they support cancellation.

@khellang
Copy link
Member Author

khellang commented Nov 16, 2016

No, I mean StreamReader.ReadToEndAsync and StreamWriter.WriteAsync 😄

That's a pretty old issue; https://connect.microsoft.com/VisualStudio/feedback/details/790215/textreader-and-streamreader-read-async-methods-are-missing-a-cancellationtoken-parameter

@stephentoub
Copy link
Member

No, I mean StreamReader.ReadToEndAsync and StreamWriter.WriteAsync

Oh... no idea why they were introduced without them ;)

@khellang
Copy link
Member Author

Oh... no idea why they were introduced without them ;)

So... do I smell another API request/proposal/PR? 😉

@stephentoub
Copy link
Member

So... do I smell another API request/proposal/PR?

That ones a bit trickier, as there are virtual methods involved; ideally the virtual one would have been the one accepting a CancellationToken. We also don't want things proposed just because there's a theoretical gap, but rather something that is actually needed (I'm not casting judgement on this case, just citing that we only want to spend time on things that actually move the needle). But you're of course welcome to propose something if you think it's important.

@JeremyKuhne
Copy link
Member

I'm happy with the current state of the proposal, sorry about the dead air.

@terrajobst
Copy link
Member

Any reason we wouldn't make the cancellation token optional?

 public static partial class File
 {
      public static void AppendAllLines(string path, IEnumerable<string> contents);
      public static void AppendAllLines(string path, IEnumerable<string> contents, Encoding encoding);
+     public static Task AppendAllLinesAsync(string path, IEnumerable<string> contents, CancellationToken cancellationToken = default(CancellationToken));
+     public static Task AppendAllLinesAsync(string path, IEnumerable<string> contents, Encoding encoding, CancellationToken cancellationToken = default(CancellationToken)));

      // ...
 }

@khellang
Copy link
Member Author

khellang commented Nov 16, 2016

Any reason we wouldn't make the cancellation token optional?

Nope. I had them optional, but have been back and forth so much, I guess they got dropped at some point 😝

JonHanna referenced this issue in JonHanna/corefx Dec 16, 2016
@JonHanna
Copy link
Contributor

PTAL @ dotnet/corefx#14557

JonHanna referenced this issue in JonHanna/corefx Dec 16, 2016
JonHanna referenced this issue in JonHanna/corefx Dec 16, 2016
JonHanna referenced this issue in JonHanna/corefx Dec 17, 2016
JonHanna referenced this issue in JonHanna/corefx Dec 17, 2016
JonHanna referenced this issue in JonHanna/corefx Dec 17, 2016
JonHanna referenced this issue in JonHanna/corefx Dec 17, 2016
JonHanna referenced this issue in JonHanna/corefx Dec 17, 2016
JonHanna referenced this issue in JonHanna/coreclr Dec 20, 2016
JonHanna referenced this issue in JonHanna/coreclr Dec 20, 2016
JonHanna referenced this issue in JonHanna/corefx Dec 21, 2016
@JonHanna
Copy link
Contributor

Okay. There's something I'm not getting about the different targets. I thought at least part of the problem was conflicts with mscorlib but it seems from dotnet/coreclr#8690 (comment) that I was wrong on that.

I'm giving up. If someone wants to take the changes in the PR above as a starting point, I was pretty happy with the actual implementation and test code.

@JonHanna
Copy link
Contributor

I was hoping that the recent engineering changes would have made my difficulty earlier irrelevant, and it seems it has. PTAL at dotnet/corefx#15372.

JonHanna referenced this issue in JonHanna/corefx Jan 22, 2017
JonHanna referenced this issue in JonHanna/corefx Jan 22, 2017
JonHanna referenced this issue in JonHanna/corefx Jan 22, 2017
JonHanna referenced this issue in JonHanna/corefx Jan 23, 2017
JonHanna referenced this issue in JonHanna/corefx Jan 23, 2017
@msftgits msftgits transferred this issue from dotnet/corefx Jan 31, 2020
@msftgits msftgits added this to the 2.0.0 milestone Jan 31, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 29, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.IO help wanted [up-for-grabs] Good issue for external contributors
Projects
None yet
Development

No branches or pull requests