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

Conversation

SteveSandersonMS
Copy link
Member

For more details, see #8274 (comment)

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.

Copy link
Member

@javiercn javiercn left a comment

Choose a reason for hiding this comment

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

Looks great. Could we perhaps have an E2E test for this?

@SteveSandersonMS
Copy link
Member Author

@javiercn I don't think we have any infrastructure for E2E tests specifically about initialization, since all our E2E tests run in a context where the app has already started running. I agree it would be good if we did have more coverage around that, but it's not something I'd like to get deep into at this point.

@Eilon Eilon added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Mar 27, 2019
@@ -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.

👍

@@ -104,21 +104,32 @@ public Task<IEnumerable<string>> PrerenderComponentAsync(Type componentType, Par
});
}

public async Task InitializeAsync(CancellationToken cancellationToken)
Copy link
Member

Choose a reason for hiding this comment

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

It would be really nice to leave the signature alone and have the hub be the one to fire-and-forget this task. When we have async code we need to be able to await it in a test or errors will creep in.

Basically have a big, thick, gooey async method that returns a task, and have a small stub (StartCircuit in this case) do _ = InitializeAsync()

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, done.

@SteveSandersonMS SteveSandersonMS force-pushed the stevesa/fix-interop-during-init branch from dd7a169 to 82018fc Compare March 28, 2019 12:30
@SteveSandersonMS SteveSandersonMS force-pushed the stevesa/fix-interop-during-init branch from 82018fc to 50c31b8 Compare March 28, 2019 12:39
@SteveSandersonMS SteveSandersonMS merged commit 03357bf into master Mar 28, 2019
@SteveSandersonMS SteveSandersonMS deleted the stevesa/fix-interop-during-init branch March 28, 2019 16:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants