Skip to content

Improve Blazor reconnection experience. #13015

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
Aug 14, 2019
Merged

Conversation

NTaylorMullen
Copy link

@NTaylorMullen NTaylorMullen commented Aug 9, 2019

  • Updated text of reconnect dialog to be more clear.
  • Added user feedback to the Retry event button click. The current flow is Attempting to reconnect -> Failed to reconnect ... [Retry] -> Click Retry -> Attempting to reconnect.
  • Found that in cases where the server went away entirely the reconnect event would through unexpectedly preventing the reconnect display from handling a failed reconnect. Added a separate error flow to understand when the server went away/could not start a SignalR connection to.
  • Could not find a great way to add tests for this scenario.
  • Fixed all issue requirements except for the last bullet point about auto-reloading. The suggested approach would break development experiences; therefore, going to do a follow up if a better experience can be suggested/is viable.
  • One big issue that still exists is when the circuit gets forgotten or the server restarts entirely. We end up successfully connecting to the server but circuit connection fails resulting in a fast response failure to reconnect (shown in the end of the gif below). However, we do not let the user know that reconnect's are no longer feasible. That would be addressed by something like: Reconnection enhancements #12442 (comment)

This is of me stopping the server, clicking retry a few times, restarting the server, clicking retry, reloading the page.
image

Addresses #12442

logger.log(LogLevel.Information, 'Reconnection attempt failed. Unable to connect to the server.');
return false;
}

if (!(await circuit.reconnect(reconnection))) {
Copy link
Author

Choose a reason for hiding this comment

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

SignalR throws if you attempt to reconnect like this while the connection is not connected. This prevented the reconnect display from properly handling scenarios where the server went away.

Copy link
Author

@NTaylorMullen NTaylorMullen Aug 14, 2019

Choose a reason for hiding this comment

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

Actually, can't remove this entirely. It breaks the expectation of calling code, i.e. in DefaultReconnectionHandler it expects:

        // reconnectCallback will asynchronously return:
        // - true to mean success
        // - false to mean we reached the server, but it rejected the connection (e.g., unknown circuit ID)
        // - exception to mean we didn't reach the server (this can be sync or async)

In a way it makes sense. Basically, we need to have 3 different return values and instead of having an enum the previous design utilized exceptions. I'll revert this change in my next change for this experience but enable the reconnect display to handle exceptions so the UI doesn't die.

@Eilon Eilon added the area-blazor Includes: Blazor, Razor Components label Aug 9, 2019
@NTaylorMullen NTaylorMullen force-pushed the nimullen/12442 branch 3 times, most recently from f2eb0d2 to d1f5dcf Compare August 13, 2019 22:44
@rynowak
Copy link
Member

rynowak commented Aug 14, 2019

One big issue that still exists is when the circuit gets forgotten or the server restarts entirely. We end up successfully connecting to the server but circuit connection fails resulting in a fast response failure to reconnect (shown in the end of the gif below). However, we do not let the user know that reconnect's are no longer feasible. That would be addressed by something like: #12442 (comment)

What's the problem with this precisely? We know that ConnectCircuit fails because it will return false. Is the problem that we have to way to show differnt UI when that happens?

@rynowak
Copy link
Member

rynowak commented Aug 14, 2019

I think these changes look good. We should talk more about how to solve the problem with ConnectCircuit but let's not block this.

@NTaylorMullen
Copy link
Author

Is the problem that we have to way to show differnt UI when that happens?

Oops didn't see that you had responded to this. Yes, here's what I think would be a good alternative: #12442 (comment)

- Updated text of reconnect dialog to be more clear.
- Added user feedback to the `Retry` event button click. The current flow is `Attempting to reconnect` -> `Failed to reconnect ... [Retry]` -> Click Retry -> `Attempting to reconnect`.
- Found that in cases where the server went away entirely the reconnect event would through unexpectedly preventing the reconnect display from handling a failed reconnect. Added a separate error flow to understand when the server went away/could not start a SignalR connection to.
- Could not find a great way to add tests for this scenario.

Addresses #12442
@NTaylorMullen NTaylorMullen merged commit 10452bf into release/3.0 Aug 14, 2019
@ghost ghost deleted the nimullen/12442 branch August 14, 2019 23:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-blazor Includes: Blazor, Razor Components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants