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 isAsync overload for Create/Open/etc. #24698

Closed
Duranom opened this issue Jan 17, 2018 · 22 comments · Fixed by #52720
Closed

Add isAsync overload for Create/Open/etc. #24698

Duranom opened this issue Jan 17, 2018 · 22 comments · Fixed by #52720
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.IO
Milestone

Comments

@Duranom
Copy link

Duranom commented Jan 17, 2018

Background and Motivation

When using System.IO.File and/or System.IO.FileInfo for opening, creating and writing files you do not have the option to properly open FileStreams for use with async methods.

A very common mistake/issue that occurs because of it is that files are opened through methods like File.OpenWrite or new FileInfo(x).Open() and the FileStream is then used with async/await operations like WriteAsync or ReadAsync. However when this is done you will incur a performance hit as the stream is not configured to be used in this way.

The proper way to use FileStreams with async await is to open a FileStream through the constructor with and supplying the argument isAsync with true. However System.IO.File and System.IO.FileInfo expose a lot of convenient shorthands but lack the possibility to open the streams properly for async actions. leading also to the misconception that async await with file IO is actually not good.

Proposed API

@adamsitnik proposed the use of a FileOptions and supply additional overloads to System.IO.StreamReader, System.IO.StreamWriter , System.IO.File and System.IO.FileInfo so not only Files can be opened with FileOptions.Asynchronous but also with FileOptions.SequentialScan.
Comment: #24698 (comment)

public static partial class File
{
    public StreamWriter AppendText(string path)
+   public StreamWriter AppendText(string path, FileOptions options)
    public static System.IO.FileStream Create(string path)
+   public static System.IO.FileStream Create(string path, System.IO.FileOptions options)
    public static System.IO.FileStream Create(string path, int bufferSize)
    public static System.IO.FileStream Create(string path, int bufferSize, System.IO.FileOptions options)
    public static System.IO.StreamWriter CreateText(string path)
+   public static System.IO.StreamWriter CreateText(string path, System.IO.FileOptions options)   
    public static System.IO.FileStream Open(string path, System.IO.FileMode mode)
+   public static System.IO.FileStream Open(string path, System.IO.FileMode mode, System.IO.FileOptions options)
    public static System.IO.FileStream Open(string path, System.IO.FileMode mode, System.IO.FileAccess access)
+   public static System.IO.FileStream Open(string path, System.IO.FileMode mode, System.IO.FileAccess access, System.IO.FileOptions options)
    public static System.IO.FileStream Open(string path, System.IO.FileMode mode, System.IO.FileAccess access, System.IO.FileShare share)
+   public static System.IO.FileStream Open(string path, System.IO.FileMode mode, System.IO.FileAccess access, System.IO.FileShare share, System.IO.FileOptions options)
    public static System.IO.FileStream OpenRead(string path) { throw null; }
+   public static System.IO.FileStream OpenRead(string path, System.IO.FileOptions options) { throw null; }
    public static System.IO.StreamReader OpenText(string path) { throw null; }
+   public static System.IO.StreamReader OpenText(string path, System.IO.FileOptions options) { throw null; }
    public static System.IO.FileStream OpenWrite(string path) { throw null; }
+   public static System.IO.FileStream OpenWrite(string path, System.IO.FileOptions options) { throw null; }
}

note that File.Create(string path, int bufferSize, System.IO.FileOptions options) already exists.

public sealed partial class FileInfo : System.IO.FileSystemInfo
{
    public StreamWriter AppendText()
+   public StreamWriter AppendText(FileOptions options)
    public System.IO.FileStream Create()
+   public System.IO.FileStream Create(System.IO.FileOptions options)
    public System.IO.StreamWriter CreateText()
+   public System.IO.StreamWriter CreateText(System.IO.FileOptions options)
    public System.IO.FileStream Open(System.IO.FileMode mode)
+   public System.IO.FileStream Open(System.IO.FileMode mode, System.IO.FileOptions options)
    public System.IO.FileStream Open(System.IO.FileMode mode, System.IO.FileAccess access)
+   public System.IO.FileStream Open(System.IO.FileMode mode, System.IO.FileAccess access, System.IO.FileOptions options)
    public System.IO.FileStream Open(System.IO.FileMode mode, System.IO.FileAccess access, System.IO.FileShare share)
+   public System.IO.FileStream Open(System.IO.FileMode mode, System.IO.FileAccess access, System.IO.FileShare share, System.IO.FileOptions options)
    public System.IO.FileStream OpenRead()
+   public System.IO.FileStream OpenRead(System.IO.FileOptions options)
    public System.IO.StreamReader OpenText()
+   public System.IO.StreamReader OpenText(System.IO.FileOptions options)    
    public System.IO.FileStream OpenWrite()
+   public System.IO.FileStream OpenWrite(System.IO.FileOptions options)
}
public partial class StreamReader : System.IO.TextReader
{
    public StreamReader(string path)
+   public StreamReader(string path, System.IO.FileOptions options)
    public StreamReader(string path, bool detectEncodingFromByteOrderMarks)
+   public StreamReader(string path, bool detectEncodingFromByteOrderMarks, System.IO.FileOptions options)
    public StreamReader(string path, System.Text.Encoding encoding)
+   public StreamReader(string path, System.Text.Encoding encoding, System.IO.FileOptions options)
    public StreamReader(string path, System.Text.Encoding encoding, bool detectEncodingFromByteOrderMarks)
+   public StreamReader(string path, System.Text.Encoding encoding, bool detectEncodingFromByteOrderMarks, System.IO.FileOptions options)
    public StreamReader(string path, System.Text.Encoding encoding, bool detectEncodingFromByteOrderMarks, int bufferSize)
+   public StreamReader(string path, System.Text.Encoding encoding, bool detectEncodingFromByteOrderMarks, int bufferSize, System.IO.FileOptions options)
}
public partial class StreamWriter : System.IO.TextWriter
{
    public StreamWriter(string path)
+   public StreamWriter(string path, System.IO.FileOptions options)
    public StreamWriter(string path, bool append)
+   public StreamWriter(string path, bool append, System.IO.FileOptions options)
    public StreamWriter(string path, bool append, System.Text.Encoding encoding)
+   public StreamWriter(string path, bool append, System.Text.Encoding encoding, System.IO.FileOptions options)
    public StreamWriter(string path, bool append, System.Text.Encoding encoding, int bufferSize)
+   public StreamWriter(string path, bool append, System.Text.Encoding encoding, int bufferSize, System.IO.FileOptions options)
}

Usage Examples

using FileStream filestream = File.OpenRead(Path.GetTempFileName(), FileOptions.Asynchronous);
byte[] bytes = Encoding.Unicode.GetBytes("example text to write");
await filestream.WriteAsync(bytes, 0, bytes.Length);

Alternative Designs

In the beginning an idea was put for exposing additional async API methods next to the existing methods and that would return return the stream in a Task / ValueTask and guide the developer immediately into the async environment, also taken into account that a lot of .NET Native only exposed async variants in the API's with IO file streams.
However as was mentioned in comments that streams are opened with constructors and would require a lot of changes and provide no additional benefit and more overhead.

Risks

@Duranom
Copy link
Author

Duranom commented Jan 23, 2018

Btw is there still a big requirement to have Filestreams by default not open for asynchronous operations?
As the performance hit is a lot less with synchronous operations on streams that are opened for asynchronous operations then it is the other way around.

@karelz karelz changed the title Addition of async method variants or argument option for opening/creating with System.IO.File and System.IO.FileInfo shorthands Add CreateAsync/OpenAsyc/etc. Jan 22, 2020
@karelz karelz changed the title Add CreateAsync/OpenAsyc/etc. Add CreateAsync/OpenAsync/etc. Jan 22, 2020
@karelz karelz changed the title Add CreateAsync/OpenAsync/etc. Add isAsync overload for Create/Open/etc. Jan 22, 2020
@msftgits msftgits transferred this issue from dotnet/corefx Jan 31, 2020
@msftgits msftgits added this to the Future milestone Jan 31, 2020
@adamsitnik
Copy link
Member

adamsitnik commented Jan 26, 2021

The proper way to use FileStreams with async await is to open the FileStream through the constructor with argument isAsync to true. However as File and FileInfo are more directly approached and provide easier shorthands for common actions the mistake is made often. Besides the performance hit this also creates a big misconception that async await with file IO is actually not good.

I agree that we should make it easier to open|create files for async IO. Especially since we want to invest in FileStream in .NET 6.0.

If an async variant could be added like File.OpenAsync,

The Async suffix suggests that given operation is asynchronous and returns a Task. As of today, the only way to open FileStream for async IO is to pass the right flag to the synchronous ctor.
Since FileStream does not offer any async API for opening files, I would prefer to not choose this solution (as it would require big changes in FileStream).

or have and variant with an added an argument like File.Open(bool isAsync)

I like File.Open(bool isAsync) because it's simple, but to make it more flexible we could add an overload that accepts FileOptions, which can be set to not just FileOptions.Asynchronous but can also be combined with other options like FileOptions.SequentialScan

Since File and FileInfo APIs rely heavily on StreamReader and StreamWriter we are going to need new ctors for these types:

public partial class StreamReader : System.IO.TextReader
{
    public StreamReader(string path)
+   public StreamReader(string path, System.IO.FileOptions options)
    public StreamReader(string path, bool detectEncodingFromByteOrderMarks)
+   public StreamReader(string path, bool detectEncodingFromByteOrderMarks,System.IO. FileOptions options)
    public StreamReader(string path, System.Text.Encoding encoding)
+   public StreamReader(string path, System.Text.Encoding encoding, System.IO.FileOptions options)
    public StreamReader(string path, System.Text.Encoding encoding, bool detectEncodingFromByteOrderMarks)
+   public StreamReader(string path, System.Text.Encoding encoding, bool detectEncodingFromByteOrderMarks, System.IO.FileOptions options)
    public StreamReader(string path, System.Text.Encoding encoding, bool detectEncodingFromByteOrderMarks, int bufferSize)
+   public StreamReader(string path, System.Text.Encoding encoding, bool detectEncodingFromByteOrderMarks, int bufferSize, System.IO.FileOptions options)
}
public partial class StreamWriter : System.IO.TextWriter
{
    public StreamWriter(string path)
+   public StreamWriter(string path, System.IO.FileOptions options)
    public StreamWriter(string path, bool append)
+   public StreamWriter(string path, bool append, System.IO.FileOptions options)
    public StreamWriter(string path, bool append, System.Text.Encoding encoding)
+   public StreamWriter(string path, bool append, System.Text.Encoding encoding, System.IO.FileOptions options)
    public StreamWriter(string path, bool append, System.Text.Encoding encoding, int bufferSize)
+   public StreamWriter(string path, bool append, System.Text.Encoding encoding, int bufferSize, System.IO.FileOptions options)
}

This should allow for:

public static partial class File
{
    public static System.IO.FileStream Create(string path)
+   public static System.IO.FileStream Create(string path, System.IO.FileOptions options)
    public static System.IO.FileStream Create(string path, int bufferSize)
    public static System.IO.FileStream Create(string path, int bufferSize, System.IO.FileOptions options)
    public static System.IO.StreamWriter CreateText(string path)
+   public static System.IO.StreamWriter CreateText(string path, System.IO.FileOptions options)   
    public static System.IO.FileStream Open(string path, System.IO.FileMode mode)
+   public static System.IO.FileStream Open(string path, System.IO.FileMode mode, System.IO.FileOptions options)
    public static System.IO.FileStream Open(string path, System.IO.FileMode mode, System.IO.FileAccess access)
+   public static System.IO.FileStream Open(string path, System.IO.FileMode mode, System.IO.FileAccess access, System.IO.FileOptions options)
    public static System.IO.FileStream Open(string path, System.IO.FileMode mode, System.IO.FileAccess access, System.IO.FileShare share)
+   public static System.IO.FileStream Open(string path, System.IO.FileMode mode, System.IO.FileAccess access, System.IO.FileShare share, System.IO.FileOptions options)
    public static System.IO.FileStream OpenRead(string path) { throw null; }
+   public static System.IO.FileStream OpenRead(string path) { throw null; }
    public static System.IO.StreamReader OpenText(string path) { throw null; }
+   public static System.IO.StreamReader OpenText(string path) { throw null; }
    public static System.IO.FileStream OpenWrite(string path) { throw null; }
+   public static System.IO.FileStream OpenWrite(string path) { throw null; }
}

note that File.Create(string path, int bufferSize, System.IO.FileOptions options) already exists.

public sealed partial class FileInfo : System.IO.FileSystemInfo
{
    public System.IO.FileStream Create()
+   public System.IO.FileStream Create(System.IO.FileOptions options)
    public System.IO.StreamWriter CreateText()
+   public System.IO.StreamWriter CreateText(System.IO.FileOptions options)
    public System.IO.FileStream Open(System.IO.FileMode mode)
+   public System.IO.FileStream Open(System.IO.FileMode mode, System.IO.FileOptions options)
    public System.IO.FileStream Open(System.IO.FileMode mode, System.IO.FileAccess access)
+   public System.IO.FileStream Open(System.IO.FileMode mode, System.IO.FileAccess access, System.IO.FileOptions options)
    public System.IO.FileStream Open(System.IO.FileMode mode, System.IO.FileAccess access, System.IO.FileShare share)
+   public System.IO.FileStream Open(System.IO.FileMode mode, System.IO.FileAccess access, System.IO.FileShare share, System.IO.FileOptions options)
    public System.IO.FileStream OpenRead()
+   public System.IO.FileStream OpenRead(System.IO.FileOptions options)
    public System.IO.StreamReader OpenText()
+   public System.IO.StreamReader OpenText(System.IO.FileOptions options)    
    public System.IO.FileStream OpenWrite()
+   public System.IO.FileStream OpenWrite(System.IO.FileOptions options)
}

@Duranom would the proposed API meet your needs?

@carlossanlop @jozkee since this task aligns with our .NET 6.0 goals, I am setting the 6.0 milestone for it

@adamsitnik adamsitnik modified the milestones: Future, 6.0.0 Jan 26, 2021
@adamsitnik adamsitnik self-assigned this Jan 26, 2021
@Duranom
Copy link
Author

Duranom commented Jan 26, 2021

Yes, think that will help with the misconception.

note that File.Create(string path, int bufferSize, System.IO.FileOptions options) already exists.

Just wondering how many use this and set the buffersize to 4096 (the default), think the File.Create(string path, System.IO.FileOptions options) would only be missing then.

@adamsitnik
Copy link
Member

think the File.Create(string path, System.IO.FileOptions options) would only be missing then

@Duranom good point! thank you for feedback!

@Duranom some time ago we have added an official template for API proposal. Is there any chance that you could update your issue description and fill the template? Then I would mark it as ready for review, introduce it in .NET Design Review Meeting and once we get the API approved, we can start coding and ship it ;)

@Duranom
Copy link
Author

Duranom commented Feb 21, 2021

Updated, apologies for delay

@adamsitnik
Copy link
Member

@Duranom thank you!

@adamsitnik adamsitnik added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Mar 25, 2021
@jozkee
Copy link
Member

jozkee commented Mar 29, 2021

Did you consider including AppendText in the proposal?

public static class File
{
    public StreamWriter AppendText(string path) { throw null; }
+   public StreamWriter AppendText(string path, FileOptions options) { throw null; }
}

public class FileInfo
{
    public StreamWriter AppendText() { throw null; }
+   public StreamWriter AppendText(FileOptions options) { throw null; }
}

@carlossanlop
Copy link
Member

@adamsitnik I have a suggestion and a question:

  • From our conversation earlier, let's order the API diffs by priority (File first, FileInfo second, StreamWriter/StreamReader third).

  • Why not use a FileStreamOptions type to wrap properties for FileOptions and allocationSize, so that we can pass an instance as argument to all the API proposals here and in FileStream file preallocation performance #45946, instead of creating all those APIs that take a FileOptions? The idea is that in the future, if users need to add another argument to a constructor, we can instead just add a new property to that type. I'm flexible with using another type name, FileStreamOptions is just a suggestion.
    Note: There's a comment in this issue's description stating that the API System.IO.FileStream Create(System.IO.FileOptions options) already exists. It's unfortunate that we wouldn't be able to keep consistency by adding similar overloads for the rest of the APIs, but I think having the FileStreamOptions type would be more flexible for future expansion of capabilities.

@terrajobst terrajobst added the blocking Marks issues that we want to fast track in order to unblock other important work label Mar 29, 2021
@adamsitnik
Copy link
Member

Did you consider including AppendText in the proposal?

I've missed it. I've added it after reading your comment. 👍

let's order the API diffs by priority

done

Why not use a FileStreamOptions type to wrap properties for FileOptions and allocationSize

Few things that come to my mind:

  • consistency - these are old types that have a lot of methods and ctors with overloads, imo we should rather not mix both approaches in the same type.
  • performance - if the new type is a class, then we increase the allocations. If it's a struct, users might get into subtle bugs when passing mutable struct by value
private FileStream CreateFileStream(FileStreamParameters parameters)
{
    EnsureIsAsyncSetToTrue(parameters); // method modifies a copy
    
    return new FileStream(parameters); // uses a different copy, where IsAsync might not be set to true
}

private static void EnsureIsAsyncSetToTrue(FileStreamParameters parameters)
{
    parameters.IsAsync = true;
}

@stephentoub
Copy link
Member

stephentoub commented Mar 30, 2021

I question whether this is actually a beneficial thing to do.

Even with all of the work being invested in FileStream this release (which I'm of course very glad we're doing, having helped spearhead it :), do you have some example scenarios where you've found using FileStreams created with FileOptions.Asynchronous yields better end-to-end performance? I expect it's primarily meaningful when accessing remote files, which isn't a super common use case. The proposed helpers are all convenience, not actually enabling anything that couldn't otherwise be done with one or two more lines of code (if that), so adding these means we want to encourage more devs to use the options with File/FileInfo/StreamWriter/StreamReader.

these are old types that have a lot of methods and ctors with overloads

So we're adding even more? 😉

@adamsitnik
Copy link
Member

@stephentoub From the examples that I've seen it's very common for the users to open the file for sync IO and later use it in async way. It's most probably the worst thing you can do from the performance perspective?

Some of them:

https://twitter.com/buhakmeh/status/1375497431962488838
https://github.com/Azure/azure-sdk-for-net/blob/e2fad94b95fa2a27a73af758e4e2c39b724c5949/sdk/storage/Azure.Storage.Blobs/src/BlobClient.cs#L1148-L1162

The main point of adding these overloads is just to make it easier for the end-users to use truly async file IO.

@stephentoub
Copy link
Member

stephentoub commented Mar 30, 2021

From the examples that I've seen it's very common for the users to open the file for sync IO and later use it in async way.

I'm not disputing that. But before you add a bazillion overloads, prove it's actually beneficial in practice, not just in theory. You might be surprised. We're not talking here about making something that wasn't possible now possible, we're talking about making something just a smidgen easier, by adding ~30 methods.

@bartonjs
Copy link
Member

bartonjs commented Mar 30, 2021

Video

Looks good as proposed

namespace System.IO
{
    public static partial class File
    {
        public StreamWriter AppendText(string path, FileOptions options) => throw null;
        public static System.IO.FileStream Create(string path, System.IO.FileOptions options) => throw null;
        public static System.IO.StreamWriter CreateText(string path, System.IO.FileOptions options) => throw null;   
        public static System.IO.FileStream Open(string path, System.IO.FileMode mode, System.IO.FileOptions options) => throw null;
        public static System.IO.FileStream Open(string path, System.IO.FileMode mode, System.IO.FileAccess access, System.IO.FileOptions options) => throw null;
        public static System.IO.FileStream Open(string path, System.IO.FileMode mode, System.IO.FileAccess access, System.IO.FileShare share, System.IO.FileOptions options) => throw null;
        public static System.IO.FileStream OpenRead(string path, System.IO.FileOptions options) => throw null; { throw null; }
        public static System.IO.StreamReader OpenText(string path, System.IO.FileOptions options) => throw null; { throw null; }
        public static System.IO.FileStream OpenWrite(string path, System.IO.FileOptions options) => throw null; { throw null; }
    }
    public sealed partial class FileInfo : System.IO.FileSystemInfo
    {
        public StreamWriter AppendText(FileOptions options) => throw null;
        public System.IO.FileStream Create(System.IO.FileOptions options) => throw null;
        public System.IO.StreamWriter CreateText(System.IO.FileOptions options) => throw null;
        public System.IO.FileStream Open(System.IO.FileMode mode, System.IO.FileOptions options) => throw null;
        public System.IO.FileStream Open(System.IO.FileMode mode, System.IO.FileAccess access, System.IO.FileOptions options) => throw null;
        public System.IO.FileStream Open(System.IO.FileMode mode, System.IO.FileAccess access, System.IO.FileShare share, System.IO.FileOptions options) => throw null;
        public System.IO.FileStream OpenRead(System.IO.FileOptions options) => throw null;
        public System.IO.StreamReader OpenText(System.IO.FileOptions options) => throw null;    
        public System.IO.FileStream OpenWrite(System.IO.FileOptions options) => throw null;
    }
    public partial class StreamReader : System.IO.TextReader
    {
        public StreamReader(string path, System.IO.FileOptions options) => throw null;
        public StreamReader(string path, bool detectEncodingFromByteOrderMarks, System.IO.FileOptions options) => throw null;
        public StreamReader(string path, System.Text.Encoding encoding, System.IO.FileOptions options) => throw null;
        public StreamReader(string path, System.Text.Encoding encoding, bool detectEncodingFromByteOrderMarks, System.IO.FileOptions options) => throw null;
        public StreamReader(string path, System.Text.Encoding encoding, bool detectEncodingFromByteOrderMarks, int bufferSize, System.IO.FileOptions options) => throw null;
    }
    public partial class StreamWriter : System.IO.TextWriter
    {
        public StreamWriter(string path, System.IO.FileOptions options) => throw null;
        public StreamWriter(string path, bool append, System.IO.FileOptions options) => throw null;
        public StreamWriter(string path, bool append, System.Text.Encoding encoding, System.IO.FileOptions options) => throw null;
        public StreamWriter(string path, bool append, System.Text.Encoding encoding, int bufferSize, System.IO.FileOptions options) => throw null;
    }
}

@bartonjs bartonjs added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Mar 30, 2021
@stephentoub stephentoub removed the blocking Marks issues that we want to fast track in order to unblock other important work label Apr 15, 2021
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label May 13, 2021
@manandre
Copy link
Contributor

I have created a PR (#52720) to implement these new overloads.

@stephentoub
Copy link
Member

Wouldn't it be simpler / more flexible for these overloads to be revised now that #52446 has been approved? e.g. we shouldn't have three new overloads of Open... if we want more functionality there, why wouldn't we just add one that takes the options bag?

@adamsitnik
Copy link
Member

@stephentoub that is our plan, but specifying preallocation size and the buffer is just higher on the priority list

@bartonjs
Copy link
Member

that is our plan, but

I'm confused. I think Steve's asking/suggesting that these new overloads not be added now, since the the functionality is present in the options bag one.

So if it's the plan, the PR and this issue would get closed. Unless the plan is do the work for this, then when adding the options bag remove these overloads, but that sounds like a more complicated plan.

@adamsitnik
Copy link
Member

adamsitnik commented May 18, 2021

By plan, I meant re-designing the API accepted in #24698 (comment) by adopting the new FileStreamOptions type and reducing the number of overloads:

namespace System.IO
{
    public static partial class File
    {
-        public StreamWriter AppendText(string path, FileOptions options)
+        public StreamWriter AppendText(string path, FileStreamOptions options)
-        public static FileStream Create(string path, FileOptions options)
+        public static FileStream Create(string path, FileStreamOptions options)
-        public static StreamWriter CreateText(string path, FileOptions options)   
+        public static StreamWriter CreateText(string path, FileStreamOptions options)  
-        public static FileStream Open(string path, FileMode mode, FileOptions options)
-        public static FileStream Open(string path, FileMode mode, FileAccess access, FileOptions options)
-        public static FileStream Open(string path, FileMode mode, FileAccess access, FileShare share, FileOptions options)
+        public static FileStream Open(string path, FileMode mode, FileStreamOptions options)
-        public static FileStream OpenRead(string path, FileOptions options)
+        public static FileStream OpenRead(string path, FileStreamOptions options)
-        public static StreamReader OpenText(string path, FileOptions options)
+        public static StreamReader OpenText(string path, FileStreamOptions options)
-        public static FileStream OpenWrite(string path, FileOptions options)
+        public static FileStream OpenWrite(string path, FileStreamOptions options)
    }
    public sealed partial class FileInfo : FileSystemInfo
    {
-        public StreamWriter AppendText(FileOptions options)
+        public StreamWriter AppendText(FileStreamOptions options)
-        public FileStream Create(FileOptions options)
+        public FileStream Create(FileStreamOptions options)
-        public StreamWriter CreateText(FileOptions options)
+        public StreamWriter CreateText(FileStreamOptions options)
-        public FileStream Open(FileMode mode, FileOptions options)
-        public FileStream Open(FileMode mode, FileAccess access, FileOptions options)
-        public FileStream Open(FileMode mode, FileAccess access, FileShare share, FileOptions options)
+        public FileStream Open(FileMode mode, FileStreamOptions options)
-        public FileStream OpenRead(FileOptions options)
+        public FileStream OpenRead(FileStreamOptions options)
-        public StreamReader OpenText(FileOptions options)    
+        public StreamReader OpenText(FileStreamOptions options)    
-        public FileStream OpenWrite(FileOptions options)
+        public FileStream OpenWrite(FileStreamOptions options)
    }
    public partial class StreamReader : TextReader
    {
-        public StreamReader(string path, FileOptions options)
+        public StreamReader(string path, FileStreamOptions options)
-        public StreamReader(string path, bool detectEncodingFromByteOrderMarks, FileOptions options)
-        public StreamReader(string path, System.Text.Encoding encoding, FileOptions options)
-        public StreamReader(string path, System.Text.Encoding encoding, bool detectEncodingFromByteOrderMarks, FileOptions options)
-        public StreamReader(string path, System.Text.Encoding encoding, bool detectEncodingFromByteOrderMarks, int bufferSize, FileOptions options)
+        public StreamReader(string path, System.Text.Encoding encoding, bool detectEncodingFromByteOrderMarks, FileStreamOptions options)
    }
    public partial class StreamWriter : TextWriter
    {
-        public StreamWriter(string path, FileOptions options)
+        public StreamWriter(string path, FileStreamOptions options)
-        public StreamWriter(string path, bool append, FileOptions options)
-        public StreamWriter(string path, bool append, System.Text.Encoding encoding, FileOptions options)
-        public StreamWriter(string path, bool append, System.Text.Encoding encoding, int bufferSize, FileOptions options)
+        public StreamWriter(string path, System.Text.Encoding encoding, FileStreamOptions options)
    }
}

@stephentoub stephentoub added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-approved API was approved in API review, it can be implemented labels May 25, 2021
@bartonjs
Copy link
Member

bartonjs commented May 27, 2021

Video

Trusting that all of the removals are not yet implemented, or were to be new for .NET 6, the update looks good as proposed.

namespace System.IO
{
    public static partial class File
    {
-        public StreamWriter AppendText(string path, FileOptions options)
+        public StreamWriter AppendText(string path, FileStreamOptions options)
-        public static FileStream Create(string path, FileOptions options)
+        public static FileStream Create(string path, FileStreamOptions options)
-        public static StreamWriter CreateText(string path, FileOptions options)   
+        public static StreamWriter CreateText(string path, FileStreamOptions options)  
-        public static FileStream Open(string path, FileMode mode, FileOptions options)
-        public static FileStream Open(string path, FileMode mode, FileAccess access, FileOptions options)
-        public static FileStream Open(string path, FileMode mode, FileAccess access, FileShare share, FileOptions options)
+        public static FileStream Open(string path, FileMode mode, FileStreamOptions options)
-        public static FileStream OpenRead(string path, FileOptions options)
+        public static FileStream OpenRead(string path, FileStreamOptions options)
-        public static StreamReader OpenText(string path, FileOptions options)
+        public static StreamReader OpenText(string path, FileStreamOptions options)
-        public static FileStream OpenWrite(string path, FileOptions options)
+        public static FileStream OpenWrite(string path, FileStreamOptions options)
    }
    public sealed partial class FileInfo : FileSystemInfo
    {
-        public StreamWriter AppendText(FileOptions options)
+        public StreamWriter AppendText(FileStreamOptions options)
-        public FileStream Create(FileOptions options)
+        public FileStream Create(FileStreamOptions options)
-        public StreamWriter CreateText(FileOptions options)
+        public StreamWriter CreateText(FileStreamOptions options)
-        public FileStream Open(FileMode mode, FileOptions options)
-        public FileStream Open(FileMode mode, FileAccess access, FileOptions options)
-        public FileStream Open(FileMode mode, FileAccess access, FileShare share, FileOptions options)
+        public FileStream Open(FileMode mode, FileStreamOptions options)
-        public FileStream OpenRead(FileOptions options)
+        public FileStream OpenRead(FileStreamOptions options)
-        public StreamReader OpenText(FileOptions options)    
+        public StreamReader OpenText(FileStreamOptions options)    
-        public FileStream OpenWrite(FileOptions options)
+        public FileStream OpenWrite(FileStreamOptions options)
    }
    public partial class StreamReader : TextReader
    {
-        public StreamReader(string path, FileOptions options)
+        public StreamReader(string path, FileStreamOptions options)
-        public StreamReader(string path, bool detectEncodingFromByteOrderMarks, FileOptions options)
-        public StreamReader(string path, System.Text.Encoding encoding, FileOptions options)
-        public StreamReader(string path, System.Text.Encoding encoding, bool detectEncodingFromByteOrderMarks, FileOptions options)
-        public StreamReader(string path, System.Text.Encoding encoding, bool detectEncodingFromByteOrderMarks, int bufferSize, FileOptions options)
+        public StreamReader(string path, System.Text.Encoding encoding, bool detectEncodingFromByteOrderMarks, FileStreamOptions options)
    }
    public partial class StreamWriter : TextWriter
    {
-        public StreamWriter(string path, FileOptions options)
+        public StreamWriter(string path, FileStreamOptions options)
-        public StreamWriter(string path, bool append, FileOptions options)
-        public StreamWriter(string path, bool append, System.Text.Encoding encoding, FileOptions options)
-        public StreamWriter(string path, bool append, System.Text.Encoding encoding, int bufferSize, FileOptions options)
+        public StreamWriter(string path, System.Text.Encoding encoding, FileStreamOptions options)
    }
}

@bartonjs bartonjs added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels May 27, 2021
@adamsitnik
Copy link
Member

When reviewing #52720 I've realized that I've made a mistake in the re-design proposal (#24698 (comment)) that got approved.

There is no need for FileMode mode in Open that accepts FileStreamOptions which allows for setting FileMode:

- public static FileStream Open(string path, FileMode mode, FileStreamOptions options)
+ public static FileStream Open(string path, FileStreamOptions options)

@bartonjs can I get this change accepted without presenting it in the review meeting?

@bartonjs
Copy link
Member

bartonjs commented Jun 1, 2021

Makes sense to me, but you probably want FileInfo.Open, too.

As the video freely admits, we didn't really look at this issue very much. It was "we approved some new overloads before, this is simplifying them for the new options type, LGTM". As long as you stay in that same principle (not making a breaking change, aren't adding net new things), you're still within the bounds we reviewed 😄.

@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jun 10, 2021
@adamsitnik adamsitnik assigned manandre and unassigned adamsitnik Jun 10, 2021
@MgSam
Copy link

MgSam commented Jul 8, 2021

Maybe I'm missing something but why not add Async overloads for all these operations as well? For example, File.AppendTextAsync()?

I'd never even noticed the isAsync argument until reading this thread. The vast majority of developers are used to seeing these synchronous overloads next to newer asynchronous overloads. I think you want people to fall into the pit of success.

@ghost ghost locked as resolved and limited conversation to collaborators Aug 7, 2021
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
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants