search

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
34.89k stars 9.85k forks source link

[SignalR] Add an option to enable client side ping check instead of only relying on client side PingMessage #23794

Open vicancy opened 4 years ago

vicancy commented 4 years ago

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

The issue is SignalR server-side relies on the first PingMessage to enable the client-side ping check. https://github.com/dotnet/aspnetcore/blob/2ad8121efbdddd46920918bddb9ee67e1c4144a9/src/SignalR/server/Core/src/Internal/DefaultHubDispatcher.cs#L180

However, if the client-side sends out messages constantly and more frequently than keep alive, the client never has any chance to send out PingMessage, and as a result the server-side ping check never invokes.

https://github.com/dotnet/aspnetcore/blob/d5cf36acc71fe576541bbb8d694a88815c22c800/src/SignalR/clients/ts/signalr/src/HubConnection.ts#L344

And this can cause the issue that when using SSE to connect to the service, although client fails to send to the server when JWT token expiration(401), the connection never closes, like described here https://github.com/Azure/azure-signalr/issues/943

Describe the solution you'd like

Provide an option to explicitly enable the ping check through SignalROptions can solve the issue.

BrennanConroy commented 4 years ago

The feature you describe above is close to something I've been wanting to do for awhile. There should be a way to enforce clients to have to send pings.

if the client-side sends out messages constantly and more frequently than keep alive, the client never has any chance to send out PingMessage, and as a result the server-side ping check never invokes.

This is a small bug, it should send the ping immediately after starting the connection, like the .NET client does.

when using SSE to connect to the service, although client fails to send to the server when JWT token expiration(401), the connection never closes

This is the actual issue they are seeing, it's an issue with the auth story and tracked by https://github.com/dotnet/aspnetcore/issues/5283

halter73 commented 2 years ago

We should make this automatic for newer clients, so they can opt-in to this. Not for security, but to notice issues like those described in #41081 sooner.

vicancy commented 2 years ago

Do we have an ETA for fixing this issue? Shall it follow the net7 release pipeline or it can be released on its own since it only affects the js package? We have several customers reporting this issue. cc @davidfowl

davidfowl commented 2 years ago

We can do it during the .NET 7 release timeframe.

cc @rafikiassumani-msft I'm going to move this out of the backlog for re-triage.

ghost commented 1 year ago

Thanks for contacting us.

We're moving this issue to the .NET 8 Planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s). If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues. To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

BrennanConroy commented 1 year ago

Shall it follow the net7 release pipeline or it can be released on its own since it only affects the js package?

We have updating the js client for .NET 7.

v-paulino commented 1 year ago

Hello @BrennanConroy ! I was wondering what could be the reason why the issue is still open.

BrennanConroy commented 1 year ago

Because the work hasn't been done...