Updates based on code review feedback

src/Workspaces/Core/Desktop/Workspace/Host/TemporaryStorage/TemporaryStorageServiceFactory.MemoryMappedInfo.cs

@@ -33,23 +33,6 @@ internal partial class TemporaryStorageServiceFactory
         /// </remarks>
         internal sealed class MemoryMappedInfo : IDisposable
         {
-            /// <summary>
-            /// The name of the memory mapped file.
-            /// </summary>
-            private readonly string _name;
-
-            /// <summary>
-            /// The offset into the memory mapped file of the region described by the current
-            /// <see cref="MemoryMappedInfo"/>.
-            /// </summary>
-            private readonly long _offset;
-
-            /// <summary>
-            /// The size of the region of the memory mapped file described by the current
-            /// <see cref="MemoryMappedInfo"/>.
-            /// </summary>
-            private readonly long _size;
-
             /// <summary>
             /// The memory mapped file.
             /// </summary>
@@ -61,7 +44,7 @@ internal sealed class MemoryMappedInfo : IDisposable
             private readonly ReferenceCountedDisposable<MemoryMappedFile> _memoryMappedFile;
 
             /// <summary>
-            /// actual memory accessor that owns the VM
+            /// A weak reference to the actual memory accessor that owns the VM
             /// </summary>
             /// <remarks>
             /// <para>This holds a weak counted reference to current <see cref="MemoryMappedViewAccessor"/>, which
@@ -71,31 +54,37 @@ internal sealed class MemoryMappedInfo : IDisposable
             /// unmapped, making the process address space it previously claimed available for other purposes. If/when
             /// it is needed again, a new view is created.</para>
             /// </remarks>
-            private ReferenceCountedDisposable<MemoryMappedViewAccessor>.WeakReference _accessor;
+            private ReferenceCountedDisposable<MemoryMappedViewAccessor>.WeakReference _weakAccessor;
 
             public MemoryMappedInfo(ReferenceCountedDisposable<MemoryMappedFile> memoryMappedFile, string name, long offset, long size)
             {
                 _memoryMappedFile = memoryMappedFile;
-                _name = name;
-                _offset = offset;
-                _size = size;
+                Name = name;
+                Offset = offset;
+                Size = size;
             }
 
             public MemoryMappedInfo(string name, long offset, long size)
+                : this(new ReferenceCountedDisposable<MemoryMappedFile>(MemoryMappedFile.OpenExisting(name)), name, offset, size)
             {
-                _name = name;
-                _offset = offset;
-                _size = size;
-
-                _memoryMappedFile = new ReferenceCountedDisposable<MemoryMappedFile>(MemoryMappedFile.OpenExisting(_name));
             }
 
             /// <summary>
-            /// Name and Size of memory map file
+            /// The name of the memory mapped file.
+            /// </summary>
+            public string Name { get; }
+
+            /// <summary>
+            /// The offset into the memory mapped file of the region described by the current
+            /// <see cref="MemoryMappedInfo"/>.
+            /// </summary>
+            public long Offset { get; }
+
+            /// <summary>
+            /// The size of the region of the memory mapped file described by the current
+            /// <see cref="MemoryMappedInfo"/>.
             /// </summary>
-            public string Name => _name;
-            public long Offset => _offset;
-            public long Size => _size;
+            public long Size { get; }
 
             private static void ForceCompactingGC()
             {
@@ -116,18 +105,19 @@ public Stream CreateReadableStream()
                 // CreateViewAccessor is not guaranteed to be thread-safe
                 lock (_memoryMappedFile)
                 {
-                    // Note: TryAddReference will not return a non-null but invalid reference, even if the current
-                    // object has been disposed (see comments on _memoryMappedFile and TryAddReference).
-                    var streamAccessor = _accessor.TryAddReference();
+                    // Note: TryAddReference behaves according to its documentation even if the target object has been
+                    // disposed. If it returns non-null, then the object will not be disposed before the returned
+                    // reference is disposed (see comments on _memoryMappedFile and TryAddReference).
+                    var streamAccessor = _weakAccessor.TryAddReference();
                     if (streamAccessor == null)
                     {
-                        var rawAccessor = RunWithCompactingGCFallback(info => info._memoryMappedFile.Target.CreateViewAccessor(info._offset, info._size, MemoryMappedFileAccess.Read), this);
+                        var rawAccessor = RunWithCompactingGCFallback(info => info._memoryMappedFile.Target.CreateViewAccessor(info.Offset, info.Size, MemoryMappedFileAccess.Read), this);
                         streamAccessor = new ReferenceCountedDisposable<MemoryMappedViewAccessor>(rawAccessor);
-                        _accessor = new ReferenceCountedDisposable<MemoryMappedViewAccessor>.WeakReference(streamAccessor);
+                        _weakAccessor = new ReferenceCountedDisposable<MemoryMappedViewAccessor>.WeakReference(streamAccessor);
                     }
 
                     Contract.Assert(streamAccessor.Target.CanRead);
-                    return new SharedReadableStream(this, streamAccessor, _size);
+                    return new SharedReadableStream(this, streamAccessor, Size);
                 }
             }
 
@@ -140,7 +130,7 @@ public Stream CreateWritableStream()
                 // CreateViewStream is not guaranteed to be thread-safe
                 lock (_memoryMappedFile)
                 {
-                    return RunWithCompactingGCFallback(info => info._memoryMappedFile.Target.CreateViewStream(info._offset, info._size, MemoryMappedFileAccess.Write), this);
+                    return RunWithCompactingGCFallback(info => info._memoryMappedFile.Target.CreateViewStream(info.Offset, info.Size, MemoryMappedFileAccess.Write), this);
                 }
             }
 
@@ -185,7 +175,7 @@ private void Dispose(bool disposing)
                 if (disposing)
                 {
                     // See remarks on field for relation between _memoryMappedFile and the views/streams. There is no
-                    // need to write _accessor here since the types involved adhere to their contracts even in
+                    // need to write _weakAccessor here since the types involved adhere to their contracts even in
                     // concurrent code.
                     _memoryMappedFile.Dispose();
                 }

src/Workspaces/Core/Desktop/Workspace/Host/TemporaryStorage/TemporaryStorageServiceFactory.ReferenceCountedDisposable.cs

@@ -52,6 +52,11 @@ internal sealed class ReferenceCountedDisposable<T> : IDisposable
             /// The boxed reference count, which is shared by all references with the same <see cref="Target"/> object.
             /// </summary>
             /// <remarks>
+            /// <para>Only use equality operators to compare this value with 0. The actual reference count is allowed to
+            /// be a negative integer in order to support a reference count full 32-bit number of reference. Ideally it
+            /// would be represented as a <see cref="uint"/>, but some <see cref="Interlocked"/> operations are not
+            /// implemented for this type.</para>
+            ///
             /// <para>This field is set to <see langword="null"/> at the point in time when this reference is disposed.
             /// This occurs prior to clearing the <see cref="_instance"/> field in order to support concurrent
             /// code.</para>
@@ -207,7 +212,7 @@ public struct WeakReference
                 /// <summary>
                 /// DO NOT DISPOSE OF THE TARGET.
                 /// </summary>
-                private readonly WeakReference<T> _instance;
+                private readonly WeakReference<T> _weakInstance;
                 private readonly StrongBox<int> _boxedReferenceCount;
 
                 public WeakReference(ReferenceCountedDisposable<T> reference)
@@ -226,7 +231,7 @@ public WeakReference(ReferenceCountedDisposable<T> reference)
                         return;
                     }
 
-                    _instance = new WeakReference<T>(instance);
+                    _weakInstance = new WeakReference<T>(instance);
                     _boxedReferenceCount = referenceCount;
                 }
 
@@ -247,8 +252,8 @@ public WeakReference(ReferenceCountedDisposable<T> reference)
                 /// already been disposed.</returns>
                 public ReferenceCountedDisposable<T> TryAddReference()
                 {
-                    var instance = _instance;
-                    if (instance == null || !_instance.TryGetTarget(out var target))
+                    var weakInstance = _weakInstance;
+                    if (weakInstance == null || !_weakInstance.TryGetTarget(out var target))
                     {
                         return null;
                     }

src/Workspaces/Core/Desktop/Workspace/Host/TemporaryStorage/TemporaryStorageServiceFactory.cs

@@ -100,7 +100,7 @@ private MemoryMappedInfo CreateTemporaryStorage(long size)
                     // mapped file is obtained in this section, it must either be disposed within the loop or returned
                     // to the caller who will own it through the MemoryMappedInfo.
                     var storage = Volatile.Read(ref _storage);
-                    var reference = storage?.FileReference.TryAddReference();
+                    var reference = storage?.WeakFileReference.TryAddReference();
                     if (reference == null)
                     {
                         var oldStorage = storage;
@@ -140,14 +140,14 @@ public static string CreateUniqueName(long size)
             /// </summary>
             private sealed class MemoryMappedFileStorage
             {
-                public readonly ReferenceCountedDisposable<MemoryMappedFile>.WeakReference FileReference;
+                public readonly ReferenceCountedDisposable<MemoryMappedFile>.WeakReference WeakFileReference;
                 public readonly string Name;
                 public readonly long Size;
                 public long Offset;
 
                 private MemoryMappedFileStorage(ReferenceCountedDisposable<MemoryMappedFile>.WeakReference memoryMappedFile, string name, long size)
                 {
-                    FileReference = memoryMappedFile;
+                    WeakFileReference = memoryMappedFile;
                     Name = name;
                     Size = size;
                 }