diff --git a/src/Workspaces/Core/Portable/Remote/IRemoteKeepAliveService.cs b/src/Workspaces/Core/Portable/Remote/IRemoteKeepAliveService.cs index 44fef3920f41a..a9c1189a0f783 100644 --- a/src/Workspaces/Core/Portable/Remote/IRemoteKeepAliveService.cs +++ b/src/Workspaces/Core/Portable/Remote/IRemoteKeepAliveService.cs @@ -5,44 +5,186 @@ using System; using System.Threading; using System.Threading.Tasks; +using Microsoft.CodeAnalysis.ErrorReporting; using Microsoft.CodeAnalysis.Shared.TestHooks; +using Microsoft.CodeAnalysis.Threading; namespace Microsoft.CodeAnalysis.Remote; internal interface IRemoteKeepAliveService { /// - /// Keeps alive this solution in the OOP process until the cancellation token is triggered. Used so that long - /// running features (like 'inline rename' or 'lightbulbs') we can call into oop several times, with the same - /// snapshot, knowing that things will stay hydrated and alive on the OOP side. Importantly, by keeping the - /// same snapshot alive on the OOP side, computed attached values (like s) will stay alive as well. + /// Keeps the solution identified by alive in the OOP process until is triggered. This enables long-running features (like inline rename or lightbulbs) + /// to make multiple OOP calls against the same snapshot, ensuring that computed values (like s) remain cached rather than being rebuilt on each call. /// - ValueTask KeepAliveAsync(Checksum solutionChecksum, CancellationToken cancellationToken); + /// Checksum identifying the solution to pin. + /// Unique identifier for this session. The host uses this with to block until the solution is actually pinned before proceeding with + /// dependent work. + /// Cancellation of this token releases the pinned solution. + ValueTask KeepAliveAsync(Checksum solutionChecksum, long sessionId, CancellationToken cancellationToken); + + /// + /// Blocks until the session identified by has fully synced and pinned its solution + /// in the OOP process. This ensures the host doesn't proceed with OOP calls until the solution is guaranteed to + /// be available. + /// + ValueTask WaitForSessionIdAsync(long sessionId, CancellationToken cancellationToken); } internal sealed class RemoteKeepAliveSession : IDisposable { - private readonly CancellationTokenSource _cancellationTokenSource = new(); + private static long s_sessionId = 1; + + /// + /// Unique identifier for this session. Used to coordinate between + /// (which syncs and pins the solution) and (which blocks + /// until pinning completes). + /// + private long SessionId { get; } = Interlocked.Increment(ref s_sessionId); - private RemoteKeepAliveSession( + /// + /// Controls the lifetime of the OOP-side pinning. The call + /// blocks on this token; canceling it allows that call to return, releasing the pinned solution. + /// + private CancellationTokenSource KeepAliveTokenSource { get; } = new(); + + private RemoteKeepAliveSession() + { + } + + /// + /// Creates and fully establishes a keep-alive session. Returns only after the solution is confirmed to be + /// pinned on the OOP side. + /// + /// + /// This method coordinates two concurrent OOP calls: + /// + /// : Syncs the solution to OOP, then blocks until + /// is canceled. This call is fire-and-forget from the host's perspective. + /// : Blocks until KeepAliveAsync has completed + /// syncing. This call is awaited, ensuring the solution is pinned before returning to the caller. + /// + /// The two calls share so the OOP side can correlate them. + /// + private static async Task StartSessionAsync( SolutionCompilationState compilationState, - RemoteHostClient? client) + ProjectId? projectId, + RemoteHostClient? client, + CancellationToken callerCancellationToken) { + var session = new RemoteKeepAliveSession(); + + // When running in-process (no OOP client), return immediately. The caller holds the solution snapshot + // directly, so no pinning is needed. if (client is null) - return; + return session; + + // Fire-and-forget: Start syncing and pinning the solution on the OOP side. This call will block on the OOP + // side until KeepAliveTokenSource is canceled (i.e., when this session is Disposed). + // + // Important: We pass KeepAliveTokenSource.Token (not callerCancellationToken) because: + // - The keep-alive must persist beyond this method, for the lifetime of the session + // - Disposing the session is what should cancel this work, not the caller's token + _ = InvokeKeepAliveAsync(compilationState, projectId, client, session); + + // Block until the OOP side confirms the solution is pinned. This uses callerCancellationToken so the caller + // can abandon the wait if they no longer need the session. + await WaitForSessionIdAsync(compilationState, projectId, client, session, callerCancellationToken).ConfigureAwait(false); + + return session; - // Now kick off the keep-alive work. We don't wait on this as this will stick on the OOP side until - // the cancellation token triggers. - _ = client.TryInvokeAsync( - compilationState, - (service, solutionInfo, cancellationToken) => service.KeepAliveAsync(solutionInfo, cancellationToken), - _cancellationTokenSource.Token).AsTask(); + static async Task InvokeKeepAliveAsync( + SolutionCompilationState compilationState, + ProjectId? projectId, + RemoteHostClient client, + RemoteKeepAliveSession session) + { + try + { + // Yield to allow StartSessionAsync to proceed to WaitForSessionIdAsync concurrently. + await Task.Yield().ConfigureAwait(false); + + var sessionId = session.SessionId; + await client.TryInvokeAsync( + compilationState, + projectId, + (service, solutionInfo, cancellationToken) => service.KeepAliveAsync(solutionInfo, sessionId, cancellationToken), + session.KeepAliveTokenSource.Token).ConfigureAwait(false); + } + catch (Exception ex) when (FatalError.ReportAndCatchUnlessCanceled(ex)) + { + // Non-cancellation exceptions indicate a catastrophic failure (e.g., broken OOP connection). + // We must dispose the session to: + // 1. Cancel KeepAliveTokenSource, which is linked into WaitForSessionIdAsync's token + // 2. Unblock WaitForSessionIdAsync so it doesn't hang forever + // + // Cancellation exceptions are expected and normal - they occur when the session is properly + // disposed, which cancels KeepAliveTokenSource and allows KeepAliveAsync to return. + session.Dispose(); + + // Don't rethrow: this is fire-and-forget. Errors were already reported via FatalError above. + } + } + + static async Task WaitForSessionIdAsync( + SolutionCompilationState compilationState, + ProjectId? projectId, + RemoteHostClient client, + RemoteKeepAliveSession session, + CancellationToken callerCancellationToken) + { + try + { + // Link both cancellation sources so this call aborts if either: + // - The caller cancels (they no longer need the session) + // - InvokeKeepAliveAsync fails and disposes the session (which cancels KeepAliveTokenSource) + // + // Without the link to KeepAliveTokenSource, a failure in InvokeKeepAliveAsync would leave this + // call hanging indefinitely since the OOP side would never signal completion. + using var linkedTokenSource = CancellationTokenSource.CreateLinkedTokenSource( + session.KeepAliveTokenSource.Token, + callerCancellationToken); + + await client.TryInvokeAsync( + compilationState, + projectId, + (service, _, cancellationToken) => service.WaitForSessionIdAsync(session.SessionId, cancellationToken), + linkedTokenSource.Token).ConfigureAwait(false); + } + catch + { + // Any failure means we can't establish the session. The three lines below handle all cases: + // + // 1. Dispose(): Always clean up. The caller won't receive the session, so we must release it. + // This also cancels KeepAliveTokenSource, which unblocks InvokeKeepAliveAsync if it's still running. + // + // 2. ThrowIfCancellationRequested(): If the caller's token caused the cancellation, rethrow with + // that token to maintain proper cancellation semantics (the exception's CancellationToken + // property should match what the caller passed in). This is a no-op if the caller didn't cancel. + // + // 3. throw: For all other failures (e.g., KeepAliveTokenSource canceled due to InvokeKeepAliveAsync + // failing, or a non-cancellation exception), propagate the original exception. + session.Dispose(); + callerCancellationToken.ThrowIfCancellationRequested(); + throw; + } + } } + /// + /// Constructor for synchronous, best-effort session creation. Does not wait for the session to be established. + /// private RemoteKeepAliveSession(SolutionCompilationState compilationState, IAsynchronousOperationListener listener) { - var cancellationToken = _cancellationTokenSource.Token; + // Unlike the async entry-point, this constructor returns immediately without waiting for the solution to + // be pinned on the OOP side. This is acceptable for scenarios where: + // - The caller cannot await (e.g., in a constructor) + // - Best-effort pinning is sufficient (subsequent OOP calls will still work, just potentially slower) + // + // Track the async work so test infrastructure can detect outstanding operations. var token = listener.BeginAsyncOperation(nameof(RemoteKeepAliveSession)); var task = CreateClientAndKeepAliveAsync(); @@ -52,16 +194,25 @@ private RemoteKeepAliveSession(SolutionCompilationState compilationState, IAsync async Task CreateClientAndKeepAliveAsync() { + var cancellationToken = this.KeepAliveTokenSource.Token; var client = await RemoteHostClient.TryGetClientAsync(compilationState.Services, cancellationToken).ConfigureAwait(false); if (client is null) return; - // Now kick off the keep-alive work. We don't wait on this as this will stick on the OOP side until - // the cancellation token triggers. - _ = client.TryInvokeAsync( - compilationState, - (service, solutionInfo, cancellationToken) => service.KeepAliveAsync(solutionInfo, cancellationToken), - cancellationToken).AsTask(); + // Fire-and-forget: Start the keep-alive without waiting for confirmation. Unlike StartSessionAsync, + // we don't call WaitForSessionIdAsync because this is a best-effort, non-blocking path. + try + { + var sessionId = this.SessionId; + await client.TryInvokeAsync( + compilationState, + projectId: null, + (service, solutionInfo, cancellationToken) => service.KeepAliveAsync(solutionInfo, sessionId, cancellationToken), + cancellationToken).ConfigureAwait(false); + } + catch (Exception ex) when (FatalError.ReportAndCatchUnlessCanceled(ex)) + { + } } } @@ -76,43 +227,54 @@ async Task CreateClientAndKeepAliveAsync() public void Dispose() { GC.SuppressFinalize(this); - _cancellationTokenSource.Cancel(); - _cancellationTokenSource.Dispose(); + + // Cancel rather than dispose the token source. CancellationTokenSource.Dispose() is only necessary when + // not canceling (to clean up internal wait handles), but we always cancel. The finalizer's Contract.Fail + // will catch any cases where Dispose is forgotten. + this.KeepAliveTokenSource.Cancel(); } /// - /// Creates a session between the host and OOP, effectively pinning this until is called on it. By pinning the solution we ensure that all calls to OOP for - /// the same solution during the life of this session do not need to resync the solution. Nor do they then need - /// to rebuild any compilations they've already built due to the solution going away and then coming back. + /// Creates a best-effort keep-alive session synchronously. Returns immediately without waiting for the session + /// to be established on the OOP side. /// /// - /// The is not strictly necessary for this type. This class functions just as an - /// optimization to hold onto data so it isn't resync'ed or recomputed. However, we still want to let the - /// system know when unobserved async work is kicked off in case we have any tooling that keep track of this for - /// any reason (for example for tracking down problems in testing scenarios). - /// - /// - /// This synchronous entrypoint should be used only in contexts where using the async is not possible (for example, in a constructor). + /// Use this overload only when async code is not possible (e.g., in constructors). For guaranteed session + /// establishment, use instead. + /// Because this method doesn't wait for establishment, subsequent OOP calls may not benefit from the + /// pinned solution if they race ahead of the keep-alive setup. In practice this is rare, but callers requiring + /// guaranteed consistency must use the async overloads. + /// The is used to track the async keep-alive work for testing infrastructure. /// public static RemoteKeepAliveSession Create(Solution solution, IAsynchronousOperationListener listener) => new(solution.CompilationState, listener); /// - /// Creates a session between the host and OOP, effectively pinning this until is called on it. By pinning the solution we ensure that all calls to OOP for - /// the same solution during the life of this session do not need to resync the solution. Nor do they then need - /// to rebuild any compilations they've already built due to the solution going away and then coming back. + /// Creates a keep-alive session, returning only after the session is fully established on the OOP side. /// + /// + /// All subsequent OOP calls made while this session is alive will see the same pinned solution instance, + /// provided they pass matching solution/project-cone data. Mismatched calls (e.g., session created for full + /// solution but call made for project-cone) will not benefit from the pinning. + /// public static Task CreateAsync(Solution solution, CancellationToken cancellationToken) - => CreateAsync(solution.CompilationState, cancellationToken); + => CreateAsync(solution, projectId: null, cancellationToken); + + /// + public static Task CreateAsync(Solution solution, ProjectId? projectId, CancellationToken cancellationToken) + => CreateAsync(solution.CompilationState, projectId, cancellationToken); + + /// + public static Task CreateAsync(SolutionCompilationState compilationState, CancellationToken cancellationToken) + => CreateAsync(compilationState, projectId: null, cancellationToken); /// public static async Task CreateAsync( - SolutionCompilationState compilationState, CancellationToken cancellationToken) + SolutionCompilationState compilationState, ProjectId? projectId, CancellationToken cancellationToken) { - var client = await RemoteHostClient.TryGetClientAsync(compilationState.Services, cancellationToken).ConfigureAwait(false); - return new RemoteKeepAliveSession(compilationState, client); + var client = await RemoteHostClient.TryGetClientAsync( + compilationState.Services, cancellationToken).ConfigureAwait(false); + + return await StartSessionAsync(compilationState, projectId, client, cancellationToken).ConfigureAwait(false); } } diff --git a/src/Workspaces/Core/Portable/Remote/RemoteHostClient.cs b/src/Workspaces/Core/Portable/Remote/RemoteHostClient.cs index 54846ba331e66..433dca5a99a23 100644 --- a/src/Workspaces/Core/Portable/Remote/RemoteHostClient.cs +++ b/src/Workspaces/Core/Portable/Remote/RemoteHostClient.cs @@ -105,17 +105,24 @@ public ValueTask TryInvokeAsync( CancellationToken cancellationToken) where TService : class { - return TryInvokeAsync(solution.CompilationState, invocation, cancellationToken); + return TryInvokeAsync(solution.CompilationState, projectId: null, invocation, cancellationToken); } + /// If the entire solution snapshot represented by will be synchronized with the OOP side. If not only the + /// project-cone represented by that id will be synchronized over. + /// public async ValueTask TryInvokeAsync( SolutionCompilationState compilationState, + ProjectId? projectId, Func invocation, CancellationToken cancellationToken) where TService : class { using var connection = CreateConnection(callbackTarget: null); - return await connection.TryInvokeAsync(compilationState, invocation, cancellationToken).ConfigureAwait(false); + return projectId is null + ? await connection.TryInvokeAsync(compilationState, invocation, cancellationToken).ConfigureAwait(false) + : await connection.TryInvokeAsync(compilationState, projectId, invocation, cancellationToken).ConfigureAwait(false); } public async ValueTask> TryInvokeAsync( diff --git a/src/Workspaces/Core/Portable/Workspace/Solution/SolutionCompilationState.RegularCompilationTracker_Generators.cs b/src/Workspaces/Core/Portable/Workspace/Solution/SolutionCompilationState.RegularCompilationTracker_Generators.cs index f7300c281c98b..55829e99ea81b 100644 --- a/src/Workspaces/Core/Portable/Workspace/Solution/SolutionCompilationState.RegularCompilationTracker_Generators.cs +++ b/src/Workspaces/Core/Portable/Workspace/Solution/SolutionCompilationState.RegularCompilationTracker_Generators.cs @@ -106,6 +106,7 @@ private async Task HasRequiredGeneratorsAsync(SolutionCompilationState com CancellationToken cancellationToken) { var solution = compilationState.SolutionState; + var projectId = this.ProjectState.Id; var client = await RemoteHostClient.TryGetClientAsync(solution.Services, cancellationToken).ConfigureAwait(false); if (client is null) @@ -114,14 +115,23 @@ private async Task HasRequiredGeneratorsAsync(SolutionCompilationState com // We're going to be making multiple calls over to OOP. No point in resyncing data multiple times. Keep a // single connection, and keep this solution instance alive (and synced) on both sides of the connection // throughout the calls. + // + // CRITICAL: We pass the "compilationState+projectId" as the context for the connection. All subsequent + // uses of this connection must do that aas well. This ensures that all calls will see the same exact + // snapshot on the OOP side, which is necessary for the GetSourceGeneratedDocumentInfoAsync and + // GetContentsAsync calls to see the exact same data and return sensible results. using var connection = client.CreateConnection(callbackTarget: null); - using var _ = await RemoteKeepAliveSession.CreateAsync(compilationState, cancellationToken).ConfigureAwait(false); + using var _ = await RemoteKeepAliveSession.CreateAsync( + compilationState, projectId, cancellationToken).ConfigureAwait(false); // First, grab the info from our external host about the generated documents it has for this project. Note: // we ourselves are the innermost "RegularCompilationTracker" responsible for actually running generators. // As such, our call to the oop side reflects that by asking for the real source generated docs, and *not* // any overlaid 'frozen' source generated documents. - var projectId = this.ProjectState.Id; + // + // CRITICAL: We pass the "compilationState+projectId" as the context for the invocation, matching the + // KeepAliveSession above. This ensures the call to GetContentsAsync below sees the exact same solution + // instance as this call. var infosOpt = await connection.TryInvokeAsync( compilationState, projectId, @@ -191,6 +201,10 @@ private async Task HasRequiredGeneratorsAsync(SolutionCompilationState com // "RegularCompilationTracker" responsible for actually running generators. As such, our call to the oop // side reflects that by asking for the real source generated docs, and *not* any overlaid 'frozen' source // generated documents. + // + // CRITICAL: We pass the "compilationState+projectId" as the context for the invocation, matching the + // KeepAliveSession above. This ensures that we see the exact same solution instance on the OOP side as the + // call to GetSourceGeneratedDocumentInfoAsync above. var generatedSourcesOpt = await connection.TryInvokeAsync( compilationState, projectId, diff --git a/src/Workspaces/Remote/ServiceHub/Services/KeepAlive/RemoteKeepAliveService.cs b/src/Workspaces/Remote/ServiceHub/Services/KeepAlive/RemoteKeepAliveService.cs index 749dc0c6b9128..34b3a2b1cd06f 100644 --- a/src/Workspaces/Remote/ServiceHub/Services/KeepAlive/RemoteKeepAliveService.cs +++ b/src/Workspaces/Remote/ServiceHub/Services/KeepAlive/RemoteKeepAliveService.cs @@ -2,8 +2,10 @@ // 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.Collections.Concurrent; using System.Threading; using System.Threading.Tasks; +using Microsoft.VisualStudio.Threading; namespace Microsoft.CodeAnalysis.Remote; @@ -15,23 +17,69 @@ protected override IRemoteKeepAliveService CreateService(in ServiceConstructionA => new RemoteKeepAliveService(arguments); } + /// + /// Mapping from sessionId to the completion source that will be used to signal when that session's solution + /// snapshot is fully hydrated on the OOP (this) side. The `bool` result of the TaskCompletionSource is unused, and + /// is only there because TaskCompletionSource requires some type on netstandard2.0. + /// + private readonly ConcurrentDictionary> _sessionIdToCompletionSource = new(); + public RemoteKeepAliveService(in ServiceConstructionArguments arguments) : base(arguments) { } - public ValueTask KeepAliveAsync( + private TaskCompletionSource GetSessionCompletionSource(long sessionId) + => _sessionIdToCompletionSource.GetOrAdd(sessionId, static _ => new()); + + public async ValueTask KeepAliveAsync( Checksum solutionChecksum, + long sessionId, CancellationToken cancellationToken) { - // First get the solution, ensuring that it is currently pinned. - return RunServiceAsync(solutionChecksum, async solution => + // Ensure we have a completion source for this sessionId. Note: we are potentially racing with + // WaitForSessionIdAsync on the host side, so it's possible that it will beat us to creating the entry in this + // dictionary. That's fine though, as both calls will be guaranteed to get the same completionSource thanks to + // GetOrAdd. + var completionSource = GetSessionCompletionSource(sessionId); + try + { + // First get the solution, ensuring that it is currently pinned. + await RunServiceAsync(solutionChecksum, async solution => + { + // Now that we have the solution, we can trigger the completion source to let the host know it can + // proceed with its work. + completionSource.TrySetResult(true); + + // Wait for our caller to tell us to cancel. That way we can release this solution and allow it + // to be collected if not needed anymore. + // + // This was provided by stoub as an idiomatic way to wait indefinitely until a cancellation token triggers. + await Task.Delay(Timeout.Infinite, cancellationToken).ConfigureAwait(false); + }, cancellationToken).ConfigureAwait(false); + } + finally { - // Wait for our caller to tell us to cancel. That way we can release this solution and allow it - // to be collected if not needed anymore. - // - // This was provided by stoub as an idiomatic way to wait indefinitely until a cancellation token triggers. - await Task.Delay(-1, cancellationToken).ConfigureAwait(false); - }, cancellationToken); + // Now that we've been cancelled, remove the completion source from our map. Note: this cancellationToken + // is the one owned by RemoteKeepAliveSession, and only triggered in its Dispose method. We only get to + // that point once the RemoteKeepAliveSession is fully created, returned back out to the caller, and then + // eventually Disposed of. That means that by the time we get here, WaitForSessionIdAsync already must have + // returned, so there is no concern about racing at this point, and we are certain to clear out the value + // and not leave around stale entries. + Contract.ThrowIfFalse(_sessionIdToCompletionSource.TryRemove(sessionId, out var foundSource)); + Contract.ThrowIfFalse(foundSource == completionSource); + } + } + + public async ValueTask WaitForSessionIdAsync(long sessionId, CancellationToken cancellationToken) + { + // Ensure we have a completion source for this sessionId. Note: we are potentially racing with KeepAliveAsync + // on the host side, so it's possible that it will beat us to creating the entry in this dictionary. That's + // fine though, as both calls will be guaranteed to get the same completionSource thanks to GetOrAdd. + var completionSource = GetSessionCompletionSource(sessionId); + + // Now, wait for KeepAliveAsync to finally signal us to proceed. We also listen for cancellation so that the + // host can bail out if needed, without waiting on the full solution (or project cone) to sync over. + await completionSource.Task.WithCancellation(cancellationToken).ConfigureAwait(false); } }