Skip to content
This repository has been archived by the owner on Dec 18, 2018. It is now read-only.

Add HubLifetimeManager.InvokeConnectionsAsync() #683

Closed
KPixel opened this issue Jul 31, 2017 · 6 comments
Closed

Add HubLifetimeManager.InvokeConnectionsAsync() #683

KPixel opened this issue Jul 31, 2017 · 6 comments

Comments

@KPixel
Copy link

KPixel commented Jul 31, 2017

Hello

Can we add the following method to the class HubLifetimeManager?

Task InvokeConnectionsAsync(IEnumerable<string> connectionIds, string methodName, object[] args);

This will enable dynamic "grouping" where the application decides to message connections arbitrarily. We could use the InvokeConnectionAsync() but this one can be far more efficient.

Here is an example of how it could be used (INode is like a tree node which recursively find all attached connections):

        public static async Task InvokeNodeAsync<THub, T>(this HubLifetimeManager<THub> hub, INode<T> node, string methodName, object[] args) where T : INode<T>
        {
            var connectionIds = await node.GetConnectionIdsAsync();
            await hub.InvokeConnectionsAsync(connectionIds, methodName, args);
        }

Some other practical scenarios that this API will enable:

  • Noisy group: Even in a "static" group, we could choose to dynamically exclude some connections that already received another message recently (like a notification system where if you haven't read the previous notification, we don't send another one)
  • Too many groups: In a large forum where you auto-subscribe to every thread you participate in, you can be part of so many groups, that it is impractical to load them when you connect. Another variant of this scenario is to want an implementation where the group has a list of (users who have a list of) connections...
  • Undefinable "group": An app using geo-localisation may allow a user to send a message to everyone who is X kilometers close to a point of interest. So, the app will dynamically find all the devices in that area, and message them.

I am giving these scenarios because they may suggest other ways to improve SignalR.

@davidfowl
Copy link
Member

Sure this is part of work here to round out the APIs #394.

@KPixel
Copy link
Author

KPixel commented Aug 2, 2017

Indeed.
Edit: I would prefer connectionIds to be an IEnumerable (instead of an IList) because I use HashSet to ensure their uniqueness and HashSet doesn't implement IList.

If we take it one step further, I don't think that ALL Hubs should be forced to implement the current Group(s) & User(s) APIs. I think they are opinionated ways to group connectionIds and should be optional, so each application can decide what a "group" is for them.

However, I understand that people coming from the previous version of SignalR will expect these familiar APIs. So, hopefully, you can figure out a balanced design.

@davidfowl
Copy link
Member

This is already true today if you ignore Add/Remove Group on the lifetime manager. You can make the group logic do whatever you wanted to without a specific InvokeConnectionsAsync call. It's actually a bit more flexible than what you suggest since you can map that to anything really.

The downside is that the existing APIs mean that there's expectation that a group Add/Remove is based on explicit calls so maybe there needs to be another concept that gives you a string and you can implement that in anyway that makes sense.

If we take it one step further, I don't think that ALL Hubs should be forced to implement the current Group(s) & User(s) APIs. I think they are opinionated ways to group connectionIds and should be optional, so each application can decide what a "group" is for them.

Yes but the other big feature is being able to send to connection ids that don't exist in any storage. The current redis implementation doesn't store a list of connection ids anywhere, it just uses pub/sub with a specific key to send to a connection id.

@davidfowl
Copy link
Member

@KPixel I also realized that this doesn't describe how the caller would initiate the call to this dynamic set of connections. How does the dispatch work? Clients.Magic.InvokeAsync()

@KPixel
Copy link
Author

KPixel commented Sep 28, 2017

@davidfowl Maybe we could cache the connectionIds list somewhere with a key, then call the Group() API with that key and have a custom HubLifetimeManager to process it... That is a workaround that could work.

Still, I don't see the downside of adding this method (aside for having to implement it).

My other point is that "user" and "group" are examples of how many connectionIds can be indirectly referred to. And there are many other ways that this indirection could be implemented (see the scenarios at the top). So, ideally, HubLifetimeManager should only deal with connectionIds and defer these grouping techniques to specialized implementations; maybe with something like IFeatureCollection. Same for the layers above.

But I understand that it would complicate the overall architecture of SignalR and as such not be worth the effort. And there is also the historical heritage of SignalR that can be hard to get away from.

Note: Now that I am no longer using the SignalR layer, this is a theoretical discussion for me (until the day when I work on a different project where SignalR is a better fit :) ).

@davidfowl
Copy link
Member

My point is that groups already covers this and I see this being less flexible than what's already there. Dealing only with connnection ids is limiting as you said requires another abstraction that stores them and requires the lifetime manager to use that abstraction to map keys to groups of connections?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants