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.

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

Refactor server-side blazor startup to allow Azure SignalR. Fixes #1227 #1396

Merged
merged 9 commits into from Sep 11, 2018

Conversation

Projects
None yet
4 participants
@SteveSandersonMS
Copy link
Member

SteveSandersonMS commented Sep 7, 2018

Fixes #1227

The main challenge with this is:
(1) We don't want to take a strict dependency on the Microsoft.Azure.SignalR package (at least not for everyone)
(2) The Microsoft.Azure.SignalR package uses its own custom equivalent to MapHub that looks the same as the standard Microsoft.AspNetCore.SignalR one, but actually uses different types. So if we comply with (1), then we can't possibly call MapHub on the developer's behalf.

Possible resolutions, then:

  • A: Use reflection tricks and make it more magic (no thanks)
  • B: Have a separate Microsoft.AspNetCore.Blazor.AzureSignalR package that depends on Microsoft.Azure.SignalR
  • C: Let the developer call MapHub themselves, meaning that (a) we have to remove all the other magic from UseServerSideBlazor and (b) we have to make BlazorHub public.

Between B and C, I prefer option C because it makes us less tightly coupled to SignalR APIs, so if SignalR adds more overloads to MapHub or other config styles in the future, we should automatically be composable with it.

This PR implements options C.

Result

The result is that app.UseServerSideBlazor<TClient>(); is now exactly equivalent to:

app.UseSignalR(route => route.MapHub<BlazorHub>(BlazorHub.DefaultPath));
app.UseBlazor<TClient>();

So, if a developer wants, they can choose not to call UseServerSideBlazor, but rather use those two lines instead, and from there they can replace UseSignalR with UseAzureSignalR if they want.

Therefore the way to use Azure SignalR with a server-side Blazor app becomes (in the server app's Startup.cs):

    public class Startup
    {
        public void ConfigureServices(IServiceCollection services)
        {
            services.AddSignalR().AddAzureSignalR(/* optionsOrConnectionString */);
            services.AddServerSideBlazor<Client.Startup>();
            // ...
        }

        public void Configure(IApplicationBuilder app, IHostingEnvironment env)
        {
            app.UseAzureSignalR(route => route.MapHub<BlazorHub>(BlazorHub.DefaultPath));
            app.UseBlazor<Client.Startup>();
            // ...
        }
    }

Note: This means that AddSignalR is being called twice - once by the developer manually, and once inside AddServerSideBlazor. @anurse, do you know if there's any problem or disadvantage with calling that twice? We can stop it if we absolutely must, but it will be easier for developers to understand this way (because it works both with and without Azure SignalR with a minimum of user code changes).

More notes, mainly for @rynowak

I moved all the "register startup actions" logic out of UseServerSideBlazor and into AddServerSideBlazor (so that UseServerSideBlazor can be equivalent to the two lines above). I believe the net result is the same, and it still can be extended to support registering multiple server-side Blazor endpoints in the future (with different TClient values), but for now I still left it hard-coded to use /_blazor as the path without implementing support for configuring an alternate path.

public override Task OnDisconnectedAsync(Exception exception)
{
CircuitHost.Dispose();
return base.OnDisconnectedAsync(exception);
}

/// <summary>
/// Invoked by framework code. Not intended for use by applications.
/// </summary>
public async Task StartCircuit(string uriAbsolute, string baseUriAbsolute)

This comment has been minimized.

@SteveSandersonMS

SteveSandersonMS Sep 7, 2018

Author Member

This (and the one below) has to be public because it's called via JS interop.

This comment has been minimized.

@anurse

anurse Sep 10, 2018

Member

We could probably put in some logic to allow exposing internal methods via an attribute ([HubMethod]). We just use reflection anyway so nothing stops us from allowing non-public methods except our own discovery choices. Thoughts @davidfowl ? Does MVC have anything equivalent for non-public Actions?

This comment has been minimized.

@SteveSandersonMS

SteveSandersonMS Sep 11, 2018

Author Member

@anurse Interesting. We've had the same debates about Blazor JS interop and whether it should match nonpublic methods. Currently our [JSInvokable] annotation only works on public, but there's an argument for changing that.

If you choose to make some kind of nonpublic-matching [HubMethod] in the future I'm sure we'd change to it here. For now it's OK though.

This comment has been minimized.

@davidfowl

davidfowl Sep 11, 2018

Member

Yah, it's fine. I don't think we should do anything here right now.

/// <param name="app">The <see cref="IApplicationBuilder"/>.</param>
/// <param name="options">Options to configure the middleware.</param>
/// <returns>The <see cref="IApplicationBuilder"/>.</returns>
public static IApplicationBuilder UseBlazor(
this IApplicationBuilder app,
BlazorOptions options)

This comment has been minimized.

@SteveSandersonMS

SteveSandersonMS Sep 7, 2018

Author Member

At some point we should replace this with Action<BlazorOptions> configure, but it's independent of this PR.

This comment has been minimized.

@rynowak

rynowak Sep 9, 2018

Member

Usually with middleware we just accept the options as-is becuase the options is constructed by the user, and not from DI. UseBlazor() would use the options from DI.

AddBlazor(...) would take an action, since that's going to compose with DI.

This comment has been minimized.

@SteveSandersonMS

SteveSandersonMS Sep 11, 2018

Author Member

OK, sounds fine.

public override CircuitHost CreateCircuitHost(HttpContext httpContext, IClientProxy client)
{
if (!StartupActions.TryGetValue(httpContext.Request.Path, out var config))
if (!_options.StartupActions.TryGetValue(httpContext.Request.Path, out var config))

This comment has been minimized.

@SteveSandersonMS

SteveSandersonMS Sep 7, 2018

Author Member

I moved the StartupActions onto a new config class so we can use the DI system's Configure callbacks to register multiple startup actions (one for each AddServerSideBlazor<TClient> call). For now you can only call that once, because there's no way to specify distinct paths, but in the future we could have AddServerSideBlazor take an optional "path" arg.

/// <summary>
/// Options for Server-Side Blazor.
/// </summary>
public class ServerSideBlazorOptions

This comment has been minimized.

@SteveSandersonMS

SteveSandersonMS Sep 7, 2018

Author Member

I killed this because we're not doing anything with it yet, and there's a good chance that if we do add options in the future, they will mostly go to AddServerSideBlazor instead of UseServerSideBlazor. In any case it's unclear, so I'd prefer to drop it now and re-add something later when we know what it will be.

@SteveSandersonMS SteveSandersonMS requested a review from rynowak Sep 7, 2018

@anurse

This comment has been minimized.

Copy link
Member

anurse commented Sep 7, 2018

@anurse, do you know if there's any problem or disadvantage with calling that twice?

I do not believe this will cause a problem. We use TryAdd to register things in DI so the second time should just no-op (other than giving you an ISignalRBuilder to register additional stuff).

@davidfowl davidfowl requested a review from anurse Sep 8, 2018

@anurse

This comment has been minimized.

Copy link
Member

anurse commented Sep 8, 2018

I spoke a little too soon. There are two things that will be duplicated in the DI container if both you and the user call .AddSignalR. But I think we're OK:

  • SignalRMarkerService - This is just used to detect that you've called AddSignalR when you call UseSignalR (like MVC), duplicates here should be fine.
  • HubOptionsSetup - This establishes the defaults for IOptions<HubOptions>. Two instances of this should be fine, it'll just mean we set the options twice.

I'll file a bug to make those TryAdd, there's not really a reason they can't be. But I think this change can go forward without changing them since there isn't really a negative impact to the doubling up.

/// Constructs an instance of <see cref="BlazorHub"/>.
/// </summary>
/// <param name="logger">The <see cref="ILogger{BlazorHub}"/>.</param>
/// <param name="services">The service provider.</param>
public BlazorHub(

This comment has been minimized.

@rynowak

rynowak Sep 9, 2018

Member

I think we'll need to document everywhere that there's no support contract for anything on this class. We will need the ability to do things like change the constructor signature.

An alternative would be to define a composition root class that we inject into the constructor here.

This comment has been minimized.

@SteveSandersonMS

SteveSandersonMS Sep 11, 2018

Author Member

Good point. I've been through and updated all the XML docs to be more specific about "don't call this" (including on the constructor). Also I've sealed the class to reduce the scope for any virtual method shenanigans.

{
get => (CircuitHost)Context.Items[CircuitKey];
set => Context.Items[CircuitKey] = value;
}

/// <inheritdoc />

This comment has been minimized.

@rynowak

rynowak Sep 9, 2018

Member

FYI this doesn't actually do anything. Lots of code in our codebase does this but the compiler never added support for it.

This comment has been minimized.

@SteveSandersonMS

SteveSandersonMS Sep 11, 2018

Author Member

Yep, I know :) Maybe I'm delusional but I still hope that the proliferation of this technique will one day trigger some action on getting it implemented. And if not, and we choose to stop doing this, at least using it consistently everywhere will mean we can "find in files" all the places that need to be fixed.

This comment has been minimized.

@SteveSandersonMS

SteveSandersonMS Sep 11, 2018

Author Member

Also I do think we should "fix" this in any places where it meaningfully disadvantages a user by failing to show important docs.


return builder;
// TODO: Also allow configuring the endpoint path.
return UseSignalR(builder, BlazorHub.DefaultPath)

This comment has been minimized.

@rynowak

rynowak Sep 9, 2018

Member

This would be like 5 LOC to add 😀

This comment has been minimized.

@SteveSandersonMS

SteveSandersonMS Sep 11, 2018

Author Member

In this specific place, yes, but an E2E working system also requires a corresponding way to communicate the endpoint to the client, probably via some new JavaScript "boot with specific config" API.

private static IApplicationBuilder UseServerSideBlazorCore(
IApplicationBuilder builder,
Action<IBlazorApplicationBuilder> configure)
private static IApplicationBuilder UseSignalR(IApplicationBuilder builder, PathString path)

This comment has been minimized.

@rynowak

rynowak Sep 9, 2018

Member

nit: different name? this makes the call sites look like a call to the vanilla UseSignalR, but it's totally not.

This comment has been minimized.

@SteveSandersonMS

SteveSandersonMS Sep 11, 2018

Author Member

Good point. Renamed to UseSignalRWithBlazorHub.

// TStartup's Configure method".
services.Configure<DefaultCircuitFactoryOptions>(circuitFactoryOptions =>
{
var endpoint = BlazorHub.DefaultPath; // TODO: allow configuring this

This comment has been minimized.

@rynowak

rynowak Sep 9, 2018

Member

I'm not suggesting that we fix it in this PR, but this is leading us to a design where you specify the 'path' in both ConfigureServices and Configure.

One idea that I had about this CFOptions would need a map from path -> type, and type->startup. So during Add... we map the type->startup, and during Use... we map the path->type

Again, I don't expect that we'll solve it as part of this change, but I think we'd like to prevent you from having the specify the path in two places.

This comment has been minimized.

@SteveSandersonMS

SteveSandersonMS Sep 11, 2018

Author Member

Makes sense. Yes, I had been thinking of the "path" as being the identifier for the endpoint and thus the thing that defines correspondence between the config and the middleware.

Hopefully you're right that when we come to support multiple endpoints, we can consider possibly friendlier APIs. It might be that we use TStartup as the identifier, though technically someone might want to mount the same TStartup app paths two different paths (no idea why, but just that it's technically possible).

{
// Here we add a bunch of services that don't vary in any way based on the
// user's configuration. So even if the user has multiple independent server-side
// Blazor entrypoints, this lot is the same and repeated registrations are a no-op.
services.AddSingleton<CircuitFactory, DefaultCircuitFactory>();

This comment has been minimized.

@rynowak

rynowak Sep 9, 2018

Member

We should think about these all being TryAdd so that we don't register many instances. Generally we want framework code to be idempotent, and I think we expect users to call AddBlazor<T> multiple times for some scenarios.

This comment has been minimized.

@SteveSandersonMS

SteveSandersonMS Sep 11, 2018

Author Member

OK cool - changed these to TryAdd....

// important that people can set up these bits of middleware manually (e.g., to
// swap in UseAzureSignalR instead of UseSignalR).
subdirApp.UseSignalR(route => route.MapHub<BlazorHub>(BlazorHub.DefaultPath));
subdirApp.UseBlazor<BasicTestApp.Startup>();

This comment has been minimized.

@rynowak

rynowak Sep 9, 2018

Member

I love it. I 100% agree with the analysis and I think this is the right solution.

@rynowak

rynowak approved these changes Sep 9, 2018

@rynowak

This comment has been minimized.

Copy link
Member

rynowak commented Sep 9, 2018

@anurse - if you want to nuke this kind of problem from orbit, consider doing something like https://github.com/aspnet/Mvc/blob/release/2.2/test/Microsoft.AspNetCore.Mvc.Test/MvcServiceCollectionExtensionsTest.cs#L54

We unit test this in MVC, and it's a little annoying to maintain, but we know for sure that everything we do only happens once 😬

@anurse
Copy link
Member

anurse left a comment

if you want to nuke this kind of problem from orbit, consider doing something like

Filed aspnet/SignalR#2938, thanks @rynowak !

Looks good to me, and it should be safe to call AddSignalR twice.

public override Task OnDisconnectedAsync(Exception exception)
{
CircuitHost.Dispose();
return base.OnDisconnectedAsync(exception);
}

/// <summary>
/// Invoked by framework code. Not intended for use by applications.
/// </summary>
public async Task StartCircuit(string uriAbsolute, string baseUriAbsolute)

This comment has been minimized.

@anurse

anurse Sep 10, 2018

Member

We could probably put in some logic to allow exposing internal methods via an attribute ([HubMethod]). We just use reflection anyway so nothing stops us from allowing non-public methods except our own discovery choices. Thoughts @davidfowl ? Does MVC have anything equivalent for non-public Actions?

// WARNING: Don't add extra setup logic here. It's important for
// UseServerSideBlazor just to be shorthand for UseSignalR+UseBlazor,
// so that people who want to call those two manually instead can
// also do so. That's needed for people using Azure SignalR.

This comment has been minimized.

@anurse

anurse Sep 10, 2018

Member

The discoverability of this is a bit unpleasant... but I get the bind you're in. This may get a lot easier when/if endpoint routing comes in to play in 3.0. Since that will completely shake up the "hub registration" model anyway and it's possible that Azure SignalR and regular SignalR could share hub registrations and get you out of this trap.

This comment has been minimized.

@SteveSandersonMS

SteveSandersonMS Sep 11, 2018

Author Member

On the discoverability, I agree. But TBH there's no realistic chance that anyone's setting this up without using either a template or the docs (which I think is also true for many parts of ASP.NET Core and close relations such as EF and Azure SignalR).

If you make a design for new SignalR 3.0 routing config I'd definitely be interested to see it!

@rynowak

This comment has been minimized.

Copy link
Member

rynowak commented Sep 10, 2018

MVC doesn't have the ability to 'discover' public things for much the same reason signalR doesn't. I think there's a strong case to be made for 'discovery' to be based on public visibility, but manual wireups to support any visibility.

@davidfowl
Copy link
Member

davidfowl left a comment

I think this the the best we can do in the short term.

@SteveSandersonMS SteveSandersonMS force-pushed the stevesa/refactor-serversideblazor-startup-to-allow-azure-signalr branch from 1d64013 to a27d5d4 Sep 11, 2018

@SteveSandersonMS SteveSandersonMS merged commit a5f6311 into master Sep 11, 2018

4 checks passed

Blazor-ci-public #20180911.3 succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla All CLA requirements met.
Details

@SteveSandersonMS SteveSandersonMS deleted the stevesa/refactor-serversideblazor-startup-to-allow-azure-signalr branch Sep 11, 2018

@SteveSandersonMS SteveSandersonMS added this to the 0.6.0 milestone Sep 11, 2018

SteveSandersonMS added a commit to SteveSandersonMS/BlazorMigration that referenced this pull request Nov 27, 2018

Refactor server-side blazor startup to allow Azure SignalR. Fixes asp…
…net#1227 (aspnet#1396)

* Move startup action config into AddServerSideBlazor, so that UseServerSideBlazor is reduced to trivial shorthand that can become optional

* Make BlazorHub public so developers can use it with UseAzureSignalR

* Move BlazorHub to Microsoft.AspNetCore.Blazor.Server namespace for easier consumption

* Add notes

* Have E2E tests validate that devs don't have to call UseServerSideBlazor

* Add forgotten tweak

* CR: Document that BlazorHub methods are not intended for application use.

* CR: Rename extension method to UseSignalRWithBlazorHub

* CR: TryAdd
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.