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

Reconnection enhancements #12442

Closed
SteveSandersonMS opened this issue Jul 22, 2019 · 4 comments
Closed

Reconnection enhancements #12442

SteveSandersonMS opened this issue Jul 22, 2019 · 4 comments
Assignees

Comments

@SteveSandersonMS
Copy link
Member

@SteveSandersonMS SteveSandersonMS commented Jul 22, 2019

Following on from work done for #11999, here are some improvements to reconnection that we'd like to make:

  • Make the "retry" button show some acknowledgement when you click it
    • For example, disable it for 1 second following the click
    • If the manually-initiated retry fails, then change the dialog text to say "Reconnection failed. Try reloading the page if you're unable to reconnect.". It will still have the two options, "[Retry]" and "[Reload page]".
  • Explicitly detect connection failures due to "server says unknown circuit ID", and in those cases, auto-reload the whole page since we know reconnection isn't realistically going to work.
    • We need to avoid infinite refresh loops, e.g., put a flag in sessionStorage to say we did an auto-reload, clear that flag the next time a connection is successful, and don't do an auto-reload any time the flag is still there.

The latter one would give the biggest benefit, as it's useful not only in production but also in development. This is the flow that happens each time you change your code and rebuild the application.

@vertonghenb

This comment has been minimized.

Copy link
Contributor

@vertonghenb vertonghenb commented Aug 7, 2019

This will also solve the problem for iOS webclip "PWA's", which cannot be refreshed by nature.

@NTaylorMullen

This comment has been minimized.

Copy link
Contributor

@NTaylorMullen NTaylorMullen commented Aug 9, 2019

Explicitly detect connection failures due to "server says unknown circuit ID", and in those cases, auto-reload the whole page since we know reconnection isn't realistically going to work.

  • We need to avoid infinite refresh loops, e.g., put a flag in sessionStorage to say we did an auto-reload, clear that flag the next time a connection is successful, and don't do an auto-reload any time the flag is still there.

@SteveSandersonMS / @rynowak this seems like the wrong solution to me. If something bad happens on the client or server and we then start auto-refreshing the page the user will lose all logs/console output from the page due to the refresh.

A good example of this being a poor experience for developers is if a user is doing a specific workflow that breaks the server or client and you enter into this failed reconnection state -> page auto refreshes -> you're now back into a "valid" state;, all the logs for how you got into the original invalid state are now gone unless you manually captured them during the brief time that reconnection was occuring.

A better solution would be to:

  1. Document having your own reconnect display where a failed invocation can be used to force a refresh on the users behalf.
  2. Have a flag that turns this feature on or off.

I lean towards 1 primarily because a user having their own reconnect display will be a must for any proper Blazor server side site. I'm less enthused by 2 because having a rando flag that you turn on-off depending on if you're in production or not (in JS) would add to the complexity.

Thoughts?

@NTaylorMullen

This comment has been minimized.

Copy link
Contributor

@NTaylorMullen NTaylorMullen commented Aug 9, 2019

Another route would be to pass additional information to failed which could indicate that reconnection wont be possible. At which point we could provide the dialog to reload the page (instead of retry)

NTaylorMullen added a commit that referenced this issue 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.

Addresses #12442
NTaylorMullen added a commit that referenced this issue Aug 12, 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.

Addresses #12442
NTaylorMullen added a commit that referenced this issue Aug 13, 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.

Addresses #12442
@NTaylorMullen

This comment has been minimized.

Copy link
Contributor

@NTaylorMullen NTaylorMullen commented Aug 14, 2019

Given the lack of responses here I'm going to just pick something that feels the most "right" to me.

I plan to expand ReconnectDisplay to have a rejected method on it:

export interface ReconnectDisplay {
  show(): void;
  hide(): void;
  failed(): void;
  rejected(): void;
}

And our default implementation of ReconnectDisplay's rejected will provide a nice error message Could not reconnect to the server. <a href>Reload</a> the page to restore functionality.

If you don't like this suggestion please let me know.

NTaylorMullen added a commit that referenced this issue Aug 14, 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.

Addresses #12442
NTaylorMullen added a commit that referenced this issue Aug 14, 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.

Addresses #12442
NTaylorMullen added a commit that referenced this issue Aug 15, 2019
- Expanded `ReconnectDisplay` to have a `refused` method on it. This is the method that indicates we will never be able to reconnect to the server. By default we provide a nice little message letting users know that reconnection is no longer possible and that a refresh must take place.
- Added a logger to the `DefaultReconnectionDisplay` since part of its job is handling `Retry` clicks which indirectly call `reconnect()`. Therefore, it needed the ability to log information to the console to inform users why certain reconnects were not possible.
- Updated the `UserSpecifiedDisplay` to have a `refused` understanding. Added a new CSS class to represent the `refused` state as well.
- Updated existing tests to abide by the new `ReconnectDisplay` structure
- Added a new test to validate that the `refused``ReconnectDisplay` method results in proper behavior.

#12442
NTaylorMullen added a commit that referenced this issue Aug 15, 2019
- Expanded `ReconnectDisplay` to have a `rejected` method on it. This is the method that indicates we will never be able to reconnect to the server. By default we provide a nice little message letting users know that reconnection is no longer possible and that a refresh must take place.
- Added a logger to the `DefaultReconnectionDisplay` since part of its job is handling `Retry` clicks which indirectly call `reconnect()`. Therefore, it needed the ability to log information to the console to inform users why certain reconnects were not possible.
- Updated the `UserSpecifiedDisplay` to have a `refused` understanding. Added a new CSS class to represent the `refused` state as well.
- Updated existing tests to abide by the new `ReconnectDisplay` structure
- Added a new test to validate that the `refused``ReconnectDisplay` method results in proper behavior.

#12442
NTaylorMullen added a commit that referenced this issue Aug 15, 2019
- Expanded `ReconnectDisplay` to have a `rejected` method on it. This is the method that indicates we will never be able to reconnect to the server. By default we provide a nice little message letting users know that reconnection is no longer possible and that a refresh must take place.
- Added a logger to the `DefaultReconnectionDisplay` since part of its job is handling `Retry` clicks which indirectly call `reconnect()`. Therefore, it needed the ability to log information to the console to inform users why certain reconnects were not possible.
- Updated the `UserSpecifiedDisplay` to have a `refused` understanding. Added a new CSS class to represent the `refused` state as well.
- Updated existing tests to abide by the new `ReconnectDisplay` structure
- Added a new test to validate that the `refused``ReconnectDisplay` method results in proper behavior.

#12442
@NTaylorMullen NTaylorMullen added Done and removed Working labels Aug 15, 2019
Blazor automation moved this from In progress to Done Aug 15, 2019
@msftbot msftbot bot locked as resolved and limited conversation to collaborators Dec 3, 2019
@jaredpar jaredpar removed this from Done in Blazor Jan 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
4 participants
You can’t perform that action at this time.