From 7a0a286ce6ff5945e690c9747079e61d4e6f31d8 Mon Sep 17 00:00:00 2001 From: Javier Calvarro Nelson Date: Wed, 31 Jul 2019 13:17:00 +0200 Subject: [PATCH] [Blazor][Fixes #12197] Dispose the circuit on graceful disconnections (#12449) * Immediately releases the circuit when the client disconnects gracefully. * This functionality is limited to websockets. * We are able to release the circuit in the following situations: * The user closes the browser. * The user navigates away. * The user reloads the page. --- .../Server/src/Circuits/CircuitRegistry.cs | 19 +++ src/Components/Server/src/ComponentHub.cs | 31 ++++- .../ToggleExecutionModeServerFixture.cs | 15 ++- .../CircuitGracefulTerminationTests.cs | 113 ++++++++++++++++++ src/Shared/E2ETesting/BrowserTestBase.cs | 9 +- 5 files changed, 182 insertions(+), 5 deletions(-) create mode 100644 src/Components/test/E2ETest/ServerExecutionTests/CircuitGracefulTerminationTests.cs diff --git a/src/Components/Server/src/Circuits/CircuitRegistry.cs b/src/Components/Server/src/Circuits/CircuitRegistry.cs index 091d993548..9fe25c5b68 100644 --- a/src/Components/Server/src/Circuits/CircuitRegistry.cs +++ b/src/Components/Server/src/Circuits/CircuitRegistry.cs @@ -81,6 +81,15 @@ namespace Microsoft.AspNetCore.Components.Server.Circuits } } + public void PermanentDisconnect(CircuitHost circuitHost) + { + if (ConnectedCircuits.TryRemove(circuitHost.CircuitId, out _)) + { + Log.CircuitDisconnectedPermanently(_logger, circuitHost.CircuitId); + circuitHost.Client.SetDisconnected(); + } + } + public virtual Task DisconnectAsync(CircuitHost circuitHost, string connectionId) { Log.CircuitDisconnectStarted(_logger, circuitHost.CircuitId, connectionId); @@ -314,6 +323,7 @@ namespace Microsoft.AspNetCore.Components.Server.Circuits private static readonly Action _circuitNotActive; private static readonly Action _circuitConnectedToDifferentConnection; private static readonly Action _circuitMarkedDisconnected; + private static readonly Action _circuitDisconnectedPermanently; private static readonly Action _circuitEvicted; private static class EventIds @@ -330,6 +340,7 @@ namespace Microsoft.AspNetCore.Components.Server.Circuits public static readonly EventId CircuitConnectedToDifferentConnection = new EventId(109, "CircuitConnectedToDifferentConnection"); public static readonly EventId CircuitMarkedDisconnected = new EventId(110, "CircuitMarkedDisconnected"); public static readonly EventId CircuitEvicted = new EventId(111, "CircuitEvicted"); + public static readonly EventId CircuitDisconnectedPermanently = new EventId(112, "CircuitDisconnectedPermanently"); } static Log() @@ -394,6 +405,11 @@ namespace Microsoft.AspNetCore.Components.Server.Circuits EventIds.CircuitMarkedDisconnected, "Circuit with id {CircuitId} is disconnected."); + _circuitDisconnectedPermanently = LoggerMessage.Define( + LogLevel.Debug, + EventIds.CircuitDisconnectedPermanently, + "Circuit with id {CircuitId} has been removed from the registry for permanent disconnection."); + _circuitEvicted = LoggerMessage.Define( LogLevel.Debug, EventIds.CircuitEvicted, @@ -436,6 +452,9 @@ namespace Microsoft.AspNetCore.Components.Server.Circuits public static void CircuitMarkedDisconnected(ILogger logger, string circuitId) => _circuitMarkedDisconnected(logger, circuitId, null); + public static void CircuitDisconnectedPermanently(ILogger logger, string circuitId) => + _circuitDisconnectedPermanently(logger, circuitId, null); + public static void CircuitEvicted(ILogger logger, string circuitId, EvictionReason evictionReason) => _circuitEvicted(logger, circuitId, evictionReason, null); } diff --git a/src/Components/Server/src/ComponentHub.cs b/src/Components/Server/src/ComponentHub.cs index ed796e73a1..b4d0d061a2 100644 --- a/src/Components/Server/src/ComponentHub.cs +++ b/src/Components/Server/src/ComponentHub.cs @@ -66,7 +66,32 @@ namespace Microsoft.AspNetCore.Components.Server } CircuitHost = null; - return _circuitRegistry.DisconnectAsync(circuitHost, Context.ConnectionId); + if (exception != null) + { + return _circuitRegistry.DisconnectAsync(circuitHost, Context.ConnectionId); + } + else + { + // The client will gracefully disconnect when using websockets by correctly closing the TCP connection. + // This happens when the user closes a tab, navigates away from the page or reloads the page. + // In these situations we know the user is done with the circuit, so we can get rid of it at that point. + // This is important to be able to more efficiently manage resources, specially memory. + return TerminateCircuitGracefully(circuitHost); + } + } + + private async Task TerminateCircuitGracefully(CircuitHost circuitHost) + { + try + { + Log.CircuitTerminatedGracefully(_logger, circuitHost.CircuitId); + _circuitRegistry.PermanentDisconnect(circuitHost); + await circuitHost.DisposeAsync(); + } + catch (Exception e) + { + Log.UnhandledExceptionInCircuit(_logger, circuitHost.CircuitId, e); + } } /// @@ -248,6 +273,8 @@ namespace Microsoft.AspNetCore.Components.Server private static readonly Action _circuitHostNotInitialized = LoggerMessage.Define(LogLevel.Debug, new EventId(6, "CircuitHostNotInitialized"), "Call to '{CallSite}' received before the circuit host initialization."); + private static readonly Action _circuitTerminatedGracefully = + LoggerMessage.Define(LogLevel.Debug, new EventId(7, "CircuitTerminatedGracefully"), "Circuit '{CircuitId}' terminated gracefully."); public static void NoComponentsRegisteredInEndpoint(ILogger logger, string endpointDisplayName) { @@ -272,6 +299,8 @@ namespace Microsoft.AspNetCore.Components.Server public static void CircuitAlreadyInitialized(ILogger logger, string circuitId) => _circuitAlreadyInitialized(logger, circuitId, null); public static void CircuitHostNotInitialized(ILogger logger, [CallerMemberName] string callSite = "") => _circuitHostNotInitialized(logger, callSite, null); + + public static void CircuitTerminatedGracefully(ILogger logger, string circuitId) => _circuitTerminatedGracefully(logger, circuitId, null); } } } diff --git a/src/Components/test/E2ETest/Infrastructure/ServerFixtures/ToggleExecutionModeServerFixture.cs b/src/Components/test/E2ETest/Infrastructure/ServerFixtures/ToggleExecutionModeServerFixture.cs index 56be3c9256..cda7c50420 100644 --- a/src/Components/test/E2ETest/Infrastructure/ServerFixtures/ToggleExecutionModeServerFixture.cs +++ b/src/Components/test/E2ETest/Infrastructure/ServerFixtures/ToggleExecutionModeServerFixture.cs @@ -3,6 +3,7 @@ using System; using System.Collections.Generic; +using Microsoft.AspNetCore.Hosting; namespace Microsoft.AspNetCore.Components.E2ETest.Infrastructure.ServerFixtures { @@ -11,6 +12,8 @@ namespace Microsoft.AspNetCore.Components.E2ETest.Infrastructure.ServerFixtures { public string PathBase { get; set; } + public IWebHost Host { get; set; } + public ExecutionMode ExecutionMode { get; set; } = ExecutionMode.Client; private AspNetSiteServerFixture.BuildWebHost _buildWebHostMethod; @@ -32,7 +35,11 @@ namespace Microsoft.AspNetCore.Components.E2ETest.Infrastructure.ServerFixtures var underlying = new DevHostServerFixture(); underlying.PathBase = PathBase; _serverToDispose = underlying; - return underlying.RootUri.AbsoluteUri; + var uri = underlying.RootUri.AbsoluteUri; // As a side-effect, this starts the server + + Host = underlying.Host; + + return uri; } else { @@ -41,7 +48,11 @@ namespace Microsoft.AspNetCore.Components.E2ETest.Infrastructure.ServerFixtures underlying.AdditionalArguments.AddRange(AspNetFixtureAdditionalArguments); underlying.BuildWebHostMethod = _buildWebHostMethod; _serverToDispose = underlying; - return underlying.RootUri.AbsoluteUri; + var uri = underlying.RootUri.AbsoluteUri; // As a side-effect, this starts the server + + Host = underlying.Host; + + return uri; } } diff --git a/src/Components/test/E2ETest/ServerExecutionTests/CircuitGracefulTerminationTests.cs b/src/Components/test/E2ETest/ServerExecutionTests/CircuitGracefulTerminationTests.cs new file mode 100644 index 0000000000..970f9a9ae0 --- /dev/null +++ b/src/Components/test/E2ETest/ServerExecutionTests/CircuitGracefulTerminationTests.cs @@ -0,0 +1,113 @@ +// Copyright (c) .NET Foundation. 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.Collections.Generic; +using System.Threading.Tasks; +using BasicTestApp; +using Castle.DynamicProxy.Contributors; +using Microsoft.AspNetCore.Components.E2ETest.Infrastructure; +using Microsoft.AspNetCore.Components.E2ETest.Infrastructure.ServerFixtures; +using Microsoft.AspNetCore.Components.E2ETest.ServerExecutionTests; +using Microsoft.AspNetCore.E2ETesting; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Logging.Testing; +using OpenQA.Selenium; +using Xunit; +using Xunit.Abstractions; + +namespace Microsoft.AspNetCore.Components.E2ETests.ServerExecutionTests +{ + public class CircuitGracefulTerminationTests : BasicTestAppTestBase, IDisposable + { + public CircuitGracefulTerminationTests( + BrowserFixture browserFixture, + ToggleExecutionModeServerFixture serverFixture, + ITestOutputHelper output) + : base(browserFixture, serverFixture.WithServerExecution(), output) + { + } + + public TaskCompletionSource GracefulDisconnectCompletionSource { get; private set; } + public TestSink Sink { get; private set; } + public List<(Extensions.Logging.LogLevel level, string eventIdName)> Messages { get; private set; } + + public override async Task InitializeAsync() + { + // These tests manipulate the browser in ways that make it impossible to use the same browser + // instance across tests (One of the tests closes the browser). For that reason we simply create + // a new browser instance for every test in this class sos that there are no issues when running + // them together. + await base.InitializeAsync(Guid.NewGuid().ToString()); + } + + protected override void InitializeAsyncCore() + { + Navigate(ServerPathBase, noReload: false); + MountTestComponent(); + Browser.Equal("Current count: 0", () => Browser.FindElement(By.TagName("p")).Text); + + GracefulDisconnectCompletionSource = new TaskCompletionSource(TaskContinuationOptions.RunContinuationsAsynchronously); + Sink = _serverFixture.Host.Services.GetRequiredService(); + Messages = new List<(Extensions.Logging.LogLevel level, string eventIdName)>(); + Sink.MessageLogged += Log; + } + + [Fact] + public async Task ReloadingThePage_GracefullyDisconnects_TheCurrentCircuit() + { + // Arrange & Act + _ = ((IJavaScriptExecutor)Browser).ExecuteScript("location.reload()"); + await Task.WhenAny(Task.Delay(10000), GracefulDisconnectCompletionSource.Task); + + // Assert + Assert.Contains((Extensions.Logging.LogLevel.Debug, "CircuitTerminatedGracefully"), Messages); + Assert.Contains((Extensions.Logging.LogLevel.Debug, "CircuitDisconnectedPermanently"), Messages); + } + + [Fact] + public async Task ClosingTheBrowserWindow_GracefullyDisconnects_TheCurrentCircuit() + { + // Arrange & Act + Browser.Close(); + // Set to null so that other tests in this class can create a new browser if necessary so + // that tests don't fail when running together. + Browser = null; + + await Task.WhenAny(Task.Delay(10000), GracefulDisconnectCompletionSource.Task); + + // Assert + Assert.Contains((Extensions.Logging.LogLevel.Debug, "CircuitTerminatedGracefully"), Messages); + Assert.Contains((Extensions.Logging.LogLevel.Debug, "CircuitDisconnectedPermanently"), Messages); + } + + [Fact] + public async Task ClosingTheBrowserWindow_GracefullyDisconnects_WhenNavigatingAwayFromThePage() + { + // Arrange & Act + Browser.Navigate().GoToUrl("about:blank"); + await Task.WhenAny(Task.Delay(10000), GracefulDisconnectCompletionSource.Task); + + // Assert + Assert.Contains((Extensions.Logging.LogLevel.Debug, "CircuitTerminatedGracefully"), Messages); + Assert.Contains((Extensions.Logging.LogLevel.Debug, "CircuitDisconnectedPermanently"), Messages); + } + + private void Log(WriteContext wc) + { + if ((Extensions.Logging.LogLevel.Debug, "CircuitTerminatedGracefully") == (wc.LogLevel, wc.EventId.Name)) + { + GracefulDisconnectCompletionSource.TrySetResult(null); + } + Messages.Add((wc.LogLevel, wc.EventId.Name)); + } + + public void Dispose() + { + if (Sink != null) + { + Sink.MessageLogged -= Log; + } + } + } +} diff --git a/src/Shared/E2ETesting/BrowserTestBase.cs b/src/Shared/E2ETesting/BrowserTestBase.cs index 509501a4f5..bcec5ae2e3 100644 --- a/src/Shared/E2ETesting/BrowserTestBase.cs +++ b/src/Shared/E2ETesting/BrowserTestBase.cs @@ -37,9 +37,14 @@ namespace Microsoft.AspNetCore.E2ETesting return Task.CompletedTask; } - public virtual async Task InitializeAsync() + public virtual Task InitializeAsync() { - var (browser, logs) = await BrowserFixture.GetOrCreateBrowserAsync(Output); + return InitializeAsync(""); + } + + public virtual async Task InitializeAsync(string isolationContext) + { + var (browser, logs) = await BrowserFixture.GetOrCreateBrowserAsync(Output, isolationContext); _asyncBrowser.Value = browser; _logs.Value = logs;