From aebc3e9ffb7bdd20a859608658a9c291b1383b63 Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Thu, 25 Apr 2024 16:26:29 -0700 Subject: [PATCH 1/5] Fix issue with temp storage system on unix systems --- .../TemporaryStorageService.Factory.cs | 8 +--- ...emporaryStorageService.MemoryMappedInfo.cs | 12 +++--- .../TemporaryStorageService.cs | 13 +++++- .../ITemporaryStorageService.cs | 8 +++- .../TemporaryStorageIdentifier.cs | 5 +-- ...orageService.TrivialStorageStreamHandle.cs | 26 ------------ ...StorageService.TrivialStorageTextHandle.cs | 26 ------------ .../TrivialTemporaryStorageService.cs | 42 ------------------- .../CoreTest/SolutionTests/SolutionTests.cs | 17 +------- 9 files changed, 28 insertions(+), 129 deletions(-) delete mode 100644 src/Workspaces/Core/Portable/Workspace/Host/TemporaryStorage/TrivialTemporaryStorageService.TrivialStorageStreamHandle.cs delete mode 100644 src/Workspaces/Core/Portable/Workspace/Host/TemporaryStorage/TrivialTemporaryStorageService.TrivialStorageTextHandle.cs delete mode 100644 src/Workspaces/Core/Portable/Workspace/Host/TemporaryStorage/TrivialTemporaryStorageService.cs diff --git a/src/Workspaces/Core/Portable/TemporaryStorage/TemporaryStorageService.Factory.cs b/src/Workspaces/Core/Portable/TemporaryStorage/TemporaryStorageService.Factory.cs index 0d08d75a58210..e58f2b4935d22 100644 --- a/src/Workspaces/Core/Portable/TemporaryStorage/TemporaryStorageService.Factory.cs +++ b/src/Workspaces/Core/Portable/TemporaryStorage/TemporaryStorageService.Factory.cs @@ -22,13 +22,7 @@ internal partial class Factory( public IWorkspaceService CreateService(HostWorkspaceServices workspaceServices) { var textFactory = workspaceServices.GetRequiredService(); - - // MemoryMapped files which are used by the TemporaryStorageService are present in .NET Framework (including Mono) - // and .NET Core Windows. For non-Windows .NET Core scenarios, we can return the TrivialTemporaryStorageService - // until https://github.com/dotnet/runtime/issues/30878 is fixed. - return PlatformInformation.IsWindows || PlatformInformation.IsRunningOnMono - ? new TemporaryStorageService(workspaceThreadingService, textFactory) - : TrivialTemporaryStorageService.Instance; + return new TemporaryStorageService(workspaceThreadingService, textFactory); } } } diff --git a/src/Workspaces/Core/Portable/TemporaryStorage/TemporaryStorageService.MemoryMappedInfo.cs b/src/Workspaces/Core/Portable/TemporaryStorage/TemporaryStorageService.MemoryMappedInfo.cs index 7f6b5d80dc9e2..0e0c87e785c27 100644 --- a/src/Workspaces/Core/Portable/TemporaryStorage/TemporaryStorageService.MemoryMappedInfo.cs +++ b/src/Workspaces/Core/Portable/TemporaryStorage/TemporaryStorageService.MemoryMappedInfo.cs @@ -24,7 +24,7 @@ internal partial class TemporaryStorageService /// This class and its nested types have familiar APIs and predictable behavior when used in other code, but /// are non-trivial to work on. /// - internal sealed class MemoryMappedInfo(MemoryMappedFile memoryMappedFile, string name, long offset, long size) + internal sealed class MemoryMappedInfo(MemoryMappedFile memoryMappedFile, string? name, long offset, long size) { /// /// The memory mapped file. @@ -45,16 +45,14 @@ internal sealed class MemoryMappedInfo(MemoryMappedFile memoryMappedFile, string /// private ReferenceCountedDisposable.WeakReference _weakReadAccessor; - public static MemoryMappedInfo OpenExisting(string name, long offset, long size) - => new(MemoryMappedFile.OpenExisting(name), name, offset, size); - - public static MemoryMappedInfo CreateNew(string name, long size) + public static MemoryMappedInfo CreateNew(string? name, long size) => new(MemoryMappedFile.CreateNew(name, size), name, offset: 0, size); /// - /// The name of the memory mapped file. + /// The name of the memory mapped file. Non null on systems that support named memory mapped files, null + /// otherwise.. /// - public string Name { get; } = name; + public string? Name { get; } = name; /// /// The offset into the memory mapped file of the region described by the current diff --git a/src/Workspaces/Core/Portable/TemporaryStorage/TemporaryStorageService.cs b/src/Workspaces/Core/Portable/TemporaryStorage/TemporaryStorageService.cs index 7a8431f550016..079e12d3f7edf 100644 --- a/src/Workspaces/Core/Portable/TemporaryStorage/TemporaryStorageService.cs +++ b/src/Workspaces/Core/Portable/TemporaryStorage/TemporaryStorageService.cs @@ -173,6 +173,7 @@ MemoryMappedInfo WriteToMemoryMappedFile() internal static TemporaryStorageStreamHandle GetStreamHandle(TemporaryStorageIdentifier storageIdentifier) { + Contract.ThrowIfNull(storageIdentifier.Name, $"{nameof(GetStreamHandle)} should only be called for VS on Windows (where named memory mapped files as supported)"); var memoryMappedFile = MemoryMappedFile.OpenExisting(storageIdentifier.Name); return new(memoryMappedFile, storageIdentifier); } @@ -183,6 +184,7 @@ internal TemporaryStorageTextHandle GetTextHandle( Encoding? encoding, ImmutableArray contentHash) { + Contract.ThrowIfNull(storageIdentifier.Name, $"{nameof(GetTextHandle)} should only be called for VS on Windows (where named memory mapped files as supported)"); var memoryMappedFile = MemoryMappedFile.OpenExisting(storageIdentifier.Name); return new(this, memoryMappedFile, storageIdentifier, checksumAlgorithm, encoding, contentHash); } @@ -229,8 +231,15 @@ private MemoryMappedInfo CreateTemporaryStorage(long size) } } - public static string CreateUniqueName(long size) - => "Roslyn Temp Storage " + size.ToString() + " " + Guid.NewGuid().ToString("N"); + public static string? CreateUniqueName(long size) + { + // MemoryMapped files which are used by the TemporaryStorageService are present in .NET Framework (including Mono) + // and .NET Core Windows. For non-Windows .NET Core scenarios, we can return the TrivialTemporaryStorageService + // until https://github.com/dotnet/runtime/issues/30878 is fixed. + return PlatformInformation.IsWindows || PlatformInformation.IsRunningOnMono + ? $"Roslyn Shared File: Size={size} Id={Guid.NewGuid():N}" + : null; + } public sealed class TemporaryStorageTextHandle( TemporaryStorageService storageService, diff --git a/src/Workspaces/Core/Portable/Workspace/Host/TemporaryStorage/ITemporaryStorageService.cs b/src/Workspaces/Core/Portable/Workspace/Host/TemporaryStorage/ITemporaryStorageService.cs index 4079f19f50248..c19c7b99a852b 100644 --- a/src/Workspaces/Core/Portable/Workspace/Host/TemporaryStorage/ITemporaryStorageService.cs +++ b/src/Workspaces/Core/Portable/Workspace/Host/TemporaryStorage/ITemporaryStorageService.cs @@ -18,7 +18,13 @@ public interface ITemporaryStorageService : IWorkspaceService } /// -/// API to allow a client to write data to memory-mapped-file storage (allowing it to be shared across processes). +/// API to allow a client to write data to memory-mapped-file storage. That data can be read back in within the same +/// process using a handle returned from the writing call. The data can optionally be read back in from a different +/// process, using the information contained with the handle's Identifier (see ), but only on systems that support named memory mapped files. Currently, this +/// is any .net on Windows and mono on unix systems. This is not supported on .net core on unix systems (tracked here +/// https://github.com/dotnet/runtime/issues/30878). This is not a problem in practice as cross process sharing is only +/// needed by the VS host, which is windows only. /// internal interface ITemporaryStorageServiceInternal : IWorkspaceService { diff --git a/src/Workspaces/Core/Portable/Workspace/Host/TemporaryStorage/TemporaryStorageIdentifier.cs b/src/Workspaces/Core/Portable/Workspace/Host/TemporaryStorage/TemporaryStorageIdentifier.cs index 2457297fca662..67f19bc52fdce 100644 --- a/src/Workspaces/Core/Portable/Workspace/Host/TemporaryStorage/TemporaryStorageIdentifier.cs +++ b/src/Workspaces/Core/Portable/Workspace/Host/TemporaryStorage/TemporaryStorageIdentifier.cs @@ -2,7 +2,6 @@ // The .NET Foundation licenses this file to you under the MIT license. // See the LICENSE file in the project root for more information. -using System.Runtime.Serialization; using Roslyn.Utilities; namespace Microsoft.CodeAnalysis.Host; @@ -12,11 +11,11 @@ namespace Microsoft.CodeAnalysis.Host; /// used to identify that segment across processes, allowing for efficient sharing of data. /// internal sealed record TemporaryStorageIdentifier( - string Name, long Offset, long Size) + string? Name, long Offset, long Size) { public static TemporaryStorageIdentifier ReadFrom(ObjectReader reader) => new( - reader.ReadRequiredString(), + reader.ReadString(), reader.ReadInt64(), reader.ReadInt64()); diff --git a/src/Workspaces/Core/Portable/Workspace/Host/TemporaryStorage/TrivialTemporaryStorageService.TrivialStorageStreamHandle.cs b/src/Workspaces/Core/Portable/Workspace/Host/TemporaryStorage/TrivialTemporaryStorageService.TrivialStorageStreamHandle.cs deleted file mode 100644 index d3cb02f201d39..0000000000000 --- a/src/Workspaces/Core/Portable/Workspace/Host/TemporaryStorage/TrivialTemporaryStorageService.TrivialStorageStreamHandle.cs +++ /dev/null @@ -1,26 +0,0 @@ -// Licensed to the .NET Foundation under one or more agreements. -// The .NET Foundation licenses this file to you under the MIT license. -// See the LICENSE file in the project root for more information. - -using System.IO; -using System.Threading; -using Microsoft.CodeAnalysis.Host; - -namespace Microsoft.CodeAnalysis; - -internal sealed partial class TrivialTemporaryStorageService -{ - private sealed class TrivialStorageStreamHandle( - TemporaryStorageIdentifier storageIdentifier, - MemoryStream streamCopy) : ITemporaryStorageStreamHandle - { - public TemporaryStorageIdentifier Identifier => storageIdentifier; - - public Stream ReadFromTemporaryStorage(CancellationToken cancellationToken) - { - // Return a read-only view of the underlying buffer to prevent users from overwriting or directly - // disposing the backing storage. - return new MemoryStream(streamCopy.GetBuffer(), 0, (int)streamCopy.Length, writable: false); - } - } -} diff --git a/src/Workspaces/Core/Portable/Workspace/Host/TemporaryStorage/TrivialTemporaryStorageService.TrivialStorageTextHandle.cs b/src/Workspaces/Core/Portable/Workspace/Host/TemporaryStorage/TrivialTemporaryStorageService.TrivialStorageTextHandle.cs deleted file mode 100644 index fb09daa83f991..0000000000000 --- a/src/Workspaces/Core/Portable/Workspace/Host/TemporaryStorage/TrivialTemporaryStorageService.TrivialStorageTextHandle.cs +++ /dev/null @@ -1,26 +0,0 @@ -// Licensed to the .NET Foundation under one or more agreements. -// The .NET Foundation licenses this file to you under the MIT license. -// See the LICENSE file in the project root for more information. - -using System.Threading; -using System.Threading.Tasks; -using Microsoft.CodeAnalysis.Host; -using Microsoft.CodeAnalysis.Text; - -namespace Microsoft.CodeAnalysis; - -internal sealed partial class TrivialTemporaryStorageService -{ - private sealed class TrivialStorageTextHandle( - TemporaryStorageIdentifier identifier, - SourceText sourceText) : ITemporaryStorageTextHandle - { - public TemporaryStorageIdentifier Identifier => identifier; - - public SourceText ReadFromTemporaryStorage(CancellationToken cancellationToken) - => sourceText; - - public Task ReadFromTemporaryStorageAsync(CancellationToken cancellationToken) - => Task.FromResult(ReadFromTemporaryStorage(cancellationToken)); - } -} diff --git a/src/Workspaces/Core/Portable/Workspace/Host/TemporaryStorage/TrivialTemporaryStorageService.cs b/src/Workspaces/Core/Portable/Workspace/Host/TemporaryStorage/TrivialTemporaryStorageService.cs deleted file mode 100644 index a9304b70e07a6..0000000000000 --- a/src/Workspaces/Core/Portable/Workspace/Host/TemporaryStorage/TrivialTemporaryStorageService.cs +++ /dev/null @@ -1,42 +0,0 @@ -// Licensed to the .NET Foundation under one or more agreements. -// The .NET Foundation licenses this file to you under the MIT license. -// See the LICENSE file in the project root for more information. - -using System; -using System.IO; -using System.Threading; -using System.Threading.Tasks; -using Microsoft.CodeAnalysis.Host; -using Microsoft.CodeAnalysis.Text; - -namespace Microsoft.CodeAnalysis; - -internal sealed partial class TrivialTemporaryStorageService : ITemporaryStorageServiceInternal -{ - public static readonly TrivialTemporaryStorageService Instance = new(); - - private TrivialTemporaryStorageService() - { - } - - public ITemporaryStorageTextHandle WriteToTemporaryStorage(SourceText text, CancellationToken cancellationToken) - { - var identifier = new TemporaryStorageIdentifier(Guid.NewGuid().ToString("N"), Offset: 0, Size: text.Length); - var handle = new TrivialStorageTextHandle(identifier, text); - return handle; - } - - public Task WriteToTemporaryStorageAsync(SourceText text, CancellationToken cancellationToken) - => Task.FromResult(WriteToTemporaryStorage(text, cancellationToken)); - - public ITemporaryStorageStreamHandle WriteToTemporaryStorage(Stream stream, CancellationToken cancellationToken) - { - var newStream = new MemoryStream(); - stream.CopyTo(newStream); - newStream.Position = 0; - - var identifier = new TemporaryStorageIdentifier(Guid.NewGuid().ToString("N"), Offset: 0, Size: stream.Length); - var handle = new TrivialStorageStreamHandle(identifier, newStream); - return handle; - } -} diff --git a/src/Workspaces/CoreTest/SolutionTests/SolutionTests.cs b/src/Workspaces/CoreTest/SolutionTests/SolutionTests.cs index 67e9cabedec10..f755dc5c00bd6 100644 --- a/src/Workspaces/CoreTest/SolutionTests/SolutionTests.cs +++ b/src/Workspaces/CoreTest/SolutionTests/SolutionTests.cs @@ -2639,11 +2639,8 @@ public void TestUpdatingFilePathUpdatesSyntaxTree() } } -#if NETCOREAPP - [SupportedOSPlatform("windows")] -#endif [MethodImpl(MethodImplOptions.NoInlining)] - [Fact, WorkItem("http://vstfdevdiv:8080/DevDiv2/DevDiv/_workitems/edit/542736")] + [ConditionalFact(typeof(WindowsOnly)), WorkItem("http://vstfdevdiv:8080/DevDiv2/DevDiv/_workitems/edit/542736")] public void TestDocumentChangedOnDiskIsNotObserved() { var text1 = "public class A {}"; @@ -2669,17 +2666,7 @@ public void TestDocumentChangedOnDiskIsNotObserved() Assert.Equal(text2, textOnDisk); // stop observing it and let GC reclaim it - if (PlatformInformation.IsWindows || PlatformInformation.IsRunningOnMono) - { - Assert.IsType(workspace.Services.GetService()); - observedText.AssertReleased(); - } - else - { - // If this assertion fails, it means a new target supports the true temporary storage service, and the - // condition above should be updated to ensure 'AssertReleased' is called for this target. - Assert.IsType(workspace.Services.GetService()); - } + observedText.AssertReleased(); // if we ask for the same text again we should get the original content var observedText2 = sol.GetDocument(did).GetTextAsync().Result; From d358b004fbb6594b1fc4ad3bc756c03399db022e Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Thu, 25 Apr 2024 17:06:01 -0700 Subject: [PATCH 2/5] remove assert --- .../Core/Portable/TemporaryStorage/TemporaryStorageService.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/Workspaces/Core/Portable/TemporaryStorage/TemporaryStorageService.cs b/src/Workspaces/Core/Portable/TemporaryStorage/TemporaryStorageService.cs index 079e12d3f7edf..8c9fe3c9ac04e 100644 --- a/src/Workspaces/Core/Portable/TemporaryStorage/TemporaryStorageService.cs +++ b/src/Workspaces/Core/Portable/TemporaryStorage/TemporaryStorageService.cs @@ -224,7 +224,6 @@ private MemoryMappedInfo CreateTemporaryStorage(long size) else { // Reserve additional space in the existing storage location - Contract.ThrowIfNull(_name); _offset += size; return new MemoryMappedInfo(reference, _name, _offset - size, size); } From 7825bd706c9aa3b4544a85f072453d32eb9881ae Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Thu, 25 Apr 2024 17:07:32 -0700 Subject: [PATCH 3/5] Docs --- .../Host/TemporaryStorage/TemporaryStorageIdentifier.cs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/Workspaces/Core/Portable/Workspace/Host/TemporaryStorage/TemporaryStorageIdentifier.cs b/src/Workspaces/Core/Portable/Workspace/Host/TemporaryStorage/TemporaryStorageIdentifier.cs index 67f19bc52fdce..8ac99018bef6b 100644 --- a/src/Workspaces/Core/Portable/Workspace/Host/TemporaryStorage/TemporaryStorageIdentifier.cs +++ b/src/Workspaces/Core/Portable/Workspace/Host/TemporaryStorage/TemporaryStorageIdentifier.cs @@ -7,9 +7,11 @@ namespace Microsoft.CodeAnalysis.Host; /// -/// Identifier for a stream of data placed in a segment of temporary storage (generally a memory mapped file). Can be -/// used to identify that segment across processes, allowing for efficient sharing of data. +/// Identifier for a stream of data placed in a segment of a memory mapped file. Can be used to identify that segment +/// across processes (where supported), allowing for efficient sharing of data. /// +/// The name of the segment in the temporary storage. on platforms that don't +/// support cross process sharing of named memory mapped files. internal sealed record TemporaryStorageIdentifier( string? Name, long Offset, long Size) { From 90311b3253eb8b2b43ac448d9331ffdccc39b624 Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Thu, 25 Apr 2024 17:23:43 -0700 Subject: [PATCH 4/5] Docs --- .../Portable/TemporaryStorage/TemporaryStorageService.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Workspaces/Core/Portable/TemporaryStorage/TemporaryStorageService.cs b/src/Workspaces/Core/Portable/TemporaryStorage/TemporaryStorageService.cs index 8c9fe3c9ac04e..56a205c39481b 100644 --- a/src/Workspaces/Core/Portable/TemporaryStorage/TemporaryStorageService.cs +++ b/src/Workspaces/Core/Portable/TemporaryStorage/TemporaryStorageService.cs @@ -232,9 +232,9 @@ private MemoryMappedInfo CreateTemporaryStorage(long size) public static string? CreateUniqueName(long size) { - // MemoryMapped files which are used by the TemporaryStorageService are present in .NET Framework (including Mono) - // and .NET Core Windows. For non-Windows .NET Core scenarios, we can return the TrivialTemporaryStorageService - // until https://github.com/dotnet/runtime/issues/30878 is fixed. + // MemoryMapped files which are used by the TemporaryStorageService are present in .NET Framework (including + // Mono) and .NET Core Windows. For non-Windows .NET Core scenarios, we return null to enable create the memory + // mapped file (just not in a way that can be shared across processes). return PlatformInformation.IsWindows || PlatformInformation.IsRunningOnMono ? $"Roslyn Shared File: Size={size} Id={Guid.NewGuid():N}" : null; From 59a7bacab3df80c68d4adee9a7e116e8343b3fca Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Thu, 25 Apr 2024 18:14:06 -0700 Subject: [PATCH 5/5] Change API now that we know it's always about MMFS --- .../TemporaryStorageService.TemporaryStorageStreamHandle.cs | 3 --- .../Host/TemporaryStorage/ITemporaryStorageStreamHandle.cs | 2 +- 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/src/Workspaces/Core/Portable/TemporaryStorage/TemporaryStorageService.TemporaryStorageStreamHandle.cs b/src/Workspaces/Core/Portable/TemporaryStorage/TemporaryStorageService.TemporaryStorageStreamHandle.cs index 7b06db02ddbff..52d97d1bd762c 100644 --- a/src/Workspaces/Core/Portable/TemporaryStorage/TemporaryStorageService.TemporaryStorageStreamHandle.cs +++ b/src/Workspaces/Core/Portable/TemporaryStorage/TemporaryStorageService.TemporaryStorageStreamHandle.cs @@ -18,9 +18,6 @@ public sealed class TemporaryStorageStreamHandle( { public TemporaryStorageIdentifier Identifier => identifier; - Stream ITemporaryStorageStreamHandle.ReadFromTemporaryStorage(CancellationToken cancellationToken) - => ReadFromTemporaryStorage(cancellationToken); - public UnmanagedMemoryStream ReadFromTemporaryStorage(CancellationToken cancellationToken) { using (Logger.LogBlock(FunctionId.TemporaryStorageServiceFactory_ReadStream, cancellationToken)) diff --git a/src/Workspaces/Core/Portable/Workspace/Host/TemporaryStorage/ITemporaryStorageStreamHandle.cs b/src/Workspaces/Core/Portable/Workspace/Host/TemporaryStorage/ITemporaryStorageStreamHandle.cs index 418a8a7fe50cf..a278f5a73ab30 100644 --- a/src/Workspaces/Core/Portable/Workspace/Host/TemporaryStorage/ITemporaryStorageStreamHandle.cs +++ b/src/Workspaces/Core/Portable/Workspace/Host/TemporaryStorage/ITemporaryStorageStreamHandle.cs @@ -22,5 +22,5 @@ internal interface ITemporaryStorageStreamHandle /// Reads the data indicated to by this handle into a stream. This stream can be created in a different process /// than the one that wrote the data originally. /// - Stream ReadFromTemporaryStorage(CancellationToken cancellationToken); + UnmanagedMemoryStream ReadFromTemporaryStorage(CancellationToken cancellationToken); }