Skip to content
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

Jump to bottom

Introduce CircuitHandler to handle circuit lifetime events #6971

Merged
merged 5 commits into from
Feb 4, 2019
Merged

Introduce CircuitHandler to handle circuit lifetime events #6971

merged 5 commits into from
Feb 4, 2019

Conversation

pranavkm
Copy link
Contributor

Partial fix to #6353

rynowak
rynowak reviewed Jan 24, 2019
View reviewed changes
Copy link
Member

@rynowak rynowak left a comment

Choose a reason for hiding this comment

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

This seems super on the right track.

@Eilon Eilon added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Jan 25, 2019
@javiercn javiercn self-requested a review January 25, 2019 18:09
@pranavkm pranavkm changed the title [Design]: Introduce CircuitHandler to handle circuit lifetime events Introduce CircuitHandler to handle circuit lifetime events Jan 25, 2019
@pranavkm
Copy link
Contributor Author

🆙 📅

rynowak
rynowak previously requested changes Jan 25, 2019
View reviewed changes
Copy link
Member

@rynowak rynowak left a comment

Choose a reason for hiding this comment

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

https://github.com/aspnet/AspNetCore/pull/6971/files#r251153473

@Tratcher
Copy link
Member

Weird, I don't see why I was triggered as a code owner.

@Tratcher Tratcher removed their request for review January 25, 2019 23:04
rynowak
rynowak reviewed Jan 26, 2019
View reviewed changes
eng/Versions.props Outdated Show resolved Hide resolved
@pranavkm pranavkm changed the base branch from prkrishn/some-cleanup to master January 29, 2019 23:30
@pranavkm
Copy link
Contributor Author

🆙 📅

SteveSandersonMS
SteveSandersonMS reviewed Feb 1, 2019
View reviewed changes
src/Components/Server/src/Circuits/CircuitHandler.cs Outdated
/// Invoked when a new circuit was established.
/// </summary>
/// <param name="circuit">The <see cref="Circuit"/>.</param>
/// <param name="cancellationToken">The <see cref="CancellationToken"/>.</param>
Copy link
Member

Choose a reason for hiding this comment

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

Might be nice to specify what this particular CancellationToken represents. What operation does it represent the cancellation of? Also in the other methods in this class.

SteveSandersonMS
SteveSandersonMS reviewed Feb 1, 2019
View reviewed changes
src/Components/Server/src/Circuits/CircuitHost.cs

Renderer.UnhandledException += Renderer_UnhandledException;
SynchronizationContext.UnhandledException += SynchronizationContext_UnhandledException;
}

public string CircuitId { get; } = Guid.NewGuid().ToString();
Copy link
Member

Choose a reason for hiding this comment

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

Are there any security concerns here if this is guessable? Might a third party be able to "reconnect" to someone else's circuit and obtain their state? If so we might need to generate this value using RNGCryptoServiceProvider or similar.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tracking via #7243

SteveSandersonMS
SteveSandersonMS reviewed Feb 1, 2019
View reviewed changes
src/Components/Server/src/Circuits/CircuitHost.cs
{
for (var i = 0; i < _circuitHandlers.Length; i++)
{
await _circuitHandlers[i].OnConnectionDownAsync(Circuit, default);
Copy link
Member

Choose a reason for hiding this comment

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

It's fine to keep it simple for now, but later we might want to pass some flag to OnConnectionDownAsync to indicate that we're "disposing" deliberately, rather than encountering an unexpected connection loss. People may want to log unexpected connection loss, or perhaps skip the "serialize state" job if it's a deliberate shutdown.

SteveSandersonMS
SteveSandersonMS reviewed Feb 1, 2019
View reviewed changes
src/Components/Server/src/ComponentsHub.cs Outdated
@@ -13,9 +13,9 @@
namespace Microsoft.AspNetCore.Components.Server
{
/// <summary>
/// A SignalR hub that accepts connections to a Server-Side Blazor app.
/// A SignalR hub that accepts connections to a ASP.NET Core Components WebApp.
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: WebApp is an odd term. Maybe web application would be more consistent with usage elsewhere :)

Also we usually say "an ASP.NET Core..." rather than "a ASP.NET Core...". Minor details though, I know!

SteveSandersonMS
SteveSandersonMS approved these changes Feb 1, 2019
View reviewed changes
Copy link
Member

@SteveSandersonMS SteveSandersonMS left a comment

Choose a reason for hiding this comment

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

This looks superb to me.

@jkotalik jkotalik removed their request for review February 1, 2019 17:58
javiercn
javiercn reviewed Feb 4, 2019
View reviewed changes
src/Components/Server/src/Circuits/Circuit.cs
/// </summary>
public class Circuit
public sealed class Circuit
Copy link
Member

Choose a reason for hiding this comment

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

What's the purpose of this class? It only has a single ID and a _circuitHost that is not accessible outside this class. Can't the Id be moved to the CircuitHost class and this class be removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I expect Circuit to be the user facing type. CircuitHost is an implementation detail.

javiercn
javiercn reviewed Feb 4, 2019
View reviewed changes
src/Components/Server/src/Circuits/CircuitHost.cs
{
Scope = scope ?? throw new ArgumentNullException(nameof(scope));
_scope = scope ?? throw new ArgumentNullException(nameof(scope));
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 at some point we should do cleanup of our DI usage across circuits and Components. We shouldn't be passing DI abstractions around unless the specific class is a composition root.

Copy link
Member

Choose a reason for hiding this comment

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

The circuit host is the owner of the scope which is why it's here.

@pranavkm pranavkm mentioned this pull request Feb 4, 2019
Closed
@pranavkm pranavkm dismissed rynowak’s stale review February 4, 2019 17:50

Unavailable to complete review

@pranavkm pranavkm merged commit 2e54d64 into master Feb 4, 2019
@pranavkm pranavkm deleted the prkrishn/circuits branch February 4, 2019 18:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rynowak rynowak rynowak left review comments

@natemcmaster natemcmaster natemcmaster left review comments

@SteveSandersonMS SteveSandersonMS SteveSandersonMS approved these changes

@javiercn javiercn javiercn approved these changes

Assignees
No one assigned
Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@pranavkm @Tratcher @SteveSandersonMS @rynowak @natemcmaster @javiercn @Eilon