Skip to content

Conversation

konradbartecki
Copy link

Blazor Server - Auto reconnect on mobile browsers

  • 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.

Description

  • 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)
  • Now reconnection delay will be applied after the attempt instead of before the attempt

Re-opens a PR #32122
Fixes #32113 #26985

@konradbartecki konradbartecki requested a review from a team as a code owner October 23, 2022 07:01
@ghost ghost added area-blazor Includes: Blazor, Razor Components community-contribution Indicates that the PR has been added by a community member labels Oct 23, 2022
@ghost
Copy link

ghost commented Oct 23, 2022

Thanks for your PR, @konradbartecki. Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

konradbartecki added a commit to konradbartecki/aspnetcore that referenced this pull request Oct 23, 2022
@mkArtakMSFT
Copy link
Contributor

@TanayParikh can you please review this? Thanks!

@TanayParikh
Copy link
Contributor

Hey @konradbartecki, thanks for reaching out. At this time, I don't believe this is something we'd like to take into the framework. We have a broader undertaking with #30344 which we believe may resolve this issue at a lower level.

cc/ @javiercn @SteveSandersonMS @MackinnonBuck in case you disagree with that assessment.

Thanks for your time!

@konradbartecki
Copy link
Author

konradbartecki commented Oct 26, 2022

I have added a new commit fixing the build and tests, since the work on it was done already anyway. Would be really great to have it in 7.0 as configurable workaround if #30344 does not get into 7.0.

Testing connection rejected scenario

Assuming we continue with this PR I would need an advice on testing the following scenario that is not currently covered:

  1. User's circuit disconnects (for example because a mobile browser is sent to background)
  2. Server registers the circuit as disconnected
  3. Default timespan of 3 minutes passes
  4. Circuit gets evicted aka disposed because 3 minutes has passed.
  5. Client puts the mobile browser into a foreground
  6. Client's circuit attempts to reconnect to the server
  7. Client immediately receives a "connection reject"

// 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)
const result = await this.reconnectCallback();
if (!result) {
// If the server responded and refused to reconnect, stop auto-retrying.
this.reconnectDisplay.rejected();
return;
}

I saw your internal app used for E2E testing and was not sure what was the best way to simulate circuit eviction, basically we should:

  1. Disconnect the circuit from the client's side with Blazor._internal.forceCloseConnection()
  2. Somehow evict the circuit on the server
  3. Attempt to reconnect the circuit from the client's side

My best idea was to add to your BasicTestApp a new DisposeCircuit button:
image

Then add a ScopedCircuitContainer

using System.Reflection;
using Microsoft.AspNetCore.Components.Server.Circuits;

namespace BasicTestApp.Reconnection
{
    public class ScopedCircuitContainer : CircuitHandler
    {
        public ScopedCircuitContainer()
        {
        }

        private Circuit _circuit;

        public override Task OnCircuitOpenedAsync(Circuit circuit, CancellationToken cancellationToken)
        {
            _circuit = circuit;
            return base.OnCircuitOpenedAsync(circuit, cancellationToken);
        }

        public override Task OnCircuitClosedAsync(Circuit circuit, CancellationToken cancellationToken)
        {
            _circuit = circuit;
            return base.OnCircuitClosedAsync(circuit, cancellationToken);
        }

        public void RemoveCircuitFromServer()
        {
            var circuitHost = _circuit
                .GetType()
                .GetField("_circuitHost", BindingFlags.Instance | BindingFlags.NonPublic)
                .GetValue(_circuit);
            var disposeMethod = circuitHost
                .GetType()
                .GetMethod("DisposeAsync");
            disposeMethod.Invoke(circuitHost, Array.Empty<object>());
        }
    }
}

And attach RemoveCircuitFromServer to the button's OnClick event.

I am aware that this is nasty for using reflection, but I did not see any other way around it really and any changes to that code would be detected by a failing test.

CircuitHost.DisposeAsync method did not work on me and it seems that I should somehow get a handle of CircuitRegistry and use eviction methods from there.

I could cover this scenario regardless if you want to merge this branch, because I already did some research on that, so please let me know.

@ghost
Copy link

ghost commented Oct 26, 2022

Hi @konradbartecki. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Better reconnection logic for Blazor Server
3 participants