Merge pull request #2082 from stephentoub/improve_mmf_reliability

Improve reliability of System.IO.MemoryMappedFiles on Unix

Author: stephentoub

src/System.IO.MemoryMappedFiles/src/Microsoft/Win32/SafeMemoryMappedFileHandle.Unix.cs

@@ -13,14 +13,6 @@ namespace Microsoft.Win32.SafeHandles
 {
     public sealed partial class SafeMemoryMappedFileHandle : SafeHandle
     {
-        /// <summary>Indicates where the FileHandle came from, which then controls if/how it should be cleaned up.</summary>
-        internal enum FileStreamSource
-        {
-            Provided,
-            ManufacturedFile,
-            ManufacturedSharedMemory,
-        }
-
         /// <summary>Counter used to produce a unique handle value.</summary>
         private static long s_counter = 0;
 
@@ -31,15 +23,8 @@ internal enum FileStreamSource
         /// </summary>
         internal readonly FileStream _fileStream;
 
-        /// <summary>Indication as to where the file stream came from, if it exists.</summary>
-        internal readonly FileStreamSource _fileStreamSource;
-
-        /// <summary>
-        /// The name of the map, currently used to give internal names to anonymous,
-        /// memory-backed maps.  This will be null if the map is file-backed or in
-        /// some corner cases of memory-backed maps, e.g. read-only.
-        /// </summary>
-        internal readonly string _mapName;
+        /// <summary>Whether this SafeHandle owns the _fileStream and should Dispose it when disposed.</summary>
+        internal readonly bool _ownsFileStream;
 
         /// <summary>The inheritability of the memory-mapped file.</summary>
         internal readonly HandleInheritability _inheritability;
@@ -54,24 +39,23 @@ internal enum FileStreamSource
         internal readonly long _capacity;
 
         /// <summary>Initializes the memory-mapped file handle.</summary>
-        /// <param name="mapName">The name of the map; may be null.</param>
         /// <param name="fileStream">The underlying file stream; may be null.</param>
-        /// <param name="fileStreamSource">The source of the file stream.</param>
+        /// <param name="ownsFileStream">Whether this SafeHandle is responsible for Disposing the fileStream.</param>
         /// <param name="inheritability">The inheritability of the memory-mapped file.</param>
         /// <param name="access">The access for the memory-mapped file.</param>
         /// <param name="options">The options for the memory-mapped file.</param>
         /// <param name="capacity">The capacity of the memory-mapped file.</param>
         internal SafeMemoryMappedFileHandle(
-            string mapName,
-            FileStream fileStream, FileStreamSource fileStreamSource, HandleInheritability inheritability,
+            FileStream fileStream, bool ownsFileStream, HandleInheritability inheritability,
             MemoryMappedFileAccess access, MemoryMappedFileOptions options,
             long capacity)
             : base(new IntPtr(-1), ownsHandle: true)
         {
+            Debug.Assert(!ownsFileStream || fileStream != null, "We can only own a FileStream we're actually given.");
+
             // Store the arguments.  We'll actually open the map when the view is created.
-            _mapName = mapName;
             _fileStream = fileStream;
-            _fileStreamSource = fileStreamSource;
+            _ownsFileStream = ownsFileStream;
             _inheritability = inheritability;
             _access = access;
             _options = options;
@@ -84,26 +68,17 @@ internal SafeMemoryMappedFileHandle(
 
         protected override void Dispose(bool disposing)
         {
-            if (disposing && _fileStream != null && _fileStreamSource != FileStreamSource.Provided)
+            if (disposing && _ownsFileStream)
             {
-                // Clean up the file if we created it
+                // Clean up the file descriptor (either for a file on disk or a shared memory object) if we created it
                 _fileStream.Dispose();
             }
             base.Dispose(disposing);
         }
 
         protected override unsafe bool ReleaseHandle()
         {
-            if (_fileStreamSource == FileStreamSource.ManufacturedSharedMemory)
-            {
-                Debug.Assert(_mapName != null);
-                Debug.Assert(_fileStream != null);
-                return Interop.libc.shm_unlink(_mapName) == 0;
-            }
-
-            // For _fileHandleSource == File, there's nothing to clean up, as it's either the caller's responsibility
-            // or it was created as DeleteOnClose (if it was a temporary backing store).
-
+            // Nothing to clean up.  We unlinked immediately after creating the backing store.
             return true;
         }
 

src/System.IO.MemoryMappedFiles/src/System.IO.MemoryMappedFiles.csproj

@@ -122,9 +122,6 @@
     <Compile Include="$(CommonPath)\Interop\Unix\Interop.IOErrors.cs">
       <Link>Common\Interop\Unix\Interop.IOErrors.cs</Link>
     </Compile>
-    <Compile Include="$(CommonPath)\Interop\Unix\libc\Interop.ftruncate.cs">
-      <Link>Common\Interop\Unix\Interop.ftruncate.cs</Link>
-    </Compile>
     <Compile Include="$(CommonPath)\Interop\Unix\libc\Interop.gnu_get_libc_version.cs">
       <Link>Common\Interop\Unix\Interop.gnu_get_libc_version.cs</Link>
     </Compile>
@@ -156,8 +153,11 @@
   </ItemGroup>
   <!-- Linux -->
   <ItemGroup Condition="'$(TargetsLinux)' == 'true'">
-    <Compile Include="System\IO\MemoryMappedFiles\MemoryMappedFile.Linux.cs" />
-    <Compile Include="System\IO\MemoryMappedFiles\MemoryMappedView.Linux.cs" />
+    <Compile Include="System\IO\MemoryMappedFiles\MemoryMappedFile.Unix.BackingObject_Memory.cs" />
+    <Compile Include="System\IO\MemoryMappedFiles\MemoryMappedView.Unix.MADV_DONTFORK.cs" />
+    <Compile Include="$(CommonPath)\Interop\Unix\libc\Interop.ftruncate.cs">
+      <Link>Common\Interop\Unix\Interop.ftruncate.cs</Link>
+    </Compile>
     <Compile Include="$(CommonPath)\Interop\Linux\Interop.Errors.cs">
       <Link>Common\Interop\Linux\Interop.Errors.cs</Link>
     </Compile>
@@ -185,7 +185,10 @@
   </ItemGroup>
   <!-- OSX -->
   <ItemGroup Condition="'$(TargetsOSX)' == 'true'">
-    <Compile Include="System\IO\MemoryMappedFiles\MemoryMappedFile.OSX.cs" />
+    <Compile Include="System\IO\MemoryMappedFiles\MemoryMappedFile.Unix.BackingObject_File.cs" />
+    <Compile Include="$(CommonPath)\Interop\Unix\libc\Interop.unlink.cs">
+      <Link>Common\Interop\Unix\Interop.unlink.cs</Link>
+    </Compile>
     <Compile Include="$(CommonPath)\Interop\OSX\Interop.Errors.cs">
       <Link>Common\Interop\OSX\Interop.Errors.cs</Link>
     </Compile>
@@ -204,9 +207,6 @@
     <Compile Include="$(CommonPath)\Interop\OSX\libc\Interop.SysConfNames.cs">
       <Link>Common\Interop\OSX\Interop.SysConfNames.cs</Link>
     </Compile>
-    <Compile Include="$(CommonPath)\Interop\OSX\libsystem_kernel\Interop.shm_open.cs">
-      <Link>Common\Interop\OSX\Interop.shm_open.cs</Link>
-    </Compile>
   </ItemGroup>
   <!-- Resource files -->
   <ItemGroup>

src/System.IO.MemoryMappedFiles/src/System/IO/MemoryMappedFiles/MemoryMappedFile.OSX.cs

@@ -0,0 +1,34 @@
+// Copyright (c) Microsoft. All rights reserved.
+// Licensed under the MIT license. See LICENSE file in the project root for full license information.
+
+using Microsoft.Win32.SafeHandles;
+
+namespace System.IO.MemoryMappedFiles
+{
+    public partial class MemoryMappedFile
+    {
+        private static FileStream CreateSharedBackingObject(Interop.libc.MemoryMappedProtections protections, long capacity)
+        {
+            string path = TmpPathPrefix + Guid.NewGuid().ToString("N") + ".tmp";
+
+            FileAccess access =
+                (protections & (Interop.libc.MemoryMappedProtections.PROT_READ | Interop.libc.MemoryMappedProtections.PROT_WRITE)) != 0 ? FileAccess.ReadWrite :
+                (protections & (Interop.libc.MemoryMappedProtections.PROT_WRITE)) != 0 ? FileAccess.Write :
+                FileAccess.Read;
+
+            // Create the backing file, then immediately unlink it so that it'll be cleaned up when no longer in use.
+            // Then enlarge it to the requested capacity.
+            const int DefaultBufferSize = 0x1000;
+            var fs = new FileStream(path, FileMode.CreateNew, TranslateProtectionsToFileAccess(protections), FileShare.ReadWrite, DefaultBufferSize);
+            Interop.CheckIo(Interop.libc.unlink(path));
+            fs.SetLength(capacity);
+            return fs;
+        }
+
+        // -----------------------------
+        // ---- PAL layer ends here ----
+        // -----------------------------
+
+        private static readonly string TmpPathPrefix = Path.Combine(Path.GetTempPath(), MemoryMapObjectFilePrefix);
+    }
+}

src/System.IO.MemoryMappedFiles/src/System/IO/MemoryMappedFiles/MemoryMappedFile.Unix.BackingObject_File.cs

@@ -8,12 +8,10 @@ namespace System.IO.MemoryMappedFiles
     public partial class MemoryMappedFile
     {
         private static FileStream CreateSharedBackingObject(
-            Interop.libc.MemoryMappedProtections protections, long capacity,
-            out string mapName, out SafeMemoryMappedFileHandle.FileStreamSource fileStreamSource)
+            Interop.libc.MemoryMappedProtections protections, long capacity)
         {
             // The POSIX shared memory object name must begin with '/'.  After that we just want something short and unique.
-            mapName = "/" + MemoryMapObjectFilePrefix + Guid.NewGuid().ToString("N");
-            fileStreamSource = SafeMemoryMappedFileHandle.FileStreamSource.ManufacturedSharedMemory;
+            string mapName = "/" + MemoryMapObjectFilePrefix + Guid.NewGuid().ToString("N");
 
             // Determine the flags to use when creating the shared memory object
             Interop.libc.OpenFlags flags = (protections & Interop.libc.MemoryMappedProtections.PROT_WRITE) != 0 ?
@@ -30,15 +28,24 @@ private static FileStream CreateSharedBackingObject(
             if ((protections & Interop.libc.MemoryMappedProtections.PROT_EXEC) != 0)
                 perms |= Interop.libc.Permissions.S_IXUSR;
 
-            // Create the shared memory object. Then enlarge it to the requested capacity.
+            // Create the shared memory object.
             int fd;
             Interop.CheckIo(fd = Interop.libc.shm_open(mapName, flags, (int)perms), mapName);
             SafeFileHandle fileHandle = new SafeFileHandle((IntPtr)fd, ownsHandle: true);
 
-            // Wrap the handle in a stream and return it.
-            var fs = new FileStream(fileHandle, TranslateProtectionsToFileAccess(protections));
-            fs.SetLength(capacity);
-            return fs;
+            // Unlink the shared memory object immediatley so that it'll go away once all handles 
+            // to it are closed (as with opened then unlinked files, it'll remain usable via
+            // the open handles even though it's unlinked and can't be opened anew via its name).
+            Interop.CheckIo(Interop.libc.shm_unlink(mapName));
+
+            // Give it the right capacity.  We do this directly with ftruncate rather
+            // than via FileStream.SetLength after the FileStream is created because, on some systems,
+            // lseek fails on shared memory objects, causing the FileStream to think it's unseekable,
+            // causing it to preemptively throw from SetLength.
+            Interop.CheckIo(Interop.libc.ftruncate(fd, capacity));
+
+            // Wrap the file descriptor in a stream and return it.
+            return new FileStream(fileHandle, TranslateProtectionsToFileAccess(protections));
         }
     }
 }

src/System.IO.MemoryMappedFiles/src/System/IO/MemoryMappedFiles/MemoryMappedFile.Linux.cs renamed to src/System.IO.MemoryMappedFiles/src/System/IO/MemoryMappedFiles/MemoryMappedFile.Unix.BackingObject_Memory.cs

@@ -21,15 +21,19 @@ private static unsafe SafeMemoryMappedFileHandle CreateCore(
         {
             if (mapName != null)
             {
-                // TODO: We currently do not support named maps.  We could possibly support 
-                // named maps in the future by using shm_open / shm_unlink, as we do for
-                // giving internal names to anonymous maps.  Issues to work through will include 
-                // dealing with permissions, passing information from the creator of the 
-                // map to another opener of it, etc.
+                // Named maps are not supported in our Unix implementation.  We could support named maps on Linux using 
+                // shared memory segments (shmget/shmat/shmdt/shmctl/etc.), but that doesn't work on OSX by default due
+                // to very low default limits on OSX for the size of such objects; it also doesn't support behaviors
+                // like copy-on-write or the ability to control handle inheritability, and reliably cleaning them up
+                // relies on some non-conforming behaviors around shared memory IDs remaining valid even after they've
+                // been marked for deletion (IPC_RMID).  We could also support named maps using the current implementation
+                // by not unlinking after creating the backing store, but then the backing stores would remain around
+                // and accessible even after process exit, with no good way to appropriately clean them up.
+                // (File-backed maps may still be used for cross-process communication.)
                 throw CreateNamedMapsNotSupportedException();
             }
 
-            var fileStreamSource = SafeMemoryMappedFileHandle.FileStreamSource.Provided;
+            bool ownsFileStream = false;
             if (fileStream != null)
             {
                 // This map is backed by a file.  Make sure the file's size is increased to be
@@ -50,15 +54,16 @@ private static unsafe SafeMemoryMappedFileHandle CreateCore(
                 // of an object that has read-only permissions, but we also don't need to worry about sharing
                 // views over a read-only, anonymous, memory-backed map, because the data will never change, so all views
                 // will always see zero and can't change that.  In that case, we just use the built-in anonymous support of
-                // the map by leaving fileHandle as null.
+                // the map by leaving fileStream as null.
                 Interop.libc.MemoryMappedProtections protections = MemoryMappedView.GetProtections(access, forVerification: false);
                 if ((protections & Interop.libc.MemoryMappedProtections.PROT_WRITE) != 0 && capacity > 0)
                 {
-                    fileStream = CreateSharedBackingObject(protections, capacity, out mapName, out fileStreamSource);
+                    ownsFileStream = true;
+                    fileStream = CreateSharedBackingObject(protections, capacity);
                 }
             }
 
-            return new SafeMemoryMappedFileHandle(mapName, fileStream, fileStreamSource, inheritability, access, options, capacity);
+            return new SafeMemoryMappedFileHandle(fileStream, ownsFileStream, inheritability, access, options, capacity);
         }
 
         /// <summary>
@@ -109,7 +114,7 @@ private static Exception CreateNamedMapsNotSupportedException()
             return new PlatformNotSupportedException(SR.PlatformNotSupported_NamedMaps);
         }
 
-        private const string MemoryMapObjectFilePrefix = "CoreFX_MMF_";
+        private const string MemoryMapObjectFilePrefix = "corefx_mmf_";
 
         private static FileAccess TranslateProtectionsToFileAccess(Interop.libc.MemoryMappedProtections protections)
         {