diff --git a/src/Workspaces/Core/Desktop/Workspace/Host/TemporaryStorage/TemporaryStorageServiceFactory.CopiedMemoryMappedViewHandle.cs b/src/Workspaces/Core/Desktop/Workspace/Host/TemporaryStorage/TemporaryStorageServiceFactory.CopiedMemoryMappedViewHandle.cs new file mode 100644 index 0000000000000000000000000000000000000000..1a58af65643a36fec81765de5e39898b35af2978 --- /dev/null +++ b/src/Workspaces/Core/Desktop/Workspace/Host/TemporaryStorage/TemporaryStorageServiceFactory.CopiedMemoryMappedViewHandle.cs @@ -0,0 +1,57 @@ +// Copyright (c) Microsoft. All Rights Reserved. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. + +using System; +using System.Runtime.CompilerServices; +using System.Runtime.InteropServices; +using Microsoft.Win32.SafeHandles; + +namespace Microsoft.CodeAnalysis.Host +{ + internal partial class TemporaryStorageServiceFactory + { + /// + /// This critical handle increments the reference count of a . The view + /// will be released when it is disposed and all pointers acquired through + /// (which this class uses) are released. + /// + /// + /// types are not reference counted, and are thus somewhat limited in their + /// usefulness. However, this handle class has tightly restricted accessibility and is only used by managed + /// code which does not rely on it counting references. + /// + private unsafe sealed class CopiedMemoryMappedViewHandle : CriticalHandleZeroOrMinusOneIsInvalid + { + private readonly SafeMemoryMappedViewHandle _viewHandle; + + public CopiedMemoryMappedViewHandle(SafeMemoryMappedViewHandle viewHandle) + { + _viewHandle = viewHandle; + + byte* pointer = null; + + // The following code uses a constrained execution region (CER) to ensure that the code ends up in one + // of the following states, even in the presence of asynchronous exceptions like ThreadAbortException: + // + // 1. The pointer is not acquired, and the current handle is invalid (thus will not be released) + // 2. The pointer is acquired, and the current handle is fully initialized for later cleanup + RuntimeHelpers.PrepareConstrainedRegions(); + try + { + viewHandle.AcquirePointer(ref pointer); + } + finally + { + SetHandle((IntPtr)pointer); + } + } + + public byte* Pointer => (byte*)handle; + + protected override bool ReleaseHandle() + { + _viewHandle.ReleasePointer(); + return true; + } + } + } +} diff --git a/src/Workspaces/Core/Desktop/Workspace/Host/TemporaryStorage/TemporaryStorageServiceFactory.MemoryMappedInfo.cs b/src/Workspaces/Core/Desktop/Workspace/Host/TemporaryStorage/TemporaryStorageServiceFactory.MemoryMappedInfo.cs index 7cacb0d4d2eebbcd70fafb77f441337e4bac6177..31ca84f651b6adde04e56f650ed2179b13b1bfa0 100644 --- a/src/Workspaces/Core/Desktop/Workspace/Host/TemporaryStorage/TemporaryStorageServiceFactory.MemoryMappedInfo.cs +++ b/src/Workspaces/Core/Desktop/Workspace/Host/TemporaryStorage/TemporaryStorageServiceFactory.MemoryMappedInfo.cs @@ -22,16 +22,27 @@ internal sealed class MemoryMappedInfo : IDisposable { private readonly string _name; private readonly long _size; - private readonly MemoryMappedFile _memoryMappedFile; /// - /// ref count of stream given out + /// The memory mapped file. /// - private int _streamCount; + /// + /// It is possible for this accessor to be disposed prior to the view and/or the streams which use it. + /// However, the operating system does not actually close the views which are in use until the view handles + /// are closed as well, even if the is disposed first. + /// + private readonly MemoryMappedFile _memoryMappedFile; /// /// actual memory accessor that owns the VM /// + /// + /// It is possible for this accessor to be disposed prior to the streams which use it. However, the + /// streams interact directly with the underlying memory buffer, and keep a + /// to prevent that buffer from being released while still in + /// use. The used by this accessor is reference counted, and is not finally + /// released until the reference count reaches zero. + /// private MemoryMappedViewAccessor _accessor; public MemoryMappedInfo(long size) @@ -40,9 +51,6 @@ public MemoryMappedInfo(long size) _size = size; _memoryMappedFile = MemoryMappedFile.CreateNew(_name, size); - - _streamCount = 0; - _accessor = null; } public MemoryMappedInfo(string name, long size) @@ -51,9 +59,6 @@ public MemoryMappedInfo(string name, long size) _size = size; _memoryMappedFile = MemoryMappedFile.OpenExisting(_name); - - _streamCount = 0; - _accessor = null; } /// @@ -81,7 +86,7 @@ public Stream CreateReadableStream() // CreateViewStream is not guaranteed to be thread-safe lock (_memoryMappedFile) { - if (_streamCount == 0) + if (_accessor == null) { try { @@ -97,8 +102,8 @@ public Stream CreateReadableStream() } } - _streamCount++; - return new SharedReadableStream(this, _accessor, _size); + Contract.Assert(_accessor.CanRead); + return new SharedReadableStream(this, new CopiedMemoryMappedViewHandle(_accessor.SafeMemoryMappedViewHandle), _accessor.PointerOffset, _size); } } @@ -115,24 +120,6 @@ public Stream CreateWritableStream() } } - private void StreamDisposed() - { - lock (_memoryMappedFile) - { - _streamCount--; - if (_streamCount == 0 && _accessor != null) - { - _accessor.Dispose(); - _accessor = null; - } - } - } - - ~MemoryMappedInfo() - { - Dispose(false); - } - public void Dispose() { Dispose(true); @@ -141,25 +128,21 @@ public void Dispose() private void Dispose(bool disposing) { - lock (_memoryMappedFile) + if (disposing) { - if (_accessor != null) + lock (_memoryMappedFile) { - // dispose accessor it owns. - // if someone explicitly called Dispose when streams given out are not - // disposed yet, the accessor each stream has will simply stop working. - // - // it is caller's responsibility to make sure all streams it got from - // the temporary storage are disposed before calling dispose on the storage. - // - // otherwise, finalizer will take care of disposing stuff as we used to be. - _accessor.Dispose(); - _accessor = null; + if (_accessor != null) + { + // (see remarks on accessor for relation between _accessor and the streams) + _accessor.Dispose(); + _accessor = null; + } } - } - // Dispose the memoryMappedFile - _memoryMappedFile.Dispose(); + // (see remarks on accessor for relation between _memoryMappedFile and the views/streams) + _memoryMappedFile.Dispose(); + } } public static string CreateUniqueName(long size) @@ -169,31 +152,19 @@ public static string CreateUniqueName(long size) private unsafe sealed class SharedReadableStream : Stream, ISupportDirectMemoryAccess { - private readonly MemoryMappedViewAccessor _accessor; + private readonly CopiedMemoryMappedViewHandle _handle; - private MemoryMappedInfo _owner; private byte* _start; private byte* _current; private readonly byte* _end; - public SharedReadableStream(MemoryMappedInfo owner, MemoryMappedViewAccessor accessor, long length) + public SharedReadableStream(MemoryMappedInfo owner, CopiedMemoryMappedViewHandle handle, long offset, long length) { - Contract.Assert(accessor.CanRead); - - _owner = owner; - _accessor = accessor; - _current = _start = AcquirePointer(accessor); + _handle = handle; + _current = _start = handle.Pointer + offset; _end = checked(_start + length); } - ~SharedReadableStream() - { - // we don't have control on stream we give out to others such as - // compiler (ImageOnlyMetadataReference), make sure we dispose resource - // at the end if Disposed is not called explicitly. - Dispose(false); - } - public override bool CanRead { get @@ -326,17 +297,12 @@ protected override void Dispose(bool disposing) { base.Dispose(disposing); - if (_start != null) + if (disposing) { - _accessor.SafeMemoryMappedViewHandle.ReleasePointer(); - _start = null; + _handle.Dispose(); } - if (_owner != null) - { - _owner.StreamDisposed(); - _owner = null; - } + _start = null; } /// @@ -346,19 +312,6 @@ public IntPtr GetPointer() { return (IntPtr)_start; } - - /// - /// Acquire the fixed pointer to the start of the memory mapped view. - /// The pointer will be released during - /// - /// The pointer to the start of the memory mapped view. The pointer is valid, and remains fixed for the lifetime of this object. - private static byte* AcquirePointer(MemoryMappedViewAccessor accessor) - { - byte* ptr = null; - accessor.SafeMemoryMappedViewHandle.AcquirePointer(ref ptr); - ptr += accessor.PointerOffset; - return ptr; - } } } } diff --git a/src/Workspaces/Core/Desktop/Workspaces.Desktop.csproj b/src/Workspaces/Core/Desktop/Workspaces.Desktop.csproj index ac315231d59e0bcc2af66c3225581ab244b8b82f..ef6502385fe8f7a2ed66cc0cf9436492ec343dc1 100644 --- a/src/Workspaces/Core/Desktop/Workspaces.Desktop.csproj +++ b/src/Workspaces/Core/Desktop/Workspaces.Desktop.csproj @@ -95,6 +95,7 @@ +