dotnet / aspnetcore

ASP.NET Core is a cross-platform .NET framework for building modern cloud-based web applications on Windows, Mac, or Linux.
https://asp.net
MIT License
35.48k stars 10.03k forks source link

How to disconnect specified client from outside hub #5333

Closed bluetianx closed 3 years ago

bluetianx commented 6 years ago

I have a web Api for disconnectting specified client ,how to disconnect specified client from outside hub. I do not find a method about disconnect client from IHubContext;

davidfowl commented 4 years ago

On user X reconnect there is a (new connection established) but the old one still exists in _hubContext.Clients list So if the internet fail and user reconnect multiple times there will still be active connections saved in memory in clients list

Those connections will go away eventually, as OnDisconnectAsync will fire regardless.

bretbas commented 3 years ago

I also want an answer to the question @Tommigun1980 asked earlier:

One final question (I promise!) - I need to know if a user is connected at the time when trying to send a message. If he's not I would like to send a push instead. There seems to be no API for this and no return values from the RPCs (presumably to avoid additional data transfers), so I guess this is tangentially related to the whole Abort() thing. Is there any mode, API or anything, that could give this information when scaling out with the service? If not I think I'll go down the route you kinda suggested earlier, with another server where hubs notify of and can poll for connection status.

In the end my use case is really simple - I want to send a push if a client is offline, else make an RPC notification call.

How can this be achieved if connections are shared between multiple machines, and when call REST api endpoint, I cannot know for sure if the client is connected or not on the machine where the REST api call originated? Thanks!

davidfowl commented 3 years ago

How can this be achieved if connections are shared between multiple machines, and when call REST api endpoint, I cannot know for sure if the client is connected or not on the machine where the REST api call originated?

By storing the presence information in a database and checking that information when you want to send the message to the client. If the message is server generated then you don't need send a message.

You can build a system that polls the status of connections on each node/machine/pod/compute unit and abort connections that are marked for abort.

There are lots of ways to build this functionality

bretbas commented 3 years ago

@davidfowl , Good idea! Thanks

Tommigun1980 commented 3 years ago

Hi and thanks for the feedback.

Considering that the SignalR service has to ultimately deduce this very information itself (which is the exact service it provides) I would like to use this very information myself, instead of trying to build a robust fault-tolerant scaling system just for this (while essentially replicating the service).

...

Suggestion: Add an alternative call/method that returns delivery status (say just a boolean for whether connected or not).

It would work just like the regular send, but await until status is known, and trickle it back to the caller.

...

This is the very kind of thing I would really appreciate (and even expect) from a service as it solves a very common real life scenario, where some other action should take place if a user is not connected (in my case sending a push notification instead).

If I have to start implementing this then I’m circumventing/replicating the entire service for pretty much every call, and this information must be known by/deduced by the service in order to do its thing - if it just could be sent back would be absolutely fantastic!!

Thanks so much for considering! I would really really appreciate this feature!

davidfowl commented 3 years ago

There's no way to implement this feature in a distributed system efficiently in a way where you can always know if the person in online or offline before sending a message. Once you accept that then you'll recognize that you need to adjust the application behavior to deal with the potential to send a message to the client that isn't there and deal with the fallout of that.

Signalr doesn't give any feedback about messages being received by the client.

That said I do think presence information is useful and maybe will be in the service one day.

davidfowl commented 3 years ago

As for the client acks, there is an issue for it here https://github.com/dotnet/aspnetcore/issues/5280

Tommigun1980 commented 3 years ago

There's no way to implement this feature in a distributed system efficiently in a way where you can always know if the person in online or offline before sending a message. Once you accept that then you'll recognize that you need to adjust the application behavior to deal with the potential to send a message to the client that isn't there and deal with the fallout of that.

Signalr doesn't give any feedback about messages being received by the client.

That said I do think presence information is useful and maybe will be in the service one day.

Hi and thanks for the reply.

This doesn’t need to be known before the fact. At some point some part of your service has to make the decision for whether to send (ie does a connection exist), right? Could this information be trickled back?

It is an extremely common use case to send a push if a user is not connected, and for this it needs to be known whether any connection exists to a user.

It may be somewhat tricky, but this is the exact reason why I’m using your service in the first place. To solve these things for me.

Thank you again for considering.

Tommigun1980 commented 3 years ago

And to clarify, I only need to get back whether the service tried to send the message.

bool connected = await client.Send(x);
if (!connected)
    // send push

The send call goes through your system through various parts, until some part ultimately takes the decision whether to proceed or abort. Every part in the chain trickles this information up.

Why wouldn’t this be possible? This would save us from recreating the service. I don’t think the service is complete in any way without this feature as it’s such a common use case.

Thank you again.

davidfowl commented 3 years ago

Between the send and the call you make to send a push notification. The client can be back online. Then what?

Tommigun1980 commented 3 years ago

This “problem” exists everywhere, in any system that sends a push if a client is not connected (like Facebook Messenger).

Naturally the system returns the state that was valid at the time and that information trickles up.

If a client comes online while a call is being made which returns “not connected”, then that information is used and a push is sent. That push is not displayed on the client as the app is in the foreground (or he sees the notification while the app boots if some OS allows foreground push).

It’s the same with every application. Why would it be a problem at all? This is how they all work.

It feels to me like solving a problem that’s not there, all systems just return the state they operated on at the point in time it happened.

On Sunday, February 7, 2021, David Fowler notifications@github.com wrote:

Between the send and the call you make to send a push notification. The client can be back online. Then what?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/dotnet/aspnetcore/issues/5333#issuecomment-774557282, or unsubscribe https://github.com/notifications/unsubscribe-auth/AOL6P6ENCCKXIBJMFIUREG3S5XD2LANCNFSM4GK3Y6OA .

--

Tommigun1980 commented 3 years ago

Also, if this was a real problem it would be a thousand times worse by having to make a separate call first to see how to proceed.

The best most efficient way is for the service to return this info, it can’t get any better than that.

Tommigun1980 commented 3 years ago

And I’ll elaborate a bit even further:

The other option is to always also send a push (ie send message + push). Why not do it?

1) Cost 2) OS vendors will start throttling your pushes if you do that 3) If the user has multiple devices, his non-active devices would be constantly pinging with notifications while he uses the app

So only sending a push when needed is the way to go.

If the client connects a millisecond after the server checks connection status, and sends one "unnecessary" push - what’s the harm? It’s the best we can do, how every app works, and it doesn’t have negative effects. We were operating on the information available at the time, and it’s the best we can ever do (unless we implement time travel).

davidfowl commented 3 years ago

Today the presence information isn't provided by the service so you need to do that, it's not about the send itself, it's about this information and it isn't tied to the call to send. Just use that information to send the push notification based on the current state even if it is potentially out of sync. As long as your application can handle the eventual consistency of it (and it has to), then it's fine.

Tommigun1980 commented 3 years ago

Thank you for the reply.

I understand that. Hence my suggestion to not just "fire and forget" the call, but to send back the information all the way to the caller.

Ultimately some part in the service has to make the actual call to a client - all I'm asking for is to trickle this back, i.e. whether an invocation was attempted. I'm not asking for a state info endpoint or such, I just want to the status to be returned back to the caller.

On Sun, Feb 7, 2021 at 1:50 AM David Fowler notifications@github.com wrote:

Today the presence information isn't provided by the service so you need to do that, it's not about the send itself, it's about this information and it isn't tied to the call to send. Just use that information to send the push notification based on the current state even if it is potentially out of sync. As long as your application can handle the eventual consistency of it (and it has to), then it's fine.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/dotnet/aspnetcore/issues/5333#issuecomment-774561652, or unsubscribe https://github.com/notifications/unsubscribe-auth/AOL6P6FYL6J4LQTI6JO7PWDS5XIVPANCNFSM4GK3Y6OA .

davidfowl commented 3 years ago

I understand that. Hence my suggestion to not just "fire and forget" the call, but to send back the information all the way to the caller.

While that's a separate issue, I don't think we'd add the API for this scenario. If you choose to use it for that, that's great, but the purpose of that issue is to return results from the client, not to detect if that client is online. Maybe you can use a failure to retrieve the result as the way to detect the offlineness.

Tommigun1980 commented 3 years ago

I understand that. Hence my suggestion to not just "fire and forget" the call, but to send back the information all the way to the caller.

While that's a separate issue, I don't think we'd add the API for this scenario. If you choose to use it for that, that's great, but the purpose of that issue is to return results from the client, not to detect if that client is online. Maybe you can use a failure to retrieve the result as the way to detect the offlineness.

I would love to do that, but I am failing to see how.

This is my current implementation:

public async Task SendNotification(
    Guid accountId,
    Func<IClient, Task> RPCCallAction = null,
    Func<Task<PushMessage>> pushMessageGetter = null)
{
    if (RPCCallAction == null && pushMessageGetter == null)
        return;

    if (await this.IsConnected(accountId))
    {
        if (RPCCallAction != null)
        {
            var clientInterface = this.connectionHub.Clients.Group(accountId.ToString());
            await RPCCallAction(clientInterface);
        }
    }
    else
    {
        if (pushMessageGetter != null && this.pushService.IsEnabled())
            await this.pushService.SendPush(await pushMessageGetter(), new string[] { accountId.ToString() });
    }
}

I.e. if a client is connected, it sends an RPC to all his active devices. Else it sends a push message. IsConnected is the method I'd like to get rid of, and my earlier suggestion was something in the lines of:

var clientInterface = this.connectionHub.Clients.Group(accountId.ToString());
bool hadConnections = await RPCCallAction(clientInterface); // the status of this would trickle back here!
if (!hadConnections)
    // send push here

You said I could use an error status for this which I'd be very happy to do, but how?

There will be no error if no call is made here:

var clientInterface = this.connectionHub.Clients.Group(accountId.ToString());
await RPCCallAction(clientInterface);

So how could I catch this as an error?

Thank you again!!

Tommigun1980 commented 3 years ago

I just can't understand why awaiting an RPC can't return the status information, or why it can't be added.

But if it can't be added for any reason I'd be more than happy to catch "nothing was sent" errors and handle that as an offline case as you suggested. But calling an RPC on an empty IClient doesn't raise an error, so I don't understand how I could do that? I'm very very happy if this can be done!

Tommigun1980 commented 3 years ago

If anyone is interested, IsConnected is currently implemented as such:

Every node uses an IMemoryCache for the case where the same node also serves the destination connected. If not it calls the Azure SignalR service's $"groups/{accountId}" endpoint, which returns 404 if no connections exist. This is extremely inefficient though as I have to make a call every time an RPC is sent, instead of the API just returning that information!

I also keep connection ids in an IDistributedCache (Azure Redis), so I can disconnect any client on any node when needed. But this leads to tons and tons and tons of problems to keep them in sync when stuff is rebooted etc.

davidfowl commented 3 years ago

I just can't understand why awaiting an RPC can't return the status information, or why it can't be added.

As I mentioned, there's an issue to enable this feature when sending to specific clients https://github.com/dotnet/aspnetcore/issues/5280. It's not currently on track to be implemented for .NET 6 either. It's non trivial to implement acks like this but its highly requested and we may spend some time figuring out all of the pieces. Then you could build something that used this new feature and tried to send a message to all clients for a specific user and decide if you should send a push notification instead.

Right now, you need to store presence information (as mentioned above) and optimistically do it.

Making the RPC wait for a reply from the client would slow down every operation and is even trickier when the ACK needs to work across a server farm.

This conversation has veered far from the original topic so I'd suggest piling onto that issue specified.

Tommigun1980 commented 3 years ago

I just can't understand why awaiting an RPC can't return the status information, or why it can't be added.

As I mentioned, there's an issue to enable this feature when sending to specific clients #5280. It's not currently on track to be implemented for .NET 6 either. It's non trivial to implement acks like this but its highly requested and we may spend some time figuring out all of the pieces. Then you could build something that used this new feature and tried to send a message to all clients for a specific user and decide if you should send a push notification instead.

Right now, you need to store presence information (as mentioned above) and optimistically do it.

Making the RPC wait for a reply from the client would slow down every operation and is even trickier when the ACK needs to work across a server farm.

This conversation has veered far from the original topic so I'd suggest piling onto that issue specified.

Fair enough.

As a last thing, could you please just let me know how to implement "Maybe you can use a failure to retrieve the result as the way to detect the offlineness."?

I loved that suggestion but I don't see any errors happening anywhere. So how would I achieve what you suggested?

Thank you!

davidfowl commented 3 years ago

"Maybe you can use a failure to retrieve the result as the way to detect the offlineness."?

While that's a separate issue, I don't think we'd add the API for this scenario. If you choose to use it for that, that's great, but the purpose of that issue is to return results from the client, not to detect if that client is online. Maybe you can use a failure to retrieve the result as the way to detect the offlineness.

It was part of that paragraph meaning that's how you could use the client ACK feature to implement the semantics you want. The feature doesn't exist.

Tommigun1980 commented 3 years ago

"Maybe you can use a failure to retrieve the result as the way to detect the offlineness."?

While that's a separate issue, I don't think we'd add the API for this scenario. If you choose to use it for that, that's great, but the purpose of that issue is to return results from the client, not to detect if that client is online. Maybe you can use a failure to retrieve the result as the way to detect the offlineness.

It was part of that paragraph meaning that's how you could use the client ACK feature to implement the semantics you want. The feature doesn't exist.

Thank you for the clarification, I thought you meant I could use it as an alternative until ACK is implemented. Oh well.

ghost commented 3 years ago

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!

hhyyrylainen commented 3 years ago

I have too many other things to try to work on besides opening a new issue for this, but I'm still interested in having inbuilt support for disconnecting a specific client for security reasons (leaving clients without valid accounts connected leaks information they should no longer have access to).