Signalr internal types should continue to remain public in 3.0 until alternative pipeline is created in release 5. This is a breaking change from 2.2

[brijeshb]

Is your feature request related to a problem? Please describe.

Several internal classes which are key to customizing the signalr pipeline and message processing were made internal in 3.0. These were public in 2.2 and although they were internal classes, they exposed functionality that advanced scenarios / programmers could make use of to build enterprise grade apps. This breaking change which did not introduce an alternative approach forces developers to stay on 2.2.

Describe the solution you'd like

Please revert the conversion of these pubternal->internal types so they continue to stay pubternal until an alternative approach is developed.

Additional context

Signalr 's pipeline is not as mature as the regular asp.net mvc pipeline. It's currently a pain to implement advanced features that leads to very verbose code. This is a known problem and there is work being tracked for the next major release.
Before 3.0 as long as the core dependencies could be overridden. One could always copy the default implementations and override them with custom behavior.
In 3 these have been made internal without exposing an alternative solution. This leaves anyone hoping to make advanced use of signalr stuck on 2.2 until the signalr pipeline eventually rolls out in the next 3-6 months.

Some example use cases:
We override the hub dispatcher to implement better logging as a true cross cutting concern, without needing to sprinkle it throughout the hub methods.
We also perform global exception handling here enabling the transformation of lower level exceptions into well formed error responses, which are more useful than the standard hubexception.

One of the advanced use cases actually involves us using the hubdispatcher as a middleware to build a websocket proxy. We use a hubconnection connected to backend resources and replay the deserialized messages onto the correct backend resource. Without the ability to support generics at the hub dispatcher level, we would need to implement every method in the hub as well.

It's understandable that signalr's implementation has a long way to go before more of these features can be provided out of the box, and in some cases because of the need to support several protocols , it may be impossible to implement these in the platform. But atleast exposing the hooks allows consumers to override behavior and handle specific scenarios.

I have a humble request that we don't leave developers high and dry, by taking away functionality without introducing a sensible alternative first.

[analogrelay]

If we do such a thing it will be in 5.0, so I'm not sure what the value will be since the pipeline is planned for that release. Certainly if we don't get a full hub pipeline, we can do this as a fallback. Types in the internal namespace are unsupported and are not protected from breaking changes.

Can you provide example code of what you were doing so we can help guide to you an appropriate workaround for 3.0?

[analogrelay]

Linking #5353 for reference (the Hub Pipeline issue)

[CRidge]

I'm having a similar experience as @brijeshb - we also override the HubDispatcher as it seems to be the only way to get some access to the pipeline.

We need to start a IoC scope for each message coming in over SignalR to ensure DB changes are committed upon success and rolled back upon failures. This is something already incorporated in our IoC setup for normal web requests, and I don't want to solve the same problem twice just because of this.

I just converted our solution to Core 3.0, but at the moment this is the last blocker, and right now it means we can't convert and enjoy the nice features available in Core 3.0 :(

I get your point about the classes being internal - but there isn't a "right" way to do these things, as I can see - and the "wrong" way is pretty OK. And if a permanent solution isn't planned until 5.0 I really hope you help us out until something better is available ...

[analogrelay]

We need to start a IoC scope for each message coming in over SignalR to ensure DB changes are committed upon success and rolled back upon failures. This is something already incorporated in our IoC setup for normal web requests, and I don't want to solve the same problem twice just because of this.

I presume you're talking about a separate container from the ASP.NET Core DI system? Because we already create a DI scope for each message in that system.

[glebkozh]

@anurse I've got similar problem: I've used custom subclass of DefaultHubDispatcher in order to do some additional message parameters validation in DispatchMessageAsync. Does 3.0 release contain any extension point where I can hook in to process incoming message before actual hub methods get called?

[analogrelay]

I've used custom subclass of DefaultHubDispatcher in order to do some additional message parameters validation in DispatchMessageAsync. Does 3.0 release contain any extension point where I can hook in to process incoming message before actual hub methods get called?

You could use a custom IHubProtocol but that's a bit drastic. You could also, of course, do validation in your Hub methods themselves (which is repetitive but can be made fairly simple depending on how many Hub methods you have).

I think this is precisely the kind of reason we need #5353. Relying on internal types to do this kind of thing isn't really sustainable. The Dispatcher (being in an Internal namespace) was never really designed to support this kind of extension.

[glebkozh]

Thank you!

You could use a custom IHubProtocol but that's a bit drastic.

Yes, it seems too low-level.

You could also, of course, do validation in your Hub methods themselves (which is repetitive but can be made fairly simple depending on how many Hub methods you have).

That's, of course, the most straightforward solution, but it introduces a lot of boilerplate code (especially when you have to validate a bunch of parameters). Dispatcher allowed to use generalized mechanism to validate all parameters in attribute-driven way, for example.

I think this is precisely the kind of reason we need #5353.

+1, officially supported and documented pipline is really desired

[Exocomp]

Running into the same issue, using CustomHubDispatcher derived from DefaultHubDispatcher and overriding DispatchMessageAsync to do some cross cutting concerns. This changes forces me to spread code to hub methods which is not necessary/ideal. Looking forward to a better alternative.

[dotnetjunkie]

The integration of non-conforming containers, such as Simple Injector, Castle Windsor, and Ninject, depend on the existence of good interception points, such an IHubActivator<T>. Removing these interception points is a big oversight IMO, and an abstraction to intercept the creation of hubs (and possibly other SignalR root types) should be reintroduced a.s.a.p.

Take for instance on how Simple Injector users are expected to integrate with SignalR Core 2: simpleinjector/SimpleInjector#631. This is now broken with SignalR Core 3.

There seems a more general issue here that transcends SignalR. It seems every new framework that's coming out of the .NET Core stack misses these required abstraction. Some of the frameworks I engaged in discussions about these missing abstractions are: Azure Functions, gRPC, Blazer Components, and now SignalR Core. This seems a recurring pattern. It seems to me that some common (internal) Microsoft guidelines are missing about how to design libraries and frameworks on top of .NET Core.

/cc @davidfowl

[davidfowl]

I’m not sure why you needed a hub dispatcher in the first place but I do see a bigger need to support decorators in the default container.

[dotnetjunkie]

David, you are ignoring my bigger point here: the need to have good interception points. This goes beyond DI. You, as architect, should be pushing this message, to make the platform as extendable and usable as possible for anyone.

[davidfowl]

The system is fundamentally based on DI, non-conforming containers plug into the existing system using the built in DI. Before the solution you proposed was using inheritance as a workaround to do decoration around the default implementation. Decoration would allow this in a more elegant way. As for blazor, there’s work to be done there.

You, as architect, should be pushing this message, to make the platform as extendable and usable as possible for anyone.

We try to apply the same patterns across the stack for activation (grpc has a similar pattern to signalr). Sometimes we miss things though and it doesn’t help when we get late feedback outside of the development cycle. It makes it harder to react to changes that we made that in turn might be breaking again (we’d like to reduce the churn).

What makes it even harder is when the intended extensibility points aren’t being used but unintended ones are (like the above).

[dotnetjunkie]

Hi @davidfowl,

It would indeed be beneficial to see decoration been added as first-class citizen to the built-in Container, that's for sure. That would certainly have made the integration I proposed for integrating Simple Injector with SignalR Core 2 (a bit) "more elegant."

But although an improvement, this doesn't solve the problem we have now. You seem to mention that SignalR has "intended extensibility points [that] aren’t being used." Please help me out here. Which extensibility points currently exist in SignalR Core 3 that allow Simple Injector users, users of other non-conforming containers, and other users that which to intercept the creation of SignalR types? You would be of great help if you could guide anyone reading this in how to plug in to SignalR Core 3 atm.
In case, however, those extensibility points are currently missing, I would kindly ask you to help the SignalR Core 3 developers in adding back proper extensibility points.

Sometimes we miss things though and it doesn’t help when we get late feedback outside of the development cycle.

Yes; sometimes developers miss things. That happens. I don't blame them. My earlier comment wasn't meant as an attack on those developers. My point, however, merely was that I think that these recurring extensibility issues can be prevented elegantly by having some good guidance in place that teaches Microsoft (and third-party) developers, that they must "make sure that there are adequate composition roots per framework to make integration as smooth as it can be."

I hope this makes sense.

[davidfowl]

It would indeed be beneficial to see decoration been added as first-class citizen to the built-in Container, that's for sure. That would certainly have made the integration I proposed for integrating Simple Injector with SignalR Core 2 (a bit) "more elegant."

It would make lots of things easier and that’s a better solution for “hooking into” the hub dispatcher. All of the above implementations are trying to decorate in some form (otherwise it would just break how the system works).

I don’t understand why it isn’t sufficient to create the scope in the activator itself and destroy it in dispose. What doesn’t work there?

[dotnetjunkie]

David,

Thank you for your input. The custom HubDispatcher<T> was meant as a solution to ensure that all calls to a Hub would be wrapped in a (lifetime) scope. I'm now trying to reproduce the scenario where a hub would be called outside the context of an active Simple Injector scope. This would only happen when the scope is called outside the context of a web request; for instance when middleware hasn't ran, or already ran to completion. For some reason, however, I'm unable to reproduce this scenario and I'm now actually wondering whether this scenario is actually possible with SignalR Core, or whether this is only something that could happen in the context of SignalR 'classic.'

Can you confirm whether all Hub operations are always called from within a normal ASP.NET Core pipeline or not? If so, integration for non-conformers is actually pretty easy, because it only concerns replacing the default hub activator.

This might not hold for other scenarios, but for logging and applying other cross-cutting concerns, the same problems would exist for MVC Controllers, which is something that would normally be achieved using middleware (or using a more SOLID design internally).

Thanks in advance

[davidfowl]

That's not a great assumption, especially for things like long polling and non-http SignalR. That said, I don't understand why you can't create your scope and tear it down in CreateHub and Release. What am I missing?

[dotnetjunkie]

That's not a great assumption, especially for things like long polling and non-http SignalR.

Hmmm… that possibly complicates things… a lot.

I don't understand why you can't create your scope and tear it down in CreateHub and Release.

Although that would work in some cases, that would cause trouble when the hub is running inside a HTTP request, because in that case Simple Injector has wrapped the request with middleware (via an IStartupFilter) in a (lifetime) scope. Destroying the scope at that point might cause any other middleware that the user might have applied, to fail. On the other hand, wrapping the Hub in its own scope can cause a scoped instance to be created twice within the same request. This can lead to confusing situations.

I can see, however, a hybrid model, where the custom hub activator makes use of an active scope. In absence of such scope, the hub can manage its own scope by destroying it inside the Release method. This method, however, depends on one important assumption: SignalR resolves the hub activator on every request. This is required, because in the absence of a web request, the custom hub activator will need supply .NET Core's scoped IServiceProvider to Simple Injector's .NET Core integration in order for it to be able cross-wire MS.DI dependencies.

I tested whether SignalR resolves the activator on each request and this seems to be the case, but I want to double check with you if this is stable behavior on which anyone can safely depend. If, in a future release, SignalR would resolve a hub activator only once (and caches it), that would then break such integration. Can you confirm that resolving the hub activator on each request is intended and you have the proper verification mechanisms (i.e. unit tests and documentation) in place that secures this behavior?

[davidfowl]

Although that would work in some cases, that would cause trouble when the hub is running inside a HTTP request, because in that case Simple Injector has wrapped the request with middleware (via an IStartupFilter) in a (lifetime) scope. Destroying the scope at that point might cause any other middleware that the user might have applied, to fail. On the other hand, wrapping the Hub in its own scope can cause a scoped instance to be created twice within the same request. This can lead to confusing situations.

What happens if you create a new scope if a scope already exists in SimpleInjector?

I can see, however, a hybrid model, where the custom hub activator makes use of an active scope. In absence of such scope, the hub can manage its own scope by destroying it inside the Release method.

I'm not sure it's reasonable to rely on this behavior. Long polling for example will end up with hub execution between requests and the Azure SignalR integration runs completely outside of the http requests (though that should be easier).

This method, however, depends on one important assumption: SignalR resolves the hub activator on every request. This is required, because in the absence of a web request, the hub will need supply .NET Core's scoped IServiceProvider to Simple Injector in order for it to cross-wire dependencies.

It's resolved once per hub activation (so multiple times within a request). OnConnected, hub invocation and OnDisconnected.

[dotnetjunkie]

Let me start with apologizing to the OP for accidentally high jacking this discussion. I didn't intend to do so, but hopefully this discussion leads to something for everyone.

What happens if you create a new scope if a scope already exists in SimpleInjector?

The behavior wouldn't be different from that of MS.DI (except for the ambient model of course), because a nested scope behaves as its own isolated bubble. Nothing is shared between the bubbles. This is obviously the behavior you want, but creating new scopes implicitly on the background could obviously be confusing to the user. Unless the users do so themselves explicitly, you don't want a single web request to run in more than one scope.

I'm not sure it's reasonable to rely on this behavior. Long polling for example will end up with hub execution between requests and the Azure SignalR integration runs completely outside of the http requests (though that should be easier).

Perhaps I should show an example? This might give a better understanding of how I think I can solve this problem. Please supply me with feedback on whether this could work, or wouldn't:

// Must be registered using AddScoped
public sealed class SimpleInjectorHubActivator<T> : IHubActivator<T> where T : Hub
{
    private static AsyncScopedLifestyle ScopedLifestyle = new AsyncScopedLifestyle();

    private readonly Container container;
    private readonly IServiceProviderAccessor accessor;

    // This impl must be scoped because of these two fields.
    // IServiceProvider must be the request-scoped provider, not the root.
    private readonly IServiceProvider msScope;
    private Scope ownedScope;

    public SimpleInjectorHubActivator(
        Container container,
        SimpleInjector.Integration.ServiceCollection.IServiceProviderAccessor accessor,
        IServiceProvider msScope)
    {
        this.container = container;
        this.accessor = accessor;
        this.msScope = msScope;
    }

    public T Create()
    {
        StartScope();

        // Resolve the Hub; this might cause dependencies to be loaded from MS.DI.
        return this.container.GetInstance<T>();
    }

    public void Release(T hub)
    {
        EndScope();
    }

    private void StartScope()
    {
        if (!this.IsRunningInRequestScope)
        {
            // Create a new ambient scope. This relies on AsyncLocal<T>
            this.ownedScope = AsyncScopedLifestyle.BeginScope(this.container);

            // Supply the scoped IServiceProvider to Simple Injector's integration
            // to allow Simple Injector to cross-wire from the curent msScope.
            // This uses an AsyncLocal<IServiceProvider> to flow the scope.
            this.accessor.SetCurrent(this.msScope);
        }
    }

    private bool IsRunningInRequestScope =>
        ScopedLifestyle.GetCurrentScope(this.container) != null;

    private void EndScope()
    {
        this.ownedScope?.Dispose();
    }
}

The following points describe what this activator does:

• In this activator implementation, the creation of a hub is always wrapped in a Simple Injector Scope.
• The implementation checks to see if it is already running in an ambient scope, which would typically mean that Create() is called while being wrapped in ASP.NET Core startup middleware (since Simple Injector ensures an ambient scope to exist for requests by using startup middleware).
• If no scope exists, it will create a new ambient scope before resolving the Hub type from Simple Injector.
• Besides creating a new scope, the activator also passes the scoped MS.DI IServiceProvider on to Simple injector. This allows the provider to be used while resolving cross-wired registrations. When running inside a web request, Simple Injector ensures that the scoped IServiceProvider is captured, but since Create can run outside the context of a web request, this activator must provide Simple Injector with that instance.
• When SignalR calls the activator's Release method, the activator will dispose the scope that he created, and do nothing if the operation was executed in an already existing scope.

This implementation makes a few assumptions:

• An activator instance will be used for resolving one hub, and one hub only.
• Calling the Hub's 'action' methods is done within the same asynchronous control flow (i.e. it can access the same state that is stored by an AsyncLocal<T>) as where the Create method runs in. (Release is allowed to run outside this control flow, since it the activator holds a reference to the scope).
• Within a single logical (async) flow, there is only a call to a single activator. A single asynchronous control flow (again: with access to the same AsyncLocal<T> state) just a single activator is resolved, or in case multiple are resolved, a call to Create is never in between the calls to Create and Release from another activator.

Can you feedback on this implementation and the assumptions I'm am making with this implementation?

[davidfowl]

The implementation checks to see if it is already running in an ambient scope, which would typically mean that Create() is called while being wrapped in ASP.NET Core startup middleware (since Simple Injector ensures an ambient scope to exist for requests by using startup middleware).

I would remove this check completely.

An activator instance will be used for resolving one hub, and one hub only.

👍

Calling the Hub's 'action' methods is done within the same asynchronous control flow (i.e. it can access the same state that is stored by an AsyncLocal) as where the Create method runs in. (Release is allowed to run outside this control flow, since it the activator holds a reference to the scope).

👍

Within a single logical (async) flow, there is only a call to a single activator. A single asynchronous control flow (again: with access to the same AsyncLocal state) just a single activator is resolved, or in case multiple are resolved, a call to Create is never in between the calls to Create and Release from another activator.

👍

[dotnetjunkie]

Thank you for your feedback

I would remove this check completely.

Would you mind elaborating on this? Why would you prefer removing this? Is this because of possible issues, or something else? Why wouldn't you mind having a nested scope? Or are you suggesting something else?

[davidfowl]

Would you mind elaborating on this? Why would you prefer removing this? Is this because of possible issues, or something else? Why wouldn't you mind having a nested scope? Or are you suggesting something else?

It's broken if you end up using the existing request scope. The request could end immediately after the check for IsRunningInRequestScope returns false then you'd end up using a disposed scope to resolve the hub.

[dotnetjunkie]

It's broken if you end up using the existing request scope. The request could end immediately after the check for IsRunningInRequestScope returns false then you'd end up using a disposed scope to resolve the hub.

But if IsRunningInRequestScope returns false, it means there is no web request. In that case a new scope would be created and managed by the activator. What is the problem in that? Or did you mean that the problem exists when IsRunningInRequestScope returns true? If so, why could the request disappear in parallel to the running hub? What mechanism is in place that would allow this, and what's the reason for this?

[davidfowl]

But if IsRunningInRequestScope returns false, it means there is no web request

Sorry I meant returns true not false.

If so, why could the request disappear in parallel to the running hub? What mechanism is in place that would allow this, and what's the reason for this?

Yes. The HTTP layer in SignalR is treated like a transport and we just transfer bytes into the pipe, we don't do invocation of the operation on the request itself.

The reason it can happy though is because we don't explicitly clear the execution context before calling into the logic that eventually runs the hub. That logic can start from the request thread and execute in parallel with it.

It's decoupled because of other transports decoupled from HTTP. The execution context behavior is mostly a side effect of it not being explicitly cleared.

[dotnetjunkie]

Thank you @davidfowl for this valuable information. This means that the activator must always create a new scope for creating a hub. I now have enough information to be able to create an integration package.

[ghost]

Thank you for contacting us. Due to a lack of activity on this discussion issue we're closing it in an effort to keep our backlog clean. If you believe there is a concern related to the ASP.NET Core framework, which hasn't been addressed yet, please file a new issue.

This issue will be locked after 30 more days of inactivity. If you still wish to discuss this subject after then, please create a new issue!