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

Blazor Server - Auto reconnect on mobile browsers #32122

Conversation

konradbartecki
Copy link

@konradbartecki konradbartecki commented Apr 23, 2021

  • You've read the Contributor Guide and Code of Conduct.
  • You've included unit or integration tests for your change, where applicable.
  • You've included inline docs for your change, where applicable.
  • There's an open issue for the PR that you are making. If you'd like to propose a new feature or change, please open an issue to discuss the change or find an existing issue.

Blazor Server - Auto reconnect on mobile browsers

  • On mobile devices when resuming a page after a long time this PR fixes an issue where Rejected prompt was displayed because circuit was already recycled on server.
  • New onConnectionRejected event
  • Default implementation of that event reloads the page (configurable in options)
  • Changed default reconnection delay from 20s to 3s
  • Now reconnection delay will be applied after the attempt instead of before the attempt

Addresses #32113 #26985
Possibly adresses #23340
Related #10325

@konradbartecki konradbartecki requested a review from a team as a code owner April 23, 2021 21:42
@ghost ghost added area-blazor Includes: Blazor, Razor Components community-contribution Indicates that the PR has been added by a community member labels Apr 23, 2021
@dnfadmin
Copy link

dnfadmin commented Apr 23, 2021

CLA assistant check
All CLA requirements met.

@konradbartecki
Copy link
Author

konradbartecki commented Apr 23, 2021

There is an issue, when navigating away the reconnection modal is displayed for fraction of a second. I'll wait for comments how do we want to tackle that before updating this PR.

Maybe I will need to dig around

  window['Blazor']._internal.navigationManager.listenForNavigationEvents((uri: string, intercepted: boolean): Promise<void> => {
    return connection.send('OnLocationChanged', uri, intercepted);
  });

@mkArtakMSFT
Copy link
Member

mkArtakMSFT commented Jan 16, 2022

Sorry for taking too long to get back to you here, @konradbartecki.
@javiercn can you please review this PR and see whether this is something we'd like to take or not? Thanks!

@konradbartecki
Copy link
Author

Sure, @mkArtakMSFT @javiercn, no worries, let me know if anything I can hop on a quick 15 min call with you guys if you would want me to explain it.

};

class ReconnectionProcess {
static readonly MaximumFirstRetryInterval = 3000;
Copy link
Member

Choose a reason for hiding this comment

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

Does anyone remember why we implemented this originally?

Copy link
Author

Choose a reason for hiding this comment

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

Most likely because after connection goes down it would try to attempt the reconnection only after 20s, which is quite a long time.
So workaround that originally is there is to shorten the first reconnection attempt interval to 3s.

}

export interface ReconnectionHandler {
onConnectionDown(options: ReconnectionOptions, error?: Error): void;
onConnectionUp(): void;
onConnectionRejected(options: ReconnectionOptions): void;
Copy link
Member

Choose a reason for hiding this comment

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

This seems like a pretty good idea to me. I'm not 100% sold on reloading automatically by default (we need to at least reason about why that wasn't already our default) but the ability to configure a handler for this scenario seems valuable.

konradbartecki and others added 2 commits January 18, 2022 12:25
Co-authored-by: Steve Sanderson <SteveSandersonMS@users.noreply.github.com>
const delayDuration = i == 0 && options.retryIntervalMilliseconds > ReconnectionProcess.MaximumFirstRetryInterval
? ReconnectionProcess.MaximumFirstRetryInterval
: options.retryIntervalMilliseconds;
await this.delay(delayDuration);
Copy link
Member

Choose a reason for hiding this comment

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

Since retryIntervalMilliseconds was put back to 20000ms, I think it's also necessary to bring back this logic.

}

export interface ReconnectionHandler {
onConnectionDown(options: ReconnectionOptions, error?: Error): void;
onConnectionUp(): void;
onConnectionRejected(options: ReconnectionOptions): void;
Copy link
Member

Choose a reason for hiding this comment

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

For back-compatibility, this new property needs to be added as optional

@@ -49,6 +49,7 @@ async function boot(userOptions?: Partial<CircuitStartOptions>): Promise<void> {
const reconnection = existingConnection || await initializeConnection(options, logger, circuit);
if (!(await circuit.reconnect(reconnection))) {
logger.log(LogLevel.Information, 'Reconnection attempt to the circuit was rejected by the server. This may indicate that the associated state is no longer available on the server.');
options.reconnectionHandler!.onConnectionRejected(options.reconnectionOptions);
Copy link
Member

Choose a reason for hiding this comment

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

I suspect we should be checking whether reconnectionHandler or onConnectionRejected are unset, and skipping the call if so. Or is there some way we know for sure that both of them are set?

Example (untested):

Suggested change
options.reconnectionHandler!.onConnectionRejected(options.reconnectionOptions);
options.reconnectionHandler?.onConnectionRejected?(options.reconnectionOptions);

@SteveSandersonMS SteveSandersonMS added the pr: pending author input For automation. Specifically separate from Needs: Author Feedback label Jan 24, 2022
@ghost
Copy link

ghost commented Feb 3, 2022

Hi @konradbartecki.
It seems you haven't touched this PR for the last two weeks. To avoid accumulating old PRs, we're marking it as stale. As a result, it will be closed if no further activity occurs within 4 days of this comment. You can learn more about our Issue Management Policies here.

@ghost ghost added the stale Indicates a stale issue. These issues will be closed automatically soon. label Feb 3, 2022
@ghost ghost closed this Feb 7, 2022
konradbartecki added a commit to konradbartecki/aspnetcore that referenced this pull request Oct 23, 2022
… comments

Optional onCircuitRejected call if implemented
This pull request was closed.
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 community-contribution Indicates that the PR has been added by a community member pr: pending author input For automation. Specifically separate from Needs: Author Feedback stale Indicates a stale issue. These issues will be closed automatically soon.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants