Skip to content

Ensure DI scope is disposed #11894

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jul 5, 2019
Merged

Ensure DI scope is disposed #11894

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f71a19d
Ensure CircuitHost disposes DI scope even if other disposals throw
SteveSandersonMS Jul 4, 2019
4671efa
Ensure DI scope is disposed if prerendering fails
SteveSandersonMS Jul 4, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 10 additions & 4 deletions src/Components/Server/src/Circuits/CircuitHost.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -293,10 +293,16 @@ public async ValueTask DisposeAsync()

await Renderer.InvokeAsync(async () =>
{
await OnConnectionDownAsync(CancellationToken.None);
await OnCircuitDownAsync();
Renderer.Dispose();
_scope.Dispose();
try
{
await OnConnectionDownAsync(CancellationToken.None);
await OnCircuitDownAsync();
}
finally
{
Renderer.Dispose();
_scope.Dispose();
}
});
}

Expand Down
45 changes: 25 additions & 20 deletions src/Components/Server/src/Circuits/CircuitPrerenderer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,13 @@
using Microsoft.AspNetCore.Components.Rendering;
using Microsoft.AspNetCore.Http;
using Microsoft.AspNetCore.Http.Extensions;
using Microsoft.AspNetCore.Http.Features;

namespace Microsoft.AspNetCore.Components.Server.Circuits
{
internal class CircuitPrerenderer : IComponentPrerenderer
{
private static object CircuitHostKey = new object();
private static object NavigationStatusKey = new object();
private static object CancellationStatusKey = new object();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most of the noise in the diff here is just because I renamed the concept of NavigationStatus to CancellationStatus.

The code was never really interested in the concept of "did we navigate or not" - it was only interested in "should the circuit continue to exist", so this is a more precise name. It's relevant to change this now because there's now a second case where the circuit should not continue to exist (i.e., some component threw during prerendering, so we gave up on prerendering and released the DI scope immediately).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(i.e., some component threw during prerendering, so we gave up on prerendering and released the DI scope immediately).

I'm not sure what the E2E behavior is, but when multiple components get prerendered at the same time they share the same circuit, so isn't it bad to prevent other prerendered components from working? (limiting the error to the current prerendered component)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When a component throws an unhandled exception in prerendering, we return a 500 error to the browser, not a partially-complete HTML response.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still have some doubts about this because people could write a try catch for these scenarios to keep other components in the page working and we are taking away that option, I'm not sure if that is something we want to consider.

(For example, don't dispose the circuit if some other component rendered successfully and only do the cleanup for the circuit on response started if we got a 500 out)

Copy link
Member Author

@SteveSandersonMS SteveSandersonMS Jul 4, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good question to raise, but it makes me think maybe we shouldn't be putting all prerendered components into the same circuit, or should give some other way to control how they are grouped into circuits so those groups can fail independently.

This is because in non-prerendering cases, we don't allow circuits/renderers/etc to continue to exist and be used once they have got into an undefined state through an unhandled exception during rendering or lifecycle methods. So in that sense it's good that we prevent it for prerendering too, and don't allow people to try/catch and then continue using that circuit state.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's a fair answer, given that we are moving to a single circuit per connection model (independent of whether its prerendered or not)

I think that makes sense and leaves things in a more manageable state, so I'm ok with it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should work hard to make life better when something is crashing. We should make sure that it fails hard, if users have multiple components on a page and they want to degrade gracefully they need to do it in their own code.

So I agree with the conclusion here.


private readonly CircuitFactory _circuitFactory;
private readonly CircuitRegistry _registry;
Expand All @@ -29,15 +28,15 @@ public CircuitPrerenderer(CircuitFactory circuitFactory, CircuitRegistry registr
public async Task<ComponentPrerenderResult> PrerenderComponentAsync(ComponentPrerenderingContext prerenderingContext)
{
var context = prerenderingContext.Context;
var navigationStatus = GetOrCreateNavigationStatus(context);
if (navigationStatus.Navigated)
var cancellationStatus = GetOrCreateCancellationStatus(context);
if (cancellationStatus.Canceled)
{
// Avoid creating a circuit host if other component earlier in the pipeline already triggered
// a navigation request. Instead rendre nothing
// cancelation (e.g., by navigating or throwing). Instead render nothing.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit, mixture of cancellation and cancelation

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good spot. I'll fix the comment typo in an unrelated PR just so I don't have to wait for CI checks here again.

return new ComponentPrerenderResult(Array.Empty<string>());
}
var circuitHost = GetOrCreateCircuitHost(context, navigationStatus);
ComponentRenderedText renderResult = default;
var circuitHost = GetOrCreateCircuitHost(context, cancellationStatus);
ComponentRenderedText renderResult;
try
{
renderResult = await circuitHost.PrerenderComponentAsync(
Expand All @@ -48,7 +47,7 @@ public async Task<ComponentPrerenderResult> PrerenderComponentAsync(ComponentPre
{
// Cleanup the state as we won't need it any longer.
// Signal callbacks that we don't have to register the circuit.
await CleanupCircuitState(context, navigationStatus, circuitHost);
await CleanupCircuitState(context, cancellationStatus, circuitHost);

// Navigation was attempted during prerendering.
if (prerenderingContext.Context.Response.HasStarted)
Expand All @@ -64,6 +63,12 @@ public async Task<ComponentPrerenderResult> PrerenderComponentAsync(ComponentPre
context.Response.Redirect(navigationException.Location);
return new ComponentPrerenderResult(Array.Empty<string>());
}
catch
{
// If prerendering any component fails, cancel prerendering entirely and dispose the DI scope
await CleanupCircuitState(context, cancellationStatus, circuitHost);
throw;
}

circuitHost.Descriptors.Add(new ComponentDescriptor
{
Expand All @@ -81,28 +86,28 @@ public async Task<ComponentPrerenderResult> PrerenderComponentAsync(ComponentPre
return new ComponentPrerenderResult(result);
}

private CircuitNavigationStatus GetOrCreateNavigationStatus(HttpContext context)
private PrerenderingCancellationStatus GetOrCreateCancellationStatus(HttpContext context)
{
if (context.Items.TryGetValue(NavigationStatusKey, out var existingHost))
if (context.Items.TryGetValue(CancellationStatusKey, out var existingValue))
{
return (CircuitNavigationStatus)existingHost;
return (PrerenderingCancellationStatus)existingValue;
}
else
{
var navigationStatus = new CircuitNavigationStatus();
context.Items[NavigationStatusKey] = navigationStatus;
return navigationStatus;
var cancellationStatus = new PrerenderingCancellationStatus();
context.Items[CancellationStatusKey] = cancellationStatus;
return cancellationStatus;
}
}

private static async Task CleanupCircuitState(HttpContext context, CircuitNavigationStatus navigationStatus, CircuitHost circuitHost)
private static async Task CleanupCircuitState(HttpContext context, PrerenderingCancellationStatus cancellationStatus, CircuitHost circuitHost)
{
navigationStatus.Navigated = true;
cancellationStatus.Canceled = true;
context.Items.Remove(CircuitHostKey);
await circuitHost.DisposeAsync();
}

private CircuitHost GetOrCreateCircuitHost(HttpContext context, CircuitNavigationStatus navigationStatus)
private CircuitHost GetOrCreateCircuitHost(HttpContext context, PrerenderingCancellationStatus cancellationStatus)
{
if (context.Items.TryGetValue(CircuitHostKey, out var existingHost))
{
Expand All @@ -120,7 +125,7 @@ private CircuitHost GetOrCreateCircuitHost(HttpContext context, CircuitNavigatio
context.Response.OnCompleted(() =>
{
result.UnhandledException -= CircuitHost_UnhandledException;
if (!navigationStatus.Navigated)
if (!cancellationStatus.Canceled)
{
_registry.RegisterDisconnectedCircuit(result);
}
Expand Down Expand Up @@ -164,9 +169,9 @@ private string GetFullBaseUri(HttpRequest request)
return result;
}

private class CircuitNavigationStatus
private class PrerenderingCancellationStatus
{
public bool Navigated { get; set; }
public bool Canceled { get; set; }
}
}
}
47 changes: 47 additions & 0 deletions src/Components/Server/test/Circuits/CircuitHostTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
using System;
using System.Collections.Generic;
using System.Linq;
using System.Reflection;
using System.Text.Encodings.Web;
using System.Threading;
using System.Threading.Tasks;
Expand Down Expand Up @@ -40,6 +41,37 @@ public async Task DisposeAsync_DisposesResources()
Assert.True(remoteRenderer.Disposed);
}

[Fact]
public async Task DisposeAsync_DisposesResourcesEvenIfCircuitHandlerOrComponentThrows()
{
// Arrange
var serviceScope = new Mock<IServiceScope>();
var handler = new Mock<CircuitHandler>();
handler
.Setup(h => h.OnCircuitClosedAsync(It.IsAny<Circuit>(), It.IsAny<CancellationToken>()))
.Throws<InvalidTimeZoneException>();
var remoteRenderer = GetRemoteRenderer(Renderer.CreateDefaultDispatcher());
var circuitHost = TestCircuitHost.Create(
Guid.NewGuid().ToString(),
serviceScope.Object,
remoteRenderer,
handlers: new[] { handler.Object });

var throwOnDisposeComponent = new ThrowOnDisposeComponent();
circuitHost.Renderer.AssignRootComponentId(throwOnDisposeComponent);

// Act
await Assert.ThrowsAsync<InvalidTimeZoneException>(async () =>
{
await circuitHost.DisposeAsync();
});

// Assert
Assert.True(throwOnDisposeComponent.DidCallDispose);
serviceScope.Verify(scope => scope.Dispose(), Times.Once());
Assert.True(remoteRenderer.Disposed);
}

[Fact]
public async Task DisposeAsync_DisposesRendererWithinSynchronizationContext()
{
Expand Down Expand Up @@ -239,5 +271,20 @@ public void Dispose()
Assert.Same(Dispatcher, SynchronizationContext.Current);
}
}

private class ThrowOnDisposeComponent : IComponent, IDisposable
{
public bool DidCallDispose { get; private set; }
public void Configure(RenderHandle renderHandle) { }

public Task SetParametersAsync(ParameterCollection parameters)
=> Task.CompletedTask;

public void Dispose()
{
DidCallDispose = true;
throw new InvalidFilterCriteriaException();
}
}
}
}
46 changes: 46 additions & 0 deletions src/Components/Server/test/Circuits/CircuitPrerendererTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,32 @@ public async Task ExtractsUriFromHttpContext_NonemptyPathBase()
}), GetUnwrappedContent(result));
}

[Fact]
public async Task DisposesCircuitScopeEvenIfPrerenderingThrows()
{
// Arrange
var circuitFactory = new MockServiceScopeCircuitFactory();
var circuitRegistry = new CircuitRegistry(
Options.Create(new CircuitOptions()),
Mock.Of<ILogger<CircuitRegistry>>(),
TestCircuitIdFactory.CreateTestFactory());
var httpContext = new DefaultHttpContext();
var prerenderer = new CircuitPrerenderer(circuitFactory, circuitRegistry);
var prerenderingContext = new ComponentPrerenderingContext
{
ComponentType = typeof(ThrowExceptionComponent),
Parameters = ParameterCollection.Empty,
Context = httpContext
};

// Act
await Assert.ThrowsAsync<InvalidTimeZoneException>(async () =>
await prerenderer.PrerenderComponentAsync(prerenderingContext));

// Assert
circuitFactory.MockServiceScope.Verify(scope => scope.Dispose(), Times.Once());
}

class TestCircuitFactory : CircuitFactory
{
public override CircuitHost CreateCircuitHost(HttpContext httpContext, CircuitClientProxy client, string uriAbsolute, string baseUriAbsolute)
Expand All @@ -127,6 +153,17 @@ public override CircuitHost CreateCircuitHost(HttpContext httpContext, CircuitCl
}
}

class MockServiceScopeCircuitFactory : CircuitFactory
{
public Mock<IServiceScope> MockServiceScope { get; }
= new Mock<IServiceScope>();

public override CircuitHost CreateCircuitHost(HttpContext httpContext, CircuitClientProxy client, string uriAbsolute, string baseUriAbsolute)
{
return TestCircuitHost.Create(Guid.NewGuid().ToString(), MockServiceScope.Object);
}
}

class UriDisplayComponent : IComponent
{
private RenderHandle _renderHandle;
Expand All @@ -151,5 +188,14 @@ public Task SetParametersAsync(ParameterCollection parameters)
return Task.CompletedTask;
}
}

class ThrowExceptionComponent : IComponent
{
public void Configure(RenderHandle renderHandle)
=> throw new InvalidTimeZoneException();

public Task SetParametersAsync(ParameterCollection parameters)
=> Task.CompletedTask;
}
}
}