Skip to content

Commit

Permalink
Address PR feedback for memory mapped files PAL
Browse files Browse the repository at this point in the history
  • Loading branch information
@stephentoub
stephentoub committed Jan 17, 2015
1 parent 64e6519 commit 5d56967
Show file tree
Hide file tree
Showing 5 changed files with 50 additions and 54 deletions.
7 changes: 7 additions & 0 deletions src/System.IO.MemoryMappedFiles/src/Interop/Interop.Windows.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,7 @@ internal static partial class Interop

// From WinBase.h
internal const int SEM_FAILCRITICALERRORS = 1;
internal static readonly IntPtr INVALID_HANDLE_VALUE = new IntPtr(-1);

[StructLayout(LayoutKind.Sequential)]
internal struct SYSTEM_INFO
Expand Down Expand Up @@ -209,6 +210,12 @@ internal static extern SafeMemoryMappedFileHandle CreateFileMapping(SafeFileHand
ref SECURITY_ATTRIBUTES lpFileMappingAttributes, int flProtect,
int dwMaximumSizeHigh, int dwMaximumSizeLow, string lpName);

[DllImport(MEMORYDLL, EntryPoint = "CreateFileMappingW", CharSet = CharSet.Unicode, SetLastError = true)]
[SecurityCritical]
internal static extern SafeMemoryMappedFileHandle CreateFileMapping(IntPtr hFile,
ref SECURITY_ATTRIBUTES lpFileMappingAttributes, int flProtect,
int dwMaximumSizeHigh, int dwMaximumSizeLow, string lpName);

[DllImport(MEMORYDLL, EntryPoint = "OpenFileMappingW", CharSet = CharSet.Unicode, SetLastError = true)]
[SecurityCritical]
internal static extern SafeMemoryMappedFileHandle OpenFileMapping(
Expand Down
16 changes: 2 additions & 14 deletions src/System.IO.MemoryMappedFiles/src/System/IO/MemoryMappedFiles/MemoryMappedFile.Unix.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ namespace System.IO.MemoryMappedFiles
public partial class MemoryMappedFile
{
/// <summary>
/// Used by the 2 Create factory method groups. A -1 fileHandle specifies that the
/// Used by the 2 Create factory method groups. A null fileHandle specifies that the
/// memory mapped file should not be associated with an exsiting file on disk (ie start
/// out empty).
/// </summary>
Expand Down Expand Up @@ -46,19 +46,7 @@ private static SafeMemoryMappedFileHandle OpenCore(
}

/// <summary>
/// Used by the CreateOrOpen factory method groups. A -1 fileHandle specifies that the
/// memory mapped file should not be associated with an existing file on disk (ie start
/// out empty).
///
/// Try to open the file if it exists -- this requires a bit more work. Loop until we can
/// either create or open a memory mapped file up to a timeout. CreateFileMapping may fail
/// if the file exists and we have non-null security attributes, in which case we need to
/// use OpenFileMapping. But, there exists a race condition because the memory mapped file
/// may have closed inbetween the two calls -- hence the loop.
///
/// This uses similar retry/timeout logic as in performance counter. It increases the wait
/// time each pass through the loop and times out in approximately 1.4 minutes. If after
/// retrying, a MMF handle still hasn't been opened, throw an InvalidOperationException.
/// Used by the CreateOrOpen factory method groups.
/// </summary>
[SecurityCritical]
private static SafeMemoryMappedFileHandle CreateOrOpenCore(
Expand Down
73 changes: 38 additions & 35 deletions src/System.IO.MemoryMappedFiles/src/System/IO/MemoryMappedFiles/MemoryMappedFile.Windows.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,14 @@
using System.Diagnostics;
using System.Runtime.InteropServices;
using System.Security;
using System.Threading.Tasks;
using System.Threading;

namespace System.IO.MemoryMappedFiles
{
public partial class MemoryMappedFile
{
/// <summary>
/// Used by the 2 Create factory method groups. A -1 fileHandle specifies that the
/// Used by the 2 Create factory method groups. A null fileHandle specifies that the
/// memory mapped file should not be associated with an exsiting file on disk (ie start
/// out empty).
/// </summary>
Expand All @@ -21,21 +21,24 @@ private static SafeMemoryMappedFileHandle CreateCore(
SafeFileHandle fileHandle, String mapName, HandleInheritability inheritability,
MemoryMappedFileAccess access, MemoryMappedFileOptions options, Int64 capacity)
{
SafeMemoryMappedFileHandle handle = null;
Interop.SECURITY_ATTRIBUTES secAttrs = GetSecAttrs(inheritability);

// split the long into two ints
int capacityLow = unchecked((int)(capacity & 0x00000000FFFFFFFFL));
int capacityHigh = unchecked((int)(capacity >> 32));

handle = Interop.mincore.CreateFileMapping(fileHandle, ref secAttrs, GetPageAccess(access) | (int)options,
capacityHigh, capacityLow, mapName);
SafeMemoryMappedFileHandle handle = fileHandle != null ?
Interop.mincore.CreateFileMapping(fileHandle, ref secAttrs, GetPageAccess(access) | (int)options, capacityHigh, capacityLow, mapName) :
Interop.mincore.CreateFileMapping(Interop.INVALID_HANDLE_VALUE, ref secAttrs, GetPageAccess(access) | (int)options, capacityHigh, capacityLow, mapName);

Int32 errorCode = Marshal.GetLastWin32Error();
if (!handle.IsInvalid && errorCode == Interop.ERROR_ALREADY_EXISTS)
if (!handle.IsInvalid)
{
handle.Dispose();
throw Win32Marshal.GetExceptionForWin32Error(errorCode);
if (errorCode == Interop.ERROR_ALREADY_EXISTS)
{
handle.Dispose();
throw Win32Marshal.GetExceptionForWin32Error(errorCode);
}
}
else if (handle.IsInvalid)
{
Expand Down Expand Up @@ -68,26 +71,23 @@ private static SafeMemoryMappedFileHandle OpenCore(
}

/// <summary>
/// Used by the CreateOrOpen factory method groups. A -1 fileHandle specifies that the
/// memory mapped file should not be associated with an existing file on disk (ie start
/// out empty).
///
/// Try to open the file if it exists -- this requires a bit more work. Loop until we can
/// either create or open a memory mapped file up to a timeout. CreateFileMapping may fail
/// if the file exists and we have non-null security attributes, in which case we need to
/// use OpenFileMapping. But, there exists a race condition because the memory mapped file
/// may have closed inbetween the two calls -- hence the loop.
///
/// This uses similar retry/timeout logic as in performance counter. It increases the wait
/// time each pass through the loop and times out in approximately 1.4 minutes. If after
/// retrying, a MMF handle still hasn't been opened, throw an InvalidOperationException.
/// Used by the CreateOrOpen factory method groups.
/// </summary>
[SecurityCritical]
private static SafeMemoryMappedFileHandle CreateOrOpenCore(SafeFileHandle fileHandle, String mapName,
HandleInheritability inheritability,
MemoryMappedFileAccess access, MemoryMappedFileOptions options,
Int64 capacity)
private static SafeMemoryMappedFileHandle CreateOrOpenCore(
String mapName, HandleInheritability inheritability, MemoryMappedFileAccess access,
MemoryMappedFileOptions options, Int64 capacity)
{
/// Try to open the file if it exists -- this requires a bit more work. Loop until we can
/// either create or open a memory mapped file up to a timeout. CreateFileMapping may fail
/// if the file exists and we have non-null security attributes, in which case we need to
/// use OpenFileMapping. But, there exists a race condition because the memory mapped file
/// may have closed inbetween the two calls -- hence the loop.
///
/// The retry/timeout logic increases the wait time each pass through the loop and times
/// out in approximately 1.4 minutes. If after retrying, a MMF handle still hasn't been opened,
/// throw an InvalidOperationException.

Debug.Assert(access != MemoryMappedFileAccess.Write, "Callers requesting write access shouldn't try to create a mmf");

SafeMemoryMappedFileHandle handle = null;
Expand All @@ -104,31 +104,25 @@ private static SafeMemoryMappedFileHandle CreateOrOpenCore(SafeFileHandle fileHa
while (waitRetries > 0)
{
// try to create
handle = Interop.mincore.CreateFileMapping(fileHandle, ref secAttrs,
handle = Interop.mincore.CreateFileMapping(Interop.INVALID_HANDLE_VALUE, ref secAttrs,
GetPageAccess(access) | (int)options, capacityHigh, capacityLow, mapName);

Int32 createErrorCode = Marshal.GetLastWin32Error();
if (!handle.IsInvalid)
{
break;
}
else
{
Int32 createErrorCode = Marshal.GetLastWin32Error();
if (createErrorCode != Interop.ERROR_ACCESS_DENIED)
{
throw Win32Marshal.GetExceptionForWin32Error(createErrorCode);
}

// the mapname exists but our ACL is preventing us from opening it with CreateFileMapping.
// Let's try to open it with OpenFileMapping.
handle.SetHandleAsInvalid();
}

// try to open
handle = Interop.mincore.OpenFileMapping(GetFileMapAccess(access), (inheritability &
HandleInheritability.Inheritable) != 0, mapName);

Int32 openErrorCode = Marshal.GetLastWin32Error();
HandleInheritability.Inheritable) != 0, mapName);

// valid handle
if (!handle.IsInvalid)
Expand All @@ -138,6 +132,7 @@ private static SafeMemoryMappedFileHandle CreateOrOpenCore(SafeFileHandle fileHa
// didn't get valid handle; have to retry
else
{
Int32 openErrorCode = Marshal.GetLastWin32Error();
if (openErrorCode != Interop.ERROR_FILE_NOT_FOUND)
{
throw Win32Marshal.GetExceptionForWin32Error(openErrorCode);
Expand All @@ -151,7 +146,7 @@ private static SafeMemoryMappedFileHandle CreateOrOpenCore(SafeFileHandle fileHa
}
else
{
Task.Delay(waitSleep).Wait();
ThreadSleep(waitSleep);
waitSleep *= 2;
}
}
Expand Down Expand Up @@ -258,5 +253,13 @@ private unsafe static Interop.SECURITY_ATTRIBUTES GetSecAttrs(HandleInheritabili
}
return secAttrs;
}

/// <summary>
/// Replacement for Thread.Sleep(milliseconds), which isn't available.
/// </summary>
internal static void ThreadSleep(int milliseconds)
{
new ManualResetEventSlim(initialState: false).Wait(milliseconds);
}
}
}
6 changes: 2 additions & 4 deletions src/System.IO.MemoryMappedFiles/src/System/IO/MemoryMappedFiles/MemoryMappedFile.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -305,9 +305,7 @@ public static MemoryMappedFile CreateNew(String mapName, Int64 capacity, MemoryM
throw new ArgumentOutOfRangeException("inheritability");
}

SafeMemoryMappedFileHandle handle = CreateCore(new SafeFileHandle(new IntPtr(-1), true), mapName, inheritability,
access, options, capacity);

SafeMemoryMappedFileHandle handle = CreateCore(null, mapName, inheritability, access, options, capacity);
return new MemoryMappedFile(handle);
}

Expand Down Expand Up @@ -376,7 +374,7 @@ public static MemoryMappedFile CreateOrOpen(String mapName, Int64 capacity,
}
else
{
handle = CreateOrOpenCore(new SafeFileHandle(new IntPtr(-1), true), mapName, inheritability, access, options, capacity);
handle = CreateOrOpenCore(mapName, inheritability, access, options, capacity);
}
return new MemoryMappedFile(handle);
}
Expand Down
2 changes: 1 addition & 1 deletion src/System.IO.MemoryMappedFiles/src/System/IO/MemoryMappedFiles/MemoryMappedView.Windows.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ public void Flush(UIntPtr capacity)
for (Int32 w = 0; canRetry && w < MaxFlushWaits; w++)
{
Int32 pause = (1 << w); // MaxFlushRetries should never be over 30
Task.Delay(pause).Wait();
MemoryMappedFile.ThreadSleep(pause);

for (Int32 r = 0; canRetry && r < MaxFlushRetriesPerWait; r++)
{
Expand Down

0 comments on commit 5d56967

Please sign in to comment.