Allow setting buffer size of FileStream outside constructor

[EatonZ]

Background and Motivation

When initializing a new FileStream, you have the option of specifying a buffer size only when initializing it (bufferSize). This is fine for the majority of cases, but I have a unique use-case that has required me to add a hacky workaround, and I would like to propose a better solution.

My use-case is a class inheriting from FileStream called DiskStream. Its constructor calls the base constructor with an opened SafeFileHandle:

public DiskStream(string PhysicalDrive, Native.DesiredAccesses Access = Native.DesiredAccesses.GENERIC_WRITE | Native.DesiredAccesses.GENERIC_READ, FileShare Share = FileShare.Read, Native.FileFlags Flags = Native.FileFlags.FILE_FLAG_NO_BUFFERING) :
base(DoCreateFile(PhysicalDrive, Access, Share, Flags), DetermineFileAccess(Access)) //DoCreateFile returns a SafeFileHandle, and DetermineFileAccess a FileAccess.
{
    ...

bufferSize could be specified as the third param when calling the base, but the problem is, at that point, I do not know what it is. It is only when my constructor runs, that my code can figure it out using the base FileStream's now-populated SafeFileHandle:

public DiskStream(string PhysicalDrive, Native.DesiredAccesses Access = Native.DesiredAccesses.GENERIC_WRITE | Native.DesiredAccesses.GENERIC_READ, FileShare Share = FileShare.Read, Native.FileFlags Flags = Native.FileFlags.FILE_FLAG_NO_BUFFERING) :
base(DoCreateFile(PhysicalDrive, Access, Share, Flags), DetermineFileAccess(Access))
{
    <PhysicalSectorSize is obtained utilizing SafeFileHandle>
    //Set the FileStream buffer size. Have to set it this way because there's currently no other way. This is important when the physical sector size is 512. The default buffer size is 4096, meaning that FileStream will read 8 sectors when the physical sector size is 512, instead of 1, which is what is intended. This will cause issues when operating at the end of disks, and may cause alignment-related IO errors in general.
    GetType().BaseType.GetField("_bufferLength", BindingFlags.Instance | BindingFlags.NonPublic).SetValue(this, (int)PhysicalSectorSize);
    ...

Without an alternative method to set the buffer size outside of the constructor, my only apparent option is reflection, which is prone to break at any time if the non-public _bufferLength field is removed or renamed.

Proposed API

In FileStream:

public void SetBufferSize(int bufferSize) => _bufferLength = bufferSize;

Usage Examples

The hacky code I posted above could be cleaned up to not use reflection:

public DiskStream(string PhysicalDrive, Native.DesiredAccesses Access = Native.DesiredAccesses.GENERIC_WRITE | Native.DesiredAccesses.GENERIC_READ, FileShare Share = FileShare.Read, Native.FileFlags Flags = Native.FileFlags.FILE_FLAG_NO_BUFFERING) :
base(DoCreateFile(PhysicalDrive, Access, Share, Flags), DetermineFileAccess(Access))
{
    <PhysicalSectorSize is obtained utilizing SafeFileHandle>
    SetBufferSize((int)PhysicalSectorSize);
    ...

Much cleaner, and no risk of breakage!

Alternative Designs

Replace FileStream's private int _bufferLength; with a public property.

Risks

The proposed API may be controversial. Changing the buffer size after a FileStream has started to be used may cause problems, but from what I gather, after reviewing FileStream's source code, setting _buffer to null after changing its size may make SetBufferSize safe to use at any point during a FileStream's life.

[KalleOlaviNiemitalo]

How about a delegating constructor in your class:

public DiskStream(string PhysicalDrive, Native.DesiredAccesses Access = Native.DesiredAccesses.GENERIC_WRITE | Native.DesiredAccesses.GENERIC_READ, FileShare Share = FileShare.Read, Native.FileFlags Flags = Native.FileFlags.FILE_FLAG_NO_BUFFERING) :
this(DoCreateFile(PhysicalDrive, Access, Share, Flags), DetermineFileAccess(Access))
{
}

private DiskStream(SafeFileHandle handle, FileAccess access) : base(handle, access, GetPhysicalSectorSize(handle))
{
}

private static int GetPhysicalSectorSize(SafeFileHandle handle)
{
    try 
    {
        <PhysicalSectorSize is obtained utilizing SafeFileHandle>
    }
    catch
    {
        // Closing the handle here is somewhat ugly
        // but the constructor that calls this method
        // is not able to do that itself.
        handle.Dispose();
        throw;
    }
}

That would avoid reflection, at least.

Related: dotnet/csharplang#377, dotnet/csharplang#2335.

[EatonZ]

Hi @KalleOlaviNiemitalo, clever idea, but that won't quite do it for me because I also use that sector size value to set a few other non-static fields in the DiskStream instance. It wouldn't be possible to set those in your example GetPhysicalSectorSize, or in second constructor (the sector size would have to be obtained again).

[KalleOlaviNiemitalo]

I think you could stash the sector size by passing it from the static method to the private constructor in a struct or tuple:

public DiskStream(
    string PhysicalDrive,
    Native.DesiredAccesses Access = Native.DesiredAccesses.GENERIC_WRITE | Native.DesiredAccesses.GENERIC_READ,
    FileShare Share = FileShare.Read,
    Native.FileFlags Flags = Native.FileFlags.FILE_FLAG_NO_BUFFERING)
: this(PreInit(PhysicalDrive, Access, Share, Flags))
{
}

private DiskStream(
    (SafeFileHandle Handle, FileAccess Access, int SectorSize) args)
: base(args.Handle, args.Access, args.SectorSize)
{
    this.sectorSize = args.SectorSize;
}

private static (SafeFileHandle Handle, FileAccess Access, int SectorSize) PreInit(
    string PhysicalDrive,
    Native.DesiredAccesses Access,
    FileShare Share,
    Native.FileFlags Flags)
{
    // TODO: exception handling
    SafeFileHandle handle = DoCreateFile(PhysicalDrive, Access, Share, Flags);
    FileAccess fileAccess = DetermineFileAccess(Access);
    int sectorSize = GetSectorSize(handle);
    return (handle, fileAccess, sectorSize);
}

Alternatively, use a variable declaration in an out argument as shown in dotnet/csharplang#2335 (comment).

[EatonZ]

@KalleOlaviNiemitalo Hm, seems like an option, but it still seems like allowing setting buffer size would be the cleaner and easier option. Looking forward to hearing if this is something the .NET team would consider, as it could have other uses outside of my use-case.

[En3Tho]

Can you just make a static method Create that will return your DiskStream? I don't think constructor should necessarily do all these complex operations, determining buffersizes and so on. Just make a method, calculate all that outside and pass simple args into constructor. Also, your example does not show how changing bufferSize dynamically does help people at all. Like "detect disk slowdown and change bufferSize accordingly to gain some advantage" etc.

[carlossanlop]

@stephentoub do you know if there could be unintended or dangerous consequences if bufferSize is changed after the FileStream creation?

[stephentoub]

There are a variety of possible problems if it could be changed at any point. For example, all async usage that might be employing a buffer would need to track whether the buffer was in use, as a change in buffer size really means a change in buffer. It also impacts various other optimizations in the implementation, such as the _buffer being stored as part of the PreAllocatedOverlapped, so that would need to be sorted out (it could be as simple as disposing the old one and creating a new one, but it'd require looking into).

I understand the use case, but it is very niche, and at present I don't think it's worth complicating the implementation to support it. There are also lots of other types in .NET that accept such a buffer size, and it's not clear to me why this particular case is special.

[EatonZ]

@En3Tho Something I will check.

@stephentoub I appreciate you taking the time to review this. If you do not want to change this, do you have any other suggestions for a less-hacky workaround that have no been mentioned here?

[stephentoub]

do you have any other suggestions for a less-hacky workaround that have no been mentioned here?

You could wrap rather than derive from FileStream? e.g.

public sealed class DiskStream : Stream
{
    private readonly FileStream _fileStream;
    ...
}

Or you could have a factory method rather than a public type? e.g.

public static FileStream CreateDiskStream(...)
{
    ... // do lots of stuff
    return new FileStream(...);
}

[EatonZ]

Thank you everyone for your thoughts and suggestions. I decided to go with @KalleOlaviNiemitalo's suggestion - using out args and a PreInit method. This works, and is good enough to alleviate my breakage concern.

I agree with @stephentoub's point that other classes use buffer sizes and implementing it only here and not in others would be an odd choice.

I am closing this as my issue is now solved, and it doesn't appear an API change is desirable at this time.