API Proposal: MemoryMappedFile constructor

[carlossanlop]

Summary

public MemoryMappedFile(SafeMemoryMappedFileHandle handle, IDisposable fileStream);

A MemoryMappedFile constructor is needed by the MemoryMappedFileAcl APIs that we are porting from .NET Framework.

Both classes are part of the same namespace, System.IO.MemoryMappedFiles, but they live in different assemblies: MemoryMappedFile lives in System.IO.MemoryMappedFiles, but MemoryMappedFileAcl lives in System.IO.FileSystem.AccessControl, which prevents the latter from consuming the existing constructor from MemoryMappedFile.

Rationale and Usage

The APIs were approved in this proposal. They are being implemented in this PR (code complete).

The three main MemoryMappedFileAcl methods are CreateFromFile, CreateNew and CreateOrOpen, which allow either creating or getting a MemoryMappedFile object using the specified ACL security. The method that needs to consume the constructor we want to make public is CreateFromFile.

Having the MemoryMappedFile and the MemoryMappedFileAcl classes in separate assemblies presents a few problems:

• Making the constructor internal won't work, because even if the classes exist within the same namespace, being in different assemblies prevents us from calling it.
• We want to avoid using InternalsVisibleToAttribute (it would expose all internals).
• We want to avoid using reflection to call the constructor (it's slow, there's difficulty making it compatible with netstandard, and there could be safety concerns).

In .NET Framework, the CreateFromFile method lives inside MemoryMappedFile, allowing to call the required constructor, which is private. The constructor receives the handle of the file, the file stream of that same file and a boolean indicating if the file stream should be disposed when the memory mapped file is created (Reference Source):

namespace System.IO.MemoryMappedFiles
{
    public class MemoryMappedFile : IDisposable
    {
        ...

        private MemoryMappedFile(SafeMemoryMappedFileHandle handle,
            FileStream fileStream, bool leaveOpen) {
            ...
            _handle = handle;
            _fileStream = fileStream;
            _leaveOpen = leaveOpen;
        }
        
        ...

        public static MemoryMappedFile CreateFromFile(FileStream fileStream, String mapName,
            Int64 capacity, MemoryMappedFileAccess access, MemoryMappedFileSecurity memoryMappedFileSecurity,
            HandleInheritability inheritability, bool leaveOpen) {
            ...
            SafeMemoryMappedFileHandle handle = CreateCore(fileStream.SafeFileHandle, mapName,
            inheritability, memoryMappedFileSecurity, access, MemoryMappedFileOptions.None, capacity);
 
            return new MemoryMappedFile(handle, fileStream, leaveOpen);
        }
        
        ...

        protected virtual void Dispose(bool disposing) {
            try {
                if (_handle != null && !_handle.IsClosed) {
                    _handle.Dispose();
                }
            }
            finally {
                if (_fileStream != null && _leaveOpen == false) {
                    _fileStream.Dispose();
                }
            }
        }

        ...
    }
}

The constructor exists in .NET Core with the same signature as in .NET Framework, but it would have to be slightly changed so it can be approved:

Proposed API

namespace System.IO.MemoryMappedFiles
{
    public class MemoryMappedFile
    {
        public MemoryMappedFile(SafeMemoryMappedFileHandle handle, IDisposable fileStream);
    }
}

Differences with the .NET Framework constructor:

• We don't need to pass a FileStream object or the Boolean. Since the FileStream object is only used to call its Dispose() method, this means we can instead pass its highest abstraction, the IDisposable.
• We still need to allow the user to indicate if they want the FileStream to be left open or not, but we need to ensure it is rooted by the MemoryMappedFile, so:
  • If the user needs the FileStream to be disposed when the MemoryMappedFile is disposed, then we can pass the FileStream itself.
  • If the user wants the FileStream to remain open after the MemoryMappedFile is disposed, we can pass a disposable wrapper for the FileStream, to ensure it's rooted.

Examples

The current proposal is already being used in the PR:

The constructor is being consumed by CreateFromFile, with either the FileStream or the rooter class being passed:
https://github.com/carlossanlop/runtime/blob/MemoryMappedFileAcl/src/libraries/System.IO.FileSystem.AccessControl/src/System/IO/MemoryMappedFiles/MemoryMappedFileAcl.cs#L42-L68

The constructor is defined here:
https://github.com/carlossanlop/runtime/blob/MemoryMappedFileAcl/src/libraries/System.IO.MemoryMappedFiles/src/System/IO/MemoryMappedFiles/MemoryMappedFile.cs#L51-L71

The unit tests are located here:
https://github.com/carlossanlop/runtime/blob/MemoryMappedFileAcl/src/libraries/System.IO.FileSystem.AccessControl/tests/MemoryMappedFileAclTests.cs

cc @JeremyKuhne @bartonjs @Jozkee

[scalablecory]

SafeMemoryMappedFileHandle already has a reference to its underlying FileStream. Can the extra argument be removed?

[danmoseley]

A method parameter typed IDisposable seems unusual. I do not see other examples in our reference assemblies.

Is there any disadavntage to just passing the FileStream and bool? It may not be as "pure" but it is perhaps more standard. We "trust" our own implementation to not do anything with the FileSTream other than dispose it (or not). This API would also be a little easier to use as there is no need for the caller to invent a "disposable wrapper" for the FileStream.

[carlossanlop]

SafeMemoryMappedFileHandle already has a reference to its underlying FileStream. Can the extra argument be removed?

The Unix version of SafeMemoryMappedFileHandle is the one that has a reference to the underlying FileStream. It's passed via a constructor:
https://source.dot.net/#System.IO.MemoryMappedFiles/Microsoft/Win32/SafeMemoryMappedFileHandle.Unix.cs,731d63069327b8e6

The Windows version does not have a reference to the FileStream or a similar constructor:
https://github.com/dotnet/runtime/blob/4f9ae42d861fcb4be2fcd5d3d55d5f227d30e723/src/libraries/System.IO.MemoryMappedFiles/src/Microsoft/Win32/SafeMemoryMappedFileHandle.Windows.cs

If we wanted to make the Windows version similar to the Unix one, we could add a similar constructor instead. Notice that the constructor has a boolean to indicate if the passed fileStream is related to the handle or not. It's a similar problem to the proposed constructor.

@JeremyKuhne what are your thoughts on this?

[carlossanlop]

A method parameter typed IDisposable seems unusual. I do not see other examples in our reference assemblies.

I understand it's unusual. The other constructor (the existing one that is currently private) follows a pattern that we already have in other places (having a leaveOpen boolean). For example: https://source.dot.net/#System.Private.CoreLib/BinaryWriter.cs,51

Is there any disadavntage to just passing the FileStream and bool? It may not be as "pure" but it is perhaps more standard.

If the user can pass a FileStream that is completely unrelated to the passed handle, and the only reason why the FileStream is needed is so it can be disposed (it's strongest form, the stream functionality, is not used), then let's just allow passing it as an IDisposable (its highest abstraction).

@bartonjs worded this suggestion better in his comment :)

[danmoseley]

Yup, just a thought - I defer to api reviewers..

[JeremyKuhne]

@JeremyKuhne what are your thoughts on this?

I'm not sure how you would do it without making it public or moving the type to FileSystem.Accesscontrol and type forwarding. The Unix one is internal. We can bring it up as an option.

Is there any disadavntage to just passing the FileStream and bool? It may not be as "pure" but it is perhaps more standard. We "trust" our own implementation to not do anything with the FileSTream other than dispose it (or not). This API would also be a little easier to use as there is no need for the caller to invent a "disposable wrapper" for the FileStream.

I prefer the FileStream, bool option for exactly those reasons.

[bartonjs]

The problem we had with it during the initial review is that the FIleStream and file handle are being provided as peers; but there's no enforcement of their relationship (and, in fact, no requirement that they be related). If it took /just/ the FileStream and the boolean (and internally re-extracted the handle), sure. It's the triplet that seems weird. The only reason for the FileStream parameter in the internal ctor is to call Dispose on it later... hence the suggestion of IDisposable. (The "take the weakest type you require" idiom)

[terrajobst]

Video

• We're fine with taking the handle parameter but we have concerns with the fileStream parameter because it seems plumbing, not part of a sensible API.
  • Option 1: Move the implementation from the higher level assembly to the assembly containing MemoryMappedFile (while the ref assembly setup)
  • Option 2.1 "Smuggle" the file stream via the SafeMemoryMappedFileHandle via a field
  • Option 2.2 "Smuggle" the file stream via the SafeMemoryMappedFileHandle unsealing & deriving from SafeMemoryMappedFileHandle
  • Option 3: do what's proposed

namespace System.IO.MemoryMappedFiles
{
    public partial class MemoryMappedFile
    {
        public MemoryMappedFile(SafeMemoryMappedFileHandle handle, IDisposable fileStream);
    }
}

[carlossanlop]

We will close the PR and come back to this if a customer requests the feature.