Skip to content

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

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

Open
vicancy opened this issue Jul 9, 2020 · 8 comments
Assignees
Labels
affected-few This issue impacts only small number of customers area-signalr Includes: SignalR clients and servers enhancement This issue represents an ask for new feature or an enhancement to an existing one severity-minor This label is used by an internal tool
Milestone

Comments

@vicancy
Copy link
Contributor

vicancy commented Jul 9, 2020

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.

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.

this.resetKeepAliveInterval();

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 Azure/azure-signalr#943

Describe the solution you'd like

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

@BrennanConroy
Copy link
Member

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 #5283

@BrennanConroy BrennanConroy added this to the Backlog milestone Jul 10, 2020
@BrennanConroy BrennanConroy added affected-few This issue impacts only small number of customers enhancement This issue represents an ask for new feature or an enhancement to an existing one severity-minor This label is used by an internal tool labels Nov 9, 2020 — with ASP.NET Core Issue Ranking
@halter73
Copy link
Member

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
Copy link
Contributor Author

vicancy commented Apr 20, 2022

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
Copy link
Member

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
Copy link

ghost commented Sep 8, 2022

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
Copy link
Member

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
Copy link

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

@BrennanConroy
Copy link
Member

Because the work hasn't been done...

@BrennanConroy BrennanConroy modified the milestones: .NET 8 Planning, Backlog Mar 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affected-few This issue impacts only small number of customers area-signalr Includes: SignalR clients and servers enhancement This issue represents an ask for new feature or an enhancement to an existing one severity-minor This label is used by an internal tool
Projects
None yet
Development

No branches or pull requests

7 participants