FileStreamStrategies refactor

[adamsitnik]

@jozkee @carlossanlop the promised refactor ;)

I've tried to refactor the handling of exit codes for ReadAsync and WriteAsync but I found that they differ too much to make it worth it.

ReadAsync has some special handling of EOF:

runtime/src/libraries/System.Private.CoreLib/src/System/IO/AsyncWindowsFileStreamStrategy.cs

Lines 198 to 206 in ac5c33d

	{
	// Not an error, but EOF. AsyncFSCallback will NOT be
	// called. Call the user callback here.
	// We clear the overlapped status bit for this special case.
	// Failure to do so looks like we are freeing a pending overlapped later.
	intOverlapped->InternalLow = IntPtr.Zero;
	completionSource.SetCompletedSynchronously(0);
	}

while WriteAsync does it a little bit differently:

runtime/src/libraries/System.Private.CoreLib/src/System/IO/AsyncWindowsFileStreamStrategy.cs

Lines 312 to 318 in ac5c33d

	if (errorCode == ERROR_NO_DATA)
	{
	// Not an error, but EOF. AsyncFSCallback will NOT be called.
	// Completing TCS and return cached task allowing the GC to collect TCS.
	completionSource.SetCompletedSynchronously(0);
	return Task.CompletedTask;
	}

Moreover, ReadAsync returns a Task<int> while WriteAsync returns just a Task (so it can use Task.CompletedTask)

@jozkee @carlossanlop feel free to merge it as soon as you accept it and the CI passes

[stephentoub]

FWIW, I would prefer if all the "strategies" were just nested types within FileStream. They're not usable nor meant to be used from outside of it, they needn't be "internal" and visible to the rest of the assembly, etc.

[carlossanlop]

If nested, I assume we can still define them in their own files (inside a partial FileStream), but is it still possible to have the strategy files inside their own folder?

[adamsitnik]

IMO they are too big to be nested. The formatting would look just bad (it's a personal opinion)

[jozkee]

I guess it would look like this:

namespace System.IO
{
    public partial class FileStream : Stream
    {
        private sealed class BufferedFileStreamStrategy : FileStreamStrategy
        {
            ...
        }
    }
}

I'm on the fence on this subject, I think that is nice that we could prevent visibility of the strategies but if it is too cumbersome for the code I don't think that is worth it IMO.

[stephentoub]

IMO they are too big to be nested. The formatting would look just bad

I disagree. The only thing it adds is one extra level of indentation. And in exchange, you get:
a) Shorter type names everywhere they're mentioned because you no longer need to include "FileStream" in all the type names, e.g. it's just "Strategy" and "BufferedStrategy" rather than "FileStreamStrategy" and "BufferedFileStreamStrategy". That simplification alone makes it more appealing to me.
b) Everywhere else in corelib, you don't see System.IO.Strategies in IntelliSense / completion lists, and no one else is able to even try to create one of these things because they're invisible.

[carlossanlop]

LGTM, only a couple of non-blocking comment requests. The only pending thing is to consider addressing Stephen's suggestion of having the strategies nested inside FileStream.

[carlossanlop]

If nested, I assume we can still define them in their own files (inside a partial FileStream), but is it still possible to have the strategy files inside their own folder?

[carlossanlop]

Maybe System.IO.StreamStrategies? I know we are affecting FileStream and BufferedStream, but we don't know if we will add more unrelated strategies in the future, or maybe we will add more strategies for other Stream derived types.

This suggestion depends on whether you want to address Stephen's suggestion to have the strategies nested inside FileStream, in which case, I think we won't need this subnamespace anymore. They would live inside System.IO.

[carlossanlop]

While reviewing @jozkee's PR, I noticed the FileStreamHelpers.Windows.cs and FileStreamHelpers.Unix.cs files both have a setter method and a getter method for length, but their names don't match. If you agree, could we address that in this PR, since it's for refactoring?:

• FileStreamHelpers.GetFileLength
• FileStreamHelpers.SetLength => FileStreamHelpers.SetFileLength

[jozkee]

What is this if to if-else change achieving OR is there a guideline suggesting this? I failed to find it.

[adamsitnik]

It's an old habit that comes from https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1513.md

[adamsitnik]

the failures are not related,