From 276d605423fe3e51cb41d2a8e8258841a13c3e94 Mon Sep 17 00:00:00 2001 From: tmat Date: Tue, 6 Sep 2022 09:01:09 -0700 Subject: [PATCH 1/4] Remove timeouts from BlazorWebAssemblyDeltaApplier --- .../BlazorWebAssemblyDeltaApplier.cs | 46 +++++-------------- 1 file changed, 11 insertions(+), 35 deletions(-) diff --git a/src/BuiltInTools/dotnet-watch/HotReload/BlazorWebAssemblyDeltaApplier.cs b/src/BuiltInTools/dotnet-watch/HotReload/BlazorWebAssemblyDeltaApplier.cs index c65ab9789b2e..981bf3d157aa 100644 --- a/src/BuiltInTools/dotnet-watch/HotReload/BlazorWebAssemblyDeltaApplier.cs +++ b/src/BuiltInTools/dotnet-watch/HotReload/BlazorWebAssemblyDeltaApplier.cs @@ -24,8 +24,6 @@ internal class BlazorWebAssemblyDeltaApplier : IDeltaApplier private readonly IReporter _reporter; private int _sequenceId; - private static readonly TimeSpan VerifyDeltaTimeout = TimeSpan.FromSeconds(5); - public BlazorWebAssemblyDeltaApplier(IReporter reporter) { _reporter = reporter; @@ -59,9 +57,7 @@ async Task> GetApplyUpdateCapabilitiesCoreAsync() { // We'll query the browser and ask it send capabilities. If the browser does not respond in a short duration, we'll assume something is amiss and return // baseline capabilities. - var response = await context.BrowserRefreshServer.ReceiveAsync(buffer, cancellationToken) - .AsTask() - .WaitAsync(TimeSpan.FromSeconds(15), cancellationToken); + var response = await context.BrowserRefreshServer.ReceiveAsync(buffer, cancellationToken); if (!response.HasValue || !response.Value.EndOfMessage || response.Value.MessageType != WebSocketMessageType.Text) { return _baselineCapabilities; @@ -74,15 +70,10 @@ async Task> GetApplyUpdateCapabilitiesCoreAsync() var result = values.Split(' ').ToImmutableArray(); return result; } - catch (TimeoutException) - { - } finally { ArrayPool.Shared.Return(buffer); } - - return _baselineCapabilities; } } @@ -104,7 +95,7 @@ public async ValueTask Apply(DotNetWatchContext context, ImmutableArray new UpdatePayload { SharedSecret = sharedSecret, Deltas = deltas }, cancellationToken); - return await VerifyDeltaApplied(context, cancellationToken).WaitAsync(VerifyDeltaTimeout, cancellationToken); + return await VerifyDeltaApplied(context, cancellationToken); } public async ValueTask ReportDiagnosticsAsync(DotNetWatchContext context, IEnumerable diagnostics, CancellationToken cancellationToken) @@ -123,33 +114,18 @@ public async ValueTask ReportDiagnosticsAsync(DotNetWatchContext context, IEnume private async Task VerifyDeltaApplied(DotNetWatchContext context, CancellationToken cancellationToken) { var _receiveBuffer = new byte[1]; - try + var result = await context.BrowserRefreshServer!.ReceiveAsync(_receiveBuffer, cancellationToken); + if (result is null) { - // We want to give the client some time to ACK the deltas being applied. VerifyDeltaApplied is limited by a - // 5 second wait timeout enforced using a WaitAsync. However, WaitAsync only works reliably if the calling - // function is async. If BrowserRefreshServer.ReceiveAsync finishes synchronously, the WaitAsync would - // never have an opportunity to execute. Consequently, we'll give it some reasonable number of opportunities - // to loop before we decide that applying deltas failed. - for (var i = 0; i < 100; i++) - { - var result = await context.BrowserRefreshServer!.ReceiveAsync(_receiveBuffer, cancellationToken); - if (result is null) - { - // A null result indicates no clients are connected. No deltas could have been applied in this state. - _reporter.Verbose("No client is connected to ack deltas"); - return false; - } - - if (IsDeltaReceivedMessage(result.Value)) - { - // 1 indicates success. - return _receiveBuffer[0] == 1; - } - } + // A null result indicates no clients are connected. No deltas could have been applied in this state. + _reporter.Verbose("No client is connected to ack deltas"); + return false; } - catch (TaskCanceledException) + + if (IsDeltaReceivedMessage(result.Value)) { - _reporter.Verbose("Timed out while waiting to verify delta was applied."); + // 1 indicates success. + return _receiveBuffer[0] == 1; } return false; From 4e643fac7158e0415c015d8a4de356af497c2722 Mon Sep 17 00:00:00 2001 From: tmat Date: Tue, 6 Sep 2022 09:37:32 -0700 Subject: [PATCH 2/4] Remove timeout from delta applier. --- .../HotReload/DefaultDeltaApplier.cs | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-) diff --git a/src/BuiltInTools/dotnet-watch/HotReload/DefaultDeltaApplier.cs b/src/BuiltInTools/dotnet-watch/HotReload/DefaultDeltaApplier.cs index a46a27ab8c15..757a5ec6c94e 100644 --- a/src/BuiltInTools/dotnet-watch/HotReload/DefaultDeltaApplier.cs +++ b/src/BuiltInTools/dotnet-watch/HotReload/DefaultDeltaApplier.cs @@ -46,9 +46,9 @@ public ValueTask InitializeAsync(DotNetWatchContext context, CancellationToken c { await _connectionTask; // When the client connects, the first payload it sends is the initialization payload which includes the apply capabilities. - var capabiltiies = ClientInitializationPayload.Read(_pipe).Capabilities; - _reporter.Verbose($"Application supports the following capabilities {capabiltiies}."); - return capabiltiies.Split(' ').ToImmutableArray(); + var capabilities = ClientInitializationPayload.Read(_pipe).Capabilities; + _reporter.Verbose($"Application supports the following capabilities {capabilities}."); + return capabilities.Split(' ').ToImmutableArray(); } catch { @@ -101,15 +101,7 @@ public async ValueTask Apply(DotNetWatchContext context, ImmutableArray.Shared.Rent(1); try { - var timeout = -#if DEBUG - Timeout.InfiniteTimeSpan; -#else - TimeSpan.FromSeconds(5); -#endif - - using var cancellationTokenSource = new CancellationTokenSource(timeout); - var numBytes = await _pipe.ReadAsync(bytes, cancellationTokenSource.Token); + var numBytes = await _pipe.ReadAsync(bytes, cancellationToken); if (numBytes == 1) { From 87531d8ca197ece9a5b54b878bff3e487feac441 Mon Sep 17 00:00:00 2001 From: tmat Date: Tue, 6 Sep 2022 19:53:55 -0700 Subject: [PATCH 3/4] Remove comment --- .../dotnet-watch/HotReload/BlazorWebAssemblyDeltaApplier.cs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/BuiltInTools/dotnet-watch/HotReload/BlazorWebAssemblyDeltaApplier.cs b/src/BuiltInTools/dotnet-watch/HotReload/BlazorWebAssemblyDeltaApplier.cs index 981bf3d157aa..179655712c17 100644 --- a/src/BuiltInTools/dotnet-watch/HotReload/BlazorWebAssemblyDeltaApplier.cs +++ b/src/BuiltInTools/dotnet-watch/HotReload/BlazorWebAssemblyDeltaApplier.cs @@ -55,8 +55,7 @@ async Task> GetApplyUpdateCapabilitiesCoreAsync() var buffer = ArrayPool.Shared.Rent(32 * 1024); try { - // We'll query the browser and ask it send capabilities. If the browser does not respond in a short duration, we'll assume something is amiss and return - // baseline capabilities. + // We'll query the browser and ask it send capabilities. var response = await context.BrowserRefreshServer.ReceiveAsync(buffer, cancellationToken); if (!response.HasValue || !response.Value.EndOfMessage || response.Value.MessageType != WebSocketMessageType.Text) { From 655d7573f60d5798d8078963ad6c604082050cbc Mon Sep 17 00:00:00 2001 From: tmat Date: Tue, 13 Sep 2022 15:06:33 -0700 Subject: [PATCH 4/4] Fail when capabilities can't be retrieved --- .../Filters/BrowserRefreshServer.cs | 1 - .../BlazorWebAssemblyDeltaApplier.cs | 24 ++++++------- .../HotReload/CompilationWorkspaceProvider.cs | 36 ++++++++----------- .../HotReload/DefaultDeltaApplier.cs | 33 +++++++---------- 4 files changed, 38 insertions(+), 56 deletions(-) diff --git a/src/BuiltInTools/dotnet-watch/Filters/BrowserRefreshServer.cs b/src/BuiltInTools/dotnet-watch/Filters/BrowserRefreshServer.cs index 6fd0879dcaef..16aac65a340d 100644 --- a/src/BuiltInTools/dotnet-watch/Filters/BrowserRefreshServer.cs +++ b/src/BuiltInTools/dotnet-watch/Filters/BrowserRefreshServer.cs @@ -125,7 +125,6 @@ private async Task WebSocketRequest(HttpContext context) public async Task WaitForClientConnectionAsync(CancellationToken cancellationToken) { - _reporter.Verbose("Waiting for a browser to connect"); await _clientConnected.Task.WaitAsync(cancellationToken); } diff --git a/src/BuiltInTools/dotnet-watch/HotReload/BlazorWebAssemblyDeltaApplier.cs b/src/BuiltInTools/dotnet-watch/HotReload/BlazorWebAssemblyDeltaApplier.cs index 179655712c17..1727442ab28a 100644 --- a/src/BuiltInTools/dotnet-watch/HotReload/BlazorWebAssemblyDeltaApplier.cs +++ b/src/BuiltInTools/dotnet-watch/HotReload/BlazorWebAssemblyDeltaApplier.cs @@ -19,8 +19,7 @@ namespace Microsoft.DotNet.Watcher.Tools { internal class BlazorWebAssemblyDeltaApplier : IDeltaApplier { - private static Task>? _cachedCapabilties; - private static readonly ImmutableArray _baselineCapabilities = ImmutableArray.Create("Baseline"); + private static Task>? s_cachedCapabilties; private readonly IReporter _reporter; private int _sequenceId; @@ -38,20 +37,20 @@ public ValueTask InitializeAsync(DotNetWatchContext context, CancellationToken c public Task> GetApplyUpdateCapabilitiesAsync(DotNetWatchContext context, CancellationToken cancellationToken) { - _cachedCapabilties ??= GetApplyUpdateCapabilitiesCoreAsync(); - return _cachedCapabilties; + return s_cachedCapabilties ??= GetApplyUpdateCapabilitiesCoreAsync(); async Task> GetApplyUpdateCapabilitiesCoreAsync() { if (context.BrowserRefreshServer is null) { - return _baselineCapabilities; + throw new ApplicationException("The browser refresh server is unavailable."); } - await context.BrowserRefreshServer.WaitForClientConnectionAsync(cancellationToken); + _reporter.Verbose("Connecting to the browser."); + await context.BrowserRefreshServer.WaitForClientConnectionAsync(cancellationToken); await context.BrowserRefreshServer.SendJsonSerlialized(default(BlazorRequestApplyUpdateCapabilities), cancellationToken); - // 32k ought to be enough for anyone. + var buffer = ArrayPool.Shared.Rent(32 * 1024); try { @@ -59,15 +58,14 @@ async Task> GetApplyUpdateCapabilitiesCoreAsync() var response = await context.BrowserRefreshServer.ReceiveAsync(buffer, cancellationToken); if (!response.HasValue || !response.Value.EndOfMessage || response.Value.MessageType != WebSocketMessageType.Text) { - return _baselineCapabilities; + throw new ApplicationException("Unable to connect to the browser refresh server."); } - var values = Encoding.UTF8.GetString(buffer.AsSpan(0, response.Value.Count)); + var capabilities = Encoding.UTF8.GetString(buffer.AsSpan(0, response.Value.Count)); - // Capabilitiies are expressed a space-separated string. + // Capabilities are expressed a space-separated string. // e.g. https://github.com/dotnet/runtime/blob/14343bdc281102bf6fffa1ecdd920221d46761bc/src/coreclr/System.Private.CoreLib/src/System/Reflection/Metadata/AssemblyExtensions.cs#L87 - var result = values.Split(' ').ToImmutableArray(); - return result; + return capabilities.Split(' ').ToImmutableArray(); } finally { @@ -80,7 +78,7 @@ public async ValueTask Apply(DotNetWatchContext context, ImmutableArray hotReloadCapabilities; + try + { + hotReloadCapabilities = await hotReloadCapabilitiesTask; + } + catch (Exception ex) + { + taskCompletionSource.TrySetException(new ApplicationException("Failed to read Hot Reload capabilities: " + ex.Message, ex)); + return; + } + + reporter.Verbose($"Hot reload capabilities: {string.Join(" ", hotReloadCapabilities)}.", emoji: "🔥"); + + var hotReloadService = new WatchHotReloadService(workspace.Services, hotReloadCapabilities); await hotReloadService.StartSessionAsync(currentSolution, cancellationToken); @@ -76,23 +88,5 @@ await Task.WhenAll( taskCompletionSource.TrySetException(ex); } } - - private static async Task> GetHotReloadCapabilitiesAsync(Task> hotReloadCapabilitiesTask, IReporter reporter) - { - try - { - var capabilities = await hotReloadCapabilitiesTask; - reporter.Verbose($"Hot reload capabilities: {string.Join(" ", capabilities)}.", emoji: "🔥"); - - return capabilities; - } - catch (Exception ex) - { - reporter.Verbose("Reading hot reload capabilities failed. Using default capabilities."); - reporter.Verbose(ex.ToString()); - - return ImmutableArray.Create("Baseline", "AddDefinitionToExistingType", "NewTypeDefinition"); - } - } } } diff --git a/src/BuiltInTools/dotnet-watch/HotReload/DefaultDeltaApplier.cs b/src/BuiltInTools/dotnet-watch/HotReload/DefaultDeltaApplier.cs index 757a5ec6c94e..9637b4248ed8 100644 --- a/src/BuiltInTools/dotnet-watch/HotReload/DefaultDeltaApplier.cs +++ b/src/BuiltInTools/dotnet-watch/HotReload/DefaultDeltaApplier.cs @@ -22,8 +22,7 @@ internal class DefaultDeltaApplier : IDeltaApplier { private static readonly string _namedPipeName = Guid.NewGuid().ToString(); private readonly IReporter _reporter; - private Task? _connectionTask; - private Task>? _capabilities; + private Task>? _capabilitiesTask; private NamedPipeServerStream? _pipe; public DefaultDeltaApplier(IReporter reporter) @@ -38,24 +37,16 @@ public ValueTask InitializeAsync(DotNetWatchContext context, CancellationToken c if (!SuppressNamedPipeForTests) { _pipe = new NamedPipeServerStream(_namedPipeName, PipeDirection.InOut, 1, PipeTransmissionMode.Byte, PipeOptions.Asynchronous | PipeOptions.CurrentUserOnly); - _connectionTask = _pipe.WaitForConnectionAsync(cancellationToken); - - _capabilities = Task.Run(async () => + _capabilitiesTask = Task.Run(async () => { - try - { - await _connectionTask; - // When the client connects, the first payload it sends is the initialization payload which includes the apply capabilities. - var capabilities = ClientInitializationPayload.Read(_pipe).Capabilities; - _reporter.Verbose($"Application supports the following capabilities {capabilities}."); - return capabilities.Split(' ').ToImmutableArray(); - } - catch - { - // Do nothing. This is awaited by Apply which will surface the error. - } - - return ImmutableArray.Empty; + _reporter.Verbose($"Connecting to the application."); + + await _pipe.WaitForConnectionAsync(cancellationToken); + + // When the client connects, the first payload it sends is the initialization payload which includes the apply capabilities. + + var capabilities = ClientInitializationPayload.Read(_pipe).Capabilities; + return capabilities.Split(' ').ToImmutableArray(); }); } @@ -72,11 +63,11 @@ public ValueTask InitializeAsync(DotNetWatchContext context, CancellationToken c } public Task> GetApplyUpdateCapabilitiesAsync(DotNetWatchContext context, CancellationToken cancellationToken) - => _capabilities ?? Task.FromResult(ImmutableArray.Empty); + => _capabilitiesTask ?? Task.FromResult(ImmutableArray.Empty); public async ValueTask Apply(DotNetWatchContext context, ImmutableArray solutionUpdate, CancellationToken cancellationToken) { - if (_connectionTask is null || !_connectionTask.IsCompletedSuccessfully || _pipe is null || !_pipe.IsConnected) + if (_capabilitiesTask is null || !_capabilitiesTask.IsCompletedSuccessfully || _pipe is null || !_pipe.IsConnected) { // The client isn't listening _reporter.Verbose("No client connected to receive delta updates.");