dotnet / AspNetCore.Docs

Documentation for ASP.NET Core
https://docs.microsoft.com/aspnet/core
Creative Commons Attribution 4.0 International
12.63k stars 25.29k forks source link

Provide clarity regarding Server-side blazor SignalR Keep-Alive & Client Timeout #30136

Closed garrettlondon1 closed 6 months ago

garrettlondon1 commented 1 year ago

Important The Keep-Alive interval (keepAliveIntervalInMilliseconds or KeepAliveInterval) isn't directly related to the reconnection UI appearing. The Keep-Alive interval doesn't necessarily need to be changed. If the reconnection UI appearance issue is due to timeouts, the server timeout can be increased and the Keep-Alive interval can remain the same. The important consideration is that if you change the Keep-Alive interval, make sure that the timeout value is at least double the value of the Keep-Alive interval and that the Keep-Alive interval on the server matches the client setting.

The KeepAliveInterval isn't directly related to the reconnection UI appearing.

I think these two ways of phrasing the situation around Blazor Server and the "Reconnecting" modal is confusing with words like "isn't directly related", "doesn't necessarily need to be changed".. What is the exact SignalR logic for deciding whether the Reconnecting modal appears and to what extent can Blazor Server developers control this to fit their specific use case.


Document Details

Do not edit this section. It is required for learn.microsoft.com ➟ GitHub issue linking.

garrettlondon1 commented 1 year ago

Related https://github.com/dotnet/aspnetcore/issues/48724

guardrex commented 1 year ago

Thanks, @garrettlondon1.

cc: @mkArtakMSFT

@danroth27 ... I see you agreed on the phrasing. That phrasing was provided (not necessarily for publication) by Brennan in a remark here ...

https://github.com/dotnet/AspNetCore.Docs/pull/27197#issuecomment-1270345517

... so perhaps I can work with Brennan to improve those remarks. However, do you want to table this for now and work on it later when .NET 8 work cools off of bit, or should we ping him now?

Brennan ... when you 👀 this ...

Can we remove the words "directly" and "necessarily," along with shortening the Keep-Alive comment, to firm up the concept that the Keep-Alive isn't related to the reconnection UI appearing? If so, a revision might go like ...

The Keep-Alive interval doesn't need to be changed to configure how long the app waits before showing the reconnection UI. If the reconnection UI appearance is due to timeouts, the ClientTimeoutInterval and HandshakeTimeout can be increased, and the KeepAliveInterval can remain the same. The important consideration is that if you change the Keep-Alive interval, make sure that the client timeout value is at least double the value of the Keep-Alive interval and that the Keep-Alive interval on the client matches the server setting.

Also, I'll also check again on the split between the Fundamentals > SignalR coverage and Host and Deploy > Blazor Server coverage. We want remarks logically placed, not duplicated, and cross-linked where needed. The fundamentals coverage is at ...

https://learn.microsoft.com/en-us/aspnet/core/blazor/fundamentals/signalr?view=aspnetcore-7.0#configure-signalr-timeouts-and-keep-alive-on-the-client

... and 🦖 NOTE TO SELF 🦖 ... probably need to cross-link from the section that only discusses the reconnection UI to where we discuss config that extends the timeouts ...

https://learn.microsoft.com/en-us/aspnet/core/blazor/fundamentals/signalr?view=aspnetcore-7.0#reflect-the-server-side-connection-state-in-the-ui

danroth27 commented 1 year ago

I can work with Brennan to improve those remarks

Yeah, after seeing this user feedback and reviewing the text the timeout guidance seems like it could be clarified and simplified. For example, if the Keep-Alive interval isn't related to the reconnection UI, then why is it mentioned here? It sounds like we also need better guidance on how to make the reconnection UI show up in a timely fashion. I think we should work with @javiercn and @halter73 on this.

do you want to table this for now and work on it later when .NET 8 work cools off of bit?

Yes, the .NET 8 docs work takes priority right now.

guardrex commented 1 year ago

why is it mentioned here?

I think perhaps because ...

Also, this bit ... this whole IMPORTANT note ... is an in-passing informational message about the server timeouts, of which Keep-Alive is one that has to be discussed generally. This note contains a clarifying remark about the reconnection UI, but that's not really what this whole section is about. When I assess how this is covered, I might be able to improve how its laid out and cross-linked.

https://learn.microsoft.com/en-us/aspnet/core/blazor/host-and-deploy/server?view=aspnetcore-7.0#global-deployment-and-connection-failures

Yes, the .NET 8 docs work takes priority right now.

I'll ping him later ...... even the lowly 🦖 is buried in JS interop full-stack UI updates at the moment ⛏️🗻😅.

guardrex commented 7 months ago

@garrettlondon1 ... I finally 😅 reached this. It's been a grueling year of major updates to cover BWAs/.NET 8.

I understand from the PU issue that you opened that they've marked it for for work for .NET 10 (2025), and I understand that you wished for a higher priority. At least on the docs side, we can make adjustments now to improve the content.

I'd like to do this in two steps (two PRs) ...

I'm going to issue the first PR and go ahead and merge it this morning, and then I'll get into the second PR and ping for the PU ... "product unit" btw ... ping the PU tomorrow to look at what I come up with for the second PR.

If you have any further remarks, you can apply them here. The PR that I'm about to set up will go in immediately, so we won't have time there to discuss anything. It's going to focus on the content organization and striking those foul sentences 😈.

garrettlondon1 commented 7 months ago
  • you're asking for showing the reconnection UI faster ... sooner ... and that's still under consideration on the PU issue AFAICT.

You are correct here. I explain Blazor Server to people like a video game, because that's exactly what the Websocket connection is.

Much appreciate the update, and looking forward to clarifying these on the docs!

The way that DOM updates are built up and then sent, all at once, upon a reconnection, is one of the most important things to note.

It's kind of like if you were a kid at an arcade, put $1 in the game, press the button, nothing happens, so you naturally keep pressing the button.

guardrex commented 7 months ago

At a very minimum, we can include a section on it and link the PU issue. One of the updates that I'm making now is taking that current guidance and giving it the explicit section title of Delay the appearance of the reconnection UI. That might be temporary because the 2nd PR tomorrow might make further content layout and section changes ... in fact, I think it's likely that further changes will be made tomorrow.

I haven't analyzed the full PU issue yet. Question ❓ .... The current guidance is about delaying the appearance of the UI by changing the ClientTimeoutInterval and the HandshakeTimeout on the server and changing withServerTimeout on the client ... IF I'm understanding the PU's remarks thus far. If that's right, what happens if these values are cut in HALF from the default values? That doesn't speed up the appearance of the reconnection UI? I just mean logically that would be the effect on face value from their guidance thus far.

garrettlondon1 commented 7 months ago

If that's right, what happens if these values are cut in HALF from the default values? That doesn't speed up the appearance of the reconnection UI? I just mean logically that would be the effect on face value from their guidance thus far.

That's a great question: and I don't 100% know the answer. From my testing, it does cut the reconnection modal appearing in half. But, due to the guidance of both of those values "not necessarily" effecting the reconnection modal, I'm not positive.

For a while, I set the keep-alive to 1 or 2 seconds, but I decided against it in Production. If I disconnected, the reconnection modal would appear very quickly.

The only confusing point in the websocket connection is when the websocket goes from "connected" state > "reconnecting", before the reconnection UI is shown. I can't even find a JS method in the ReconnectHandler to log when that happens

guardrex commented 7 months ago

I see.

I want to do some testing on this, too. What I might do on the 2nd PR is refactor away from "only delay" to content that reads more like "control the timing of" (i.e., increase or decrease the period). Then, Brennan can look at that and say if my hunches are correct and if the new language is good.

WRT an immediate display of the reconnection UI, I think they'll say that it isn't possible ... which is part of the PU issue, right? Again, I haven't read it in detail yet. I'm just trying to make the basic content improvements at the moment. I'll dig into that issue shortly.

I agree that the "not necessarily" language was a poor choice of words to take from Brennan's discussion remarks. I think Brennan was saying that they are indirectly related such that ...

if you change the Keep-Alive interval, make sure that the timeout value is at least double the value of the Keep-Alive interval and that the Keep-Alive interval on the server matches the client setting.

The fix that I'm making now to remove the two confusing sentences hopefully will take care of that problem. We'll see on review of the 2nd PR tomorrow.

guardrex commented 7 months ago

Thus far, I've cleaned up the organization. The content on "delaying" the reconnection UI is now in a better spot in the SignalR article with the rest of the SignalR configuration and reconnection UI guidance.

https://learn.microsoft.com/en-us/aspnet/core/blazor/fundamentals/signalr?view=aspnetcore-8.0#delay-the-appearance-of-the-reconnection-ui

Next, I'll see about further updates that will require Brennan to take a look. I'll probably place the PR this morning, sleep on it 🛌 💤 for final updates tomorrow morning, and then ping him to take a look.

guardrex commented 7 months ago

@garrettlondon1 ... I'm done with most of the basic updates (rough draft) on the PR. Testing confirms that cutting the values in half, which keeps configuration in line with Brennan's recommendations, does indeed spawn the reconnection UI in half the time on a lost connection. Therefore, the updates seem good so far.

I don't have a remark in the article yet on possible future enhancements with a cross-link to the PU issue. I'll pick back up with this work Thursday morning and see how that can be phrased and placed.

garrettlondon1 commented 6 months ago

Just reviewed your PR @guardrex .. if we could follow up with @MackinnonBuck regarding the reconnection UI, that would be much appreciated. I still feel like the reconnection UI is not explained at all in the docs.

This ticket was closed, but the most important part remains unanswered: "What is the exact SignalR logic for deciding whether the Reconnecting UI appears and to what extent can Blazor Server developers control this to fit their specific use case."

guardrex commented 6 months ago

@garrettlondon1 ... You're right. The current content only goes this far ...

The timing of the appearance of the reconnection UI is influenced by adjusting the Keep-Alive interval and timeouts on the client.

... and that's not explicit enough. I'll try again after we hear back from Mackinnon.

guardrex commented 6 months ago

I've placed a PR with my best guess based on PU discussion and framework behavior here ... https://github.com/dotnet/AspNetCore.Docs/pull/32331. Let's take up further discussion on the PR when Mackinnon returns.