Skip to content

Components: don't block the SignalR loop during init. Fixes #8274 #8863

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.

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 3 commits into from
Mar 28, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
31 changes: 21 additions & 10 deletions src/Components/Server/src/Circuits/CircuitHost.cs
Original file line number Diff line number Diff line change
Expand Up @@ -108,20 +108,31 @@ public async Task InitializeAsync(CancellationToken cancellationToken)
{
await Renderer.InvokeAsync(async () =>
{
SetCurrentCircuitHost(this);

for (var i = 0; i < Descriptors.Count; i++)
try
{
var (componentType, domElementSelector) = Descriptors[i];
await Renderer.AddComponentAsync(componentType, domElementSelector);
}
SetCurrentCircuitHost(this);
_initialized = true; // We're ready to accept incoming JSInterop calls from here on

await OnCircuitOpenedAsync(cancellationToken);
await OnCircuitOpenedAsync(cancellationToken);
await OnConnectionUpAsync(cancellationToken);

await OnConnectionUpAsync(cancellationToken);
// We add the root components *after* the circuit is flagged as open.
// That's because AddComponentAsync waits for quiescence, which can take
// arbitrarily long. In the meantime we might need to be receiving and
// processing incoming JSInterop calls or similar.
for (var i = 0; i < Descriptors.Count; i++)
{
var (componentType, domElementSelector) = Descriptors[i];
await Renderer.AddComponentAsync(componentType, domElementSelector);
Copy link
Member

Choose a reason for hiding this comment

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

Do we still have to work for quiescence here until we declare the circuit opened?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, if we did that, then JSInterop calls made in the meantime (e.g., during InitAsync) would throw because they are only allowed once the circuit is flagged as opened.

We need the circuit to be regarded as opened synchronously. Since dispatcher calls are serialized, and since (non-quiescent) rendering is synchronous, we already know that we'll only start to process incoming JSInterop calls after the first synchronous render, which is the right time to start processing them.

}
}
catch (Exception ex)
{
// We have to handle all our own errors here, because the upstream caller
// has to fire-and-forget this
Renderer_UnhandledException(this, ex);
}
});

_initialized = true;
}

public async void BeginInvokeDotNetFromJS(string callId, string assemblyName, string methodIdentifier, long dotNetObjectId, string argsJson)
Expand Down
9 changes: 6 additions & 3 deletions src/Components/Server/src/ComponentHub.cs
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ public override Task OnDisconnectedAsync(Exception exception)
/// <summary>
/// Intended for framework use only. Applications should not call this method directly.
/// </summary>
public async Task<string> StartCircuit(string uriAbsolute, string baseUriAbsolute)
public string StartCircuit(string uriAbsolute, string baseUriAbsolute)
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

{
var circuitClient = new CircuitClientProxy(Clients.Caller, Context.ConnectionId);

Expand All @@ -76,8 +76,11 @@ public async Task<string> StartCircuit(string uriAbsolute, string baseUriAbsolut

circuitHost.UnhandledException += CircuitHost_UnhandledException;

// If initialization fails, this will throw. The caller will fail if they try to call into any interop API.
await circuitHost.InitializeAsync(Context.ConnectionAborted);
// Fire-and-forget the initialization process, because we can't block the
// SignalR message loop (we'd get a deadlock if any of the initialization
// logic relied on receiving a subsequent message from SignalR), and it will
// take care of its own errors anyway.
_ = circuitHost.InitializeAsync(Context.ConnectionAborted);

_circuitRegistry.Register(circuitHost);

Expand Down
41 changes: 41 additions & 0 deletions src/Components/Server/test/Circuits/CircuitHostTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text.Encodings.Web;
using System.Threading;
using System.Threading.Tasks;
Expand Down Expand Up @@ -81,6 +82,46 @@ public async Task InitializeAsync_InvokesHandlers()
handler2.VerifyAll();
}

[Fact]
public async Task InitializeAsync_ReportsOwnAsyncExceptions()
{
// Arrange
var handler = new Mock<CircuitHandler>(MockBehavior.Strict);
var tcs = new TaskCompletionSource<object>();
var reportedErrors = new List<UnhandledExceptionEventArgs>();

handler
.Setup(h => h.OnCircuitOpenedAsync(It.IsAny<Circuit>(), It.IsAny<CancellationToken>()))
.Returns(tcs.Task)
.Verifiable();

var circuitHost = TestCircuitHost.Create(handlers: new[] { handler.Object });
circuitHost.UnhandledException += (sender, errorInfo) =>
{
Assert.Same(circuitHost, sender);
reportedErrors.Add(errorInfo);
};

// Act
var initializeAsyncTask = circuitHost.InitializeAsync(new CancellationToken());

// Assert: No synchronous exceptions
handler.VerifyAll();
Assert.Empty(reportedErrors);

// Act: Trigger async exception
var ex = new InvalidTimeZoneException();
tcs.SetException(ex);

// Assert: The top-level task still succeeds, because the intended usage
// pattern is fire-and-forget.
await initializeAsyncTask;

// Assert: The async exception was reported via the side-channel
Assert.Same(ex, reportedErrors.Single().ExceptionObject);
Assert.False(reportedErrors.Single().IsTerminating);
}

[Fact]
public async Task DisposeAsync_InvokesCircuitHandler()
{
Expand Down