Skip to content
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

Server timeout and Keep Alive interval config #29297

Merged
merged 8 commits into from
Jun 2, 2023

Conversation

guardrex
Copy link
Collaborator

@guardrex guardrex commented May 17, 2023

Fixes #29296

Surayya ...

  • I searched through the SignalR node looking for update opportunities, but it doesn't look like explicit config for the server timeout or Keep-Alive interval with the JS client or .NET client are actually shown, the settings only seem to be named. I've tried to show the JS client code in the Release notes article correctly, but I won't be surprised if it's incorrect. I'm not as familiar with the general SignalR scenario compared to Blazor.
  • Ignore the little bit of version flipping going on where the Blazor content flips in and out of 8.0 to cover this. These pair of Blazor > Host and deploy node topics haven't been addressed yet for versioning. I'll fix up the versioning later.

Internal previews

📄 File 🔗 Preview link
aspnetcore/blazor/fundamentals/signalr.md ASP.NET Core Blazor SignalR guidance
aspnetcore/blazor/host-and-deploy/server.md Host and deploy ASP.NET Core Blazor Server
aspnetcore/blazor/host-and-deploy/webassembly.md Host and deploy ASP.NET Core Blazor WebAssembly
aspnetcore/release-notes/aspnetcore-8.0.md aspnetcore/release-notes/aspnetcore-8.0

@guardrex guardrex self-assigned this May 17, 2023
@guardrex
Copy link
Collaborator Author

Ping @surayya-MS ... Just making sure that you saw this ... no rush.

aspnetcore/blazor/fundamentals/signalr.md Outdated Show resolved Hide resolved
aspnetcore/blazor/fundamentals/signalr.md Outdated Show resolved Hide resolved
aspnetcore/blazor/fundamentals/signalr.md Show resolved Hide resolved
When creating a hub connection in a component, set the <xref:Microsoft.AspNetCore.SignalR.Client.HubConnection.ServerTimeout> (default: 30 seconds), <xref:Microsoft.AspNetCore.SignalR.Client.HubConnection.HandshakeTimeout> (default: 15 seconds), and <xref:Microsoft.AspNetCore.SignalR.Client.HubConnection.KeepAliveInterval> (default: 15 seconds) on the built <xref:Microsoft.AspNetCore.SignalR.Client.HubConnection>. The following example, based on the `Index` component in the [SignalR with Blazor tutorial](xref:blazor/tutorials/signalr-blazor), uses default values:
:::moniker range=">= aspnetcore-8.0"

When creating a hub connection in a component, set the <xref:Microsoft.AspNetCore.SignalR.Client.HubConnection.ServerTimeout> (default: 30 seconds) and <xref:Microsoft.AspNetCore.SignalR.Client.HubConnection.KeepAliveInterval> (default: 15 seconds) on the <xref:Microsoft.AspNetCore.SignalR.Client.HubConnectionBuilder>. Set the <xref:Microsoft.AspNetCore.SignalR.Client.HubConnection.HandshakeTimeout> (default: 15 seconds) on the built <xref:Microsoft.AspNetCore.SignalR.Client.HubConnection>. The following example, based on the `Index` component in the [SignalR with Blazor tutorial](xref:blazor/tutorials/signalr-blazor), shows the assignment of default values:
Copy link
Member

Choose a reason for hiding this comment

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

@javiercn , was it this chat tutorial that we wanted to delete from docs?

@guardrex
Copy link
Collaborator Author

guardrex commented Jun 2, 2023

@surayya-MS ... I was working through the changes, and I need to double-check this one ...

Let's remove the following section. This was a workaround to set serverTimeoutInMilliseconds and keepAliveIntervalInMilliseconds

... because if we do that it isn't just this section. The workaround is also part of the Global deployment and connection failures section, and it's going to get chopped for that coverage prior to 8.0, too. We'll be telling devs for <8.0 that they need to change it, but they can't because we don't support the workaround.

Also note that I don't follow the opening paragraph instructions ("rephrase"/"replace" but "keep") ... Do you mean that in the text only the names should change for 8.0 or later? ... and "actually are" implies that the current text is incorrect, but I think this was all reviewed by Javier (in ancient Blazor history 👴😆).

If the changes are complex, it might be best if you make GH suggestions for the exact line line updates.

UPDATE: I'll make the obvious changes on my next commit ...... Done!

@surayya-MS
Copy link
Member

surayya-MS commented Jun 2, 2023

... because if we do that it isn't just this section. The workaround is also part of the Global deployment and connection failures section, and it's going to get chopped for that coverage prior to 8.0, too. We'll be telling devs for <8.0 that they need to change it, but they can't because we don't support the workaround.

@guardrex sorry I previously missed the <8.0 part. I assumed that this is for aspnetcore-8.0. You are right we shouldn't remove it then.

Also note that I don't follow the opening paragraph instructions ("rephrase"/"replace" but "keep") ... Do you mean that in the text only the names should change for 8.0 or later? ... and "actually are" implies that the current text is incorrect, but I think this was all reviewed by Javier (in ancient Blazor history 👴😆).

I meant let's replace descriptions for serverTimeoutInMilliseconds and keepAliveIntervalInMilliseconds with descriptions for withServerTimeout and withKeepAliveInterval. And in there describe what serverTimeoutInMilliseconds and keepAliveIntervalInMilliseconds actually are. No, it wasn't implied that serverTimeoutInMilliseconds and keepAliveIntervalInMilliseconds are wrong. I just meant it would be better to describe what withServerTimeout and withKeepAliveInterval are setting exactly.

I'll write a follow up comment for that

Update: No follow up comments. Looks good to me!

@guardrex
Copy link
Collaborator Author

guardrex commented Jun 2, 2023

Note that the last commit is just a tiny NIT ... I'm merely hyphenating "Keep-Alive" for consistency. That's all that's on the commit.

Copy link
Member

@surayya-MS surayya-MS left a comment

Choose a reason for hiding this comment

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

Looks great!

@guardrex
Copy link
Collaborator Author

guardrex commented Jun 2, 2023

I didn't change the first part tho ... the list of two API items (serverTimeoutInMilliseconds and keepAliveIntervalInMilliseconds). Do you want to supply the text, or do you want me to try and fix that and ping u back?

@guardrex
Copy link
Collaborator Author

guardrex commented Jun 2, 2023

Oh! I see ........

Update: No follow up comments. Looks good to me!

Gotcha! Ok, good to merge then! 🎉

@guardrex
Copy link
Collaborator Author

guardrex commented Jun 2, 2023

... but I'll circle around later if you/Javier intend to pull the Blazor-SignalR tutorial from the doc set per #29297 (comment).

@surayya-MS
Copy link
Member

I didn't change the first part tho ... the paragraph. Do you want to supply the text, or do you want me to try and fix that and ping u back?

No need to change the paragraph because of the workaround example for > 8.0. The descriptions for serverTimeoutInMilliseconds and keepAliveIntervalInMilliseconds are needed there.

@guardrex
Copy link
Collaborator Author

guardrex commented Jun 2, 2023

No need to change the paragraph because of the workaround example for > 8.0. The descriptions for serverTimeoutInMilliseconds and keepAliveIntervalInMilliseconds are needed there.

Yes ... but we might want to organize this a bit differently. Perhaps, I should move that API down into the < 8.0 block and have a different version of it that only explains withServerTimeout and withKeepAliveInterval in the 8.0 or later block.

Let me put that on a commit and you can see how it composes that way. Stand-by ..................................

@guardrex
Copy link
Collaborator Author

guardrex commented Jun 2, 2023

The last commit ...

  • Groups all of the 8.0 or later content together and groups all of the <8.0 content together.
  • Goes with withServerTimeout/withKeepAliveInterval in the >=8.0 content and serverTimeoutInMilliseconds/keepAliveIntervalInMilliseconds in the <8.0 content ... but they both use the same definitions.

@surayya-MS
Copy link
Member

LGTM!

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

Successfully merging this pull request may close these issues.

Server timeout and Keep Alive interval
3 participants