Simpler way to specify leaveOpen in StreamWriter constructor

[svick]

Currently, the StreamWriter constructor has the following overloads:

public StreamWriter(Stream stream)
public StreamWriter(Stream stream, Encoding encoding)
public StreamWriter(Stream stream, Encoding encoding, int bufferSize)
public StreamWriter(Stream stream, Encoding encoding, int bufferSize, bool leaveOpen)

I believe a relatively common need is to set leaveOpen to true, while keeping both encoding and bufferSize at their default values.

But the process of doing that goes something like this:

• Figure out there is no way to specify just leaveOpen.
• From the documentation of StreamWriter(Stream stream), figure out that the default encoding is UTF-8. (Though it doesn't mention it's actually UTF-8 without BOM.)
• Either make a guess about what a good buffer size is, or look at the source code of StreamWriter.
• Finally write new StreamWriter(stream, Encoding.UTF8, 1024, true).

I don't think that's acceptable, so I'm proposing to make new StreamWriter(stream, leaveOpen: true) work.

I can see few options on how to do this:

1. Assuming optional arguments are okay (they probably aren't, see dotnet/corefx#470), make use of them by changing the full overload to:

   public StreamWriter(
       Stream stream, Encoding encoding = null,
       int bufferSize = -1, bool leaveOpen = false)

   This would require relaxing encoding and bufferSize to accept null and -1, respectively, but I think that's okay.

   I think this is the most convenient option, since it allows one to easily specify any combination of options.

2. Add a new constructor overload:

   public StreamWriter(Stream stream, bool leaveOpen)

   Adding some other overloads might make sense too.

3. Don't add anything, just relax encoding and bufferSize as in option 1.

   I think this is the most conservative option, but also least convenient, since it would require writing new StreamWriter(stream, null, -1, true).

[joshfree]

@KrzysztofCwalina @ianhays

[GSPP]

There already is a StreamWriter(string, bool) overload where the bool does something entirely different than bool leaveOpen. It means append.

The constructor system for this class is a bit messed up. These mistakes were made in the 1.0 days. Option 3 seems best because it does not add more confusing overloads.

Should the "default" buffer size be specified by 0 or by -1? Not sure. Maybe we should mirror what other classes such as FileStream and MemoryStream are doing.

[svick]

@GSPP I think that bufferSize: 0 could be confused for something like "don't use any buffer". On the other hand, it would feel weird if valid values were -1 and 1..int.MaxValue (with a hole at 0).

FileStream doesn't have a special value for default, but its default buffer size is documented.

MemoryStream has a default capacity of 0 (which is also valid value for its capacity), since the buffer is resizable.

I haven't looked at other types.

[GSPP]

Right, I was concerned about the "hole" but I was not able to express that.

FileStream has a similar design error. Some constructors require you to specify a buffer size even if you don't want to do that. I always hit that issue when I want to pass FileOptions.SequentialScan. If I'm not making use of buffering I always pass in 1 which is ugly. 0 is not allowed, alas.

Could we make the buffer size arguments int?? Is that backwards compatible? This would have been the correct choice if nullable types had been available in 1.0.

Maybe we could make a new int? based overload.

We also could change the booleans to [Flags]enum StreamWriterOptions { None = 0, LeaveOpen = 1 << 0 }. That makes the code readable and resolves overload picking issues because the use of this new enum forces the new overload(s) to be used. I believe this enum approach, again, would have been the correct choice in .NET 1.0.

[svick]

Could we make the buffer size arguments int?? Is that backwards compatible?

I think so, I can't think of any situation where it would break compatibility. And it's not a bad idea, assuming this will become the accepted way for optional value type parameters in .Net Core/.Net Framework going forward.

[karelz]

We need to think it through from source compat point of view and finalize the proposal & review it. @JeremyKuhne can you please do it?

[stephentoub]

@JeremyKuhne, this is assigned to you. Are you actively working on this? I'm assuming not since it's been assigned to you for two and a half years ;)

[JeremyKuhne]

I'm assuming not

That's a fair assumption. :) Clearly this slipped under my radar.

I have hit this numerous times myself. My vote here is for the targeted fix for the main pain point:

public StreamWriter(Stream stream, bool leaveOpen);

While on one hand having a more usable/flexible constructor (or perhaps even factory methods) would be nice I think we could end up duplicating all the options in FileStream, etc. I lean towards not adding anything else here but welcome new proposals/issues to discuss usage.

[JeremyKuhne]

Video

Just realized that adding a bool would butt heads with StreamReader(Stream, bool). Need to look at this a bit more to see if we can come up with a pattern that will work well across our readers/writers.

[scottdorman]

For consistency with FileStream the DefaultBufferSize const should be public. https://github.com/dotnet/corefx/blob/c1328f49f69d501618f1d867265efacedf107f23/src/Common/src/CoreLib/System/IO/StreamWriter.cs#L25

MemoryStream doesn't have a const defined for the default capacity (which is roughly analogous to the default buffer size) since it's an int variable (which defaults to 0). For consistency though, should it?

A new const DefaultEncoding should probably be added that is equal to UTF8NoBOM.

All of the StreamReader constructors don't do anything with the leaveOpen parameter except pass it to Init. https://github.com/dotnet/corefx/blob/c1328f49f69d501618f1d867265efacedf107f23/src/Common/src/CoreLib/System/IO/StreamWriter.cs#L148 This method doesn't do anything with the parameter either except to set an internal _closable field, which is accessed through an internal LeaveOpen property. https://github.com/dotnet/corefx/blob/c1328f49f69d501618f1d867265efacedf107f23/src/Common/src/CoreLib/System/IO/StreamWriter.cs#L324-L327 Should this property become public (and settable as well) instead of internal? That would eliminate the need to change constructor signatures at all and seems to be the simplest/least intrusive change to make.

[JeremyKuhne]

For consistency with FileStream the DefaultBufferSize const should be public.

It isn't public on FileStream. I'm not seeing any places where we currently are doing something similar. I think it is worth considering, but I'd like to take that to a separate issue because it is a new thing and we'd want to make this consistent throughout the namespace (i.e. set a new pattern).

Should this property become public (and settable as well) instead of internal?

I like that option. We can do the same for StreamReader. Here is the proposal:

Summary
Constructing StreamReader and StreamWriter from a stream that you want to leave open is awkward as you're forced to specify a buffer size and an encoding, which we don't document/expose the default values. Adding a new constructor overload is difficult as we would have conflicting bool constuctors between the reader and writer. Adding a new options flag and/or options (like EnumerationOptions) is likely the way we should go if we were to introduce any other configuration options. For this immediate long-standing usability issue a simpler solution is preferable. That solution is to expose the properties we already have.

namespace System.IO
{
    public class StreamReader : TextReader
    {
        public bool LeaveOpen { get; set; }
    }

    public class StreamWriter : TextWriter
    {
        public bool LeaveOpen { get; set; }
    }
}

[scottdorman]

It isn't public on FileStream.

Yes, I missed that. It's an internal constant on FileStream. I agree that making these constants public is a good pattern but that it should be a separate issue.

Adding a new options flag and/or options

I considered suggesting an options class (like EnumerationOptions) but that felt too heavy handed and intrusive of a change for this specific request. If other configuration options get introduced, then it's definitely a better way to go than having a bunch of properties/constructor args/additional constructors.

For this request, just making the LeaveOpen properties public and setttable solves the usability issue in a simple way without a lot of code changes.

[terrajobst]

Video

We'd prefer the first option (i.e. default constructors), reason being:

1. It works for all settings, including the ones that we can't change after construction
2. We can apply it elsewhere, such as FileStream

The counter argument is that it might make it harder for folks to find bugs because we'd now accept null, but that's already the case in the framework. Hopefully, non-nullable reference types can help with that.

[JeremyKuhne]

The approved API is to add the default values and change the behavior of null encoding and -1 for buffer size.

• If null encoding use default encoding (UTF8)
• If -1 bufferSize, use default bufferSize (internal const value)
• If 0, or other negative bufferSize, throw as before

namespace System.IO
{
    public class StreamReader : TextReader
    {
        public StreamReader(Stream stream, Encoding encoding = null, bool detectEncodingFromByteOrderMarks = true, int bufferSize -1, bool leaveOpen = false);
    }

    public class StreamWriter : TextWriter
    {
        public StreamWriter(Stream stream, Encoding encoding = null, int bufferSize = -1, bool leaveOpen = false);
    }
}

[scottdorman]

@terrajobst and @JeremyKuhne, given the approved API, does that mean the decision in https://github.com/dotnet/corefx/issues/470#issuecomment-147809240 is no longer a concern?

We do not recommend using optional args. Closing this thread as there is no actions e…

[JeremyKuhne]

given the approved API, does that mean the decision in #13995 (comment) is no longer a concern?

In some cases we'll be blocked due to ambiguity with existing constructors I'm sure. Outside of that we can use optional arguments as long as we (1) put them on the constructor with the most arguments and (2) move the arguments forward if we add a longer constructor in the future.

@terrajobst, is that a good summary?

[jdemis]

@JeremyKuhne is it still up-for-grabs?

[JeremyKuhne]

@jimdemis Yes it is- want to take care of it?

[jdemis]

@JeremyKuhne yes I do. Should I make two pull requests for this one? (in corefx and coreclr)

[karelz]

@jimdemis I sent you collaborator invite - once you accept, please ping us here - we will be able to assign it to you then.
It will automatically subscribe you to all notifications in the repo (500+ per day). We recommend to switch the setting to "Not Watching", which will send notificaitons for issues you commented on, you are author of, where you are mentioned or which you explicitly subscribed to.

[jdemis]

@karelz done

[justinvp]

Should BinaryReader and BinaryWriter get similar treatment?