Skip to content

[SignalR][TS client] Send ping on connection start #43695

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

Merged
merged 1 commit into from
Sep 9, 2022
Merged

Conversation

BrennanConroy
Copy link
Member

@BrennanConroy BrennanConroy commented Sep 1, 2022

Part of #23794

This changes the JS client to send a ping on connection startup instead of after ~15 seconds of inactivity. This helps the keep alive detection for when a client drops within the first few seconds of connecting. The .NET client already does this, so it's bringing the JS client up to par in this respect.

@BrennanConroy BrennanConroy added Servicing-consider Shiproom approval is required for the issue area-signalr Includes: SignalR clients and servers feature-client-javascript Related to the SignalR JS client labels Sep 1, 2022
@Pilchie Pilchie added this to the 7.0-rc2 milestone Sep 1, 2022
@Pilchie Pilchie removed the Servicing-consider Shiproom approval is required for the issue label Sep 8, 2022
Copy link
Member

@halter73 halter73 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am sad that we have to send a ping before the server starts requiring them. Can you link to the issue to properly handle this in the hub handshake?

Also, interesting to note. Doesn't this timeout anyone who has default server settings and then set a very large ping interval in the TS client? Could that be something someone has done? I hope not.

@BrennanConroy
Copy link
Member Author

Can you link to the issue to properly handle this in the hub handshake?

#23794 is tracking that.

Doesn't this timeout anyone who has default server settings and then set a very large ping interval in the TS client? Could that be something someone has done? I hope not.

Yeah, that should be a very uncommon scenario. And if they are doing that, then in 7.0 they can fix it by setting the servers keep alive timeout to a large value.

@BrennanConroy
Copy link
Member Author

@Pilchie for RC2. This is improving a scenario noticed by the Azure SignalR team where if a client disappears within the first ~15 seconds of connecting the server will take a long time to notice the client is gone.

@Pilchie
Copy link
Member

Pilchie commented Sep 8, 2022

Approved for .NET 7 RC2

@halter73
Copy link
Member

halter73 commented Sep 8, 2022

Should we move #23794 out of rc2 and into .NET 8 planning then?

@BrennanConroy
Copy link
Member Author

@dotnet/aspnet-admins please merge

@wtgodbe wtgodbe merged commit b83d48f into release/7.0 Sep 9, 2022
@wtgodbe wtgodbe deleted the brecon/qping branch September 9, 2022 16:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-signalr Includes: SignalR clients and servers feature-client-javascript Related to the SignalR JS client
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants