dotnet / aspnetcore

ASP.NET Core is a cross-platform .NET framework for building modern cloud-based web applications on Windows, Mac, or Linux.
https://asp.net
MIT License
35.42k stars 10.01k forks source link

Tasks returned from JSRuntime.InvokeAsync during OnInitAsync never finishes in preview3 #8274

Closed kvantetore closed 5 years ago

kvantetore commented 5 years ago

Describe the bug

Tasks returned from JSRuntime.InvokeAsync during second call to OnInitAsync (after prerendering) never finishes.

To Reproduce

Create a new razorcomponents project.

Add a js interop function to Index.cshtml and disable prerendering

    <script>
        window.interopDuringOnInit = function() {
            return "Hello World";
        }
    </script>

    <app></app>

    <script src="_framework/components.server.js"></script>

Call js interop during OnAsyncInit in Index.razor

<h1>@Greeting!</h1>

Welcome to your new app.

@functions {
    string Greeting;

    protected override async Task OnInitAsync()
    {
        try
        {
            Console.WriteLine("Calling browser");
            Greeting = await JSRuntime.InvokeAsync<string>("interopDuringOnInit");
            Console.WriteLine("Returned from browser");
        }
        catch (Exception ex)
        {
            Console.WriteLine("Could not invoke interop during OnInit, " + ex.ToString());
        }
    }
}

When OnInitAsync is called as part of an initial load (prerendering disabled) the task never finishes, This leaves the circuit hanging, and no further events are processed.

If this component is loaded from a IUriHelper.OnLocationChange, the above code works as expected.

springy76 commented 5 years ago

I just switched an existing app which worked fine to the new version and got the same problem: IJSRuntime.InvokeAsync() neither returns nor throws an error on call paths from OnInitAsync().

Is there a workaround or did I do something fundamentally wrong? (which I don't believe because the same app worked since Blazor 0.3 and did every version upgrade, got converted to server side Razor Components weeks ago.)

DNF-SaS commented 5 years ago

Same probleme here - the await for JSRuntime.InvokeAsync() in an component's OnInitAsync() does block forever.

Knudel commented 5 years ago

I migrated my code to preview 3 and I also ran into this problem. Please fix this!

DNF-SaS commented 5 years ago

Does anybody has news / a workaround for this issue?

mkArtakMSFT commented 5 years ago

@javiercn, can you please look into this? Thanks!

KP-WhatEver commented 5 years ago

I also got that problem. Anything worked fine before.

kvantetore commented 5 years ago

I have hacked around it by wrapping wrapping the router component in an if (Started), where Started is set to true after a round trip to the client. It's not exactly beautiful (and it breaks prerendering), but it unblocks the issue for me until the underlying issue is fixed.

In App.razor, delay loading the router until we have received a message from the client

@if (Started)
{
    <Router AppAssembly="typeof(Startup).Assembly" />
}
else
{
    <text>Waiting for Asp.Net Core 3.0 Preview3 Workaround...</text>
}

@functions {
    bool Started = false;

    [JSInvokable]
    public void Start()
    {
        Started = true;
        StateHasChanged();
    }

    protected override void OnInit()
    {
        _ = JSRuntime.InvokeAsync<object>("StartMe", new DotNetObjectRef(this));
    }
}

And adding window.StartMe to index.cshtml, calling back to the App component

<script>
    window.StartMe = function (dotnetHelper) {
        dotnetHelper.invokeMethodAsync('Start');
    }
</script>
vertonghenb commented 5 years ago

Not only does this happen on OnInitAsync but also on OnParametersSetAsync. Using it on a ButtonClick does return.

plasticalligator commented 5 years ago

I've confirmed that it happens on all of the lifecycle events; the task never returns. Is there a work around that doesn't break prerendering?

vertonghenb commented 5 years ago

@honkmother I'm afraid that using any JS interop is not allowed in prerendering. (not related to the bug though)

plasticalligator commented 5 years ago

I am not calling it during prerendering.

https://github.com/SQL-MisterMagoo/PreRenderComponent allows us to detect when the prerendering is active and avoid making JS Interop calls during it.

plasticalligator commented 5 years ago

@kvantetore I found an alternative work-around. It would appear if you call the InvokeAsync inside of the body of a component rather than the lifecycle methods it gets properly executed. I don't know exactly when it is executed, but I imagine it is right prior to OnAfterRender.

ex. at the end of Component.razor, prior to @functions { ... }

@ { if (IsPreRendering == false) await JSRuntime.InvokeAsync<string>("interopDuringOnInit"); }

mkArtakMSFT commented 5 years ago

@javiercn can you please confirm whether this is fixed already?

javiercn commented 5 years ago

@mkArtakMSFT It is not.

chassq commented 5 years ago

Jus to add we see the issue as well. The javascript methods below just do console.log. When we use await the second method does not run but if we remove the await then they both run.

In our LayoutMain.cshtml file

    protected override async Task OnInitAsync()
    {
        base.OnInit();

        //Call any js event listener setup. Do this here because need to make sure the .NET side is loaded in the browser.
        await jsRuntime.InvokeAsync<bool>("initScreenFull");
        await jsRuntime.InvokeAsync<bool>("initializeListeners");

    }
javiercn commented 5 years ago

@SteveSandersonMS I am able to repro this. I'm not sure what's going on here. I can see the method being invoked, but I don't get to receive anything on the server.

plasticalligator commented 5 years ago

As far as I can remember in Razor Components the method will be invoked (and can be breakpointed) in the server but will exit from its thread prematurely and never finish websocket response.       On Tuesday, March 26, 2019, 5:43:29 AM GMT+9, Javier Calvarro Nelson notifications@github.com wrote:

@SteveSandersonMS I am able to repro this. I'm not sure what's going on here. I can see the method being invoked, but I don't get to receive anything on the server.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

stavroskasidis commented 5 years ago

I have also reproduced this on Blazor 0.9.0 during OnParametersSetAsync

javiercn commented 5 years ago

I have also reproduced this on Blazor 0.9.0 during OnParametersSetAsync

Hmm, that's very informative. I thought this might have been due to some changes we did on compnents.server for preview3, but if it happens also on blazor then it might mean that is something at a lower level.

SteveSandersonMS commented 5 years ago

I am able to repro this. I'm not sure what's going on here. I can see the method being invoked, but I don't get to receive anything on the server.

OK, I'm guessing that means you're still investigating, but if you feel you're at a dead end please let me know.

stavroskasidis commented 5 years ago

Here is how I reproduced it on Blazor 0.9

  1. Create a new Blazor app
  2. Create the following component: MyComponent.cshtml
    
    <h1>My Component</h1>

@functions{ [Parameter] EventCallback ParametersSetHandler { get; set; } protected async override Task OnParametersSetAsync() { if (ParametersSetHandler.HasDelegate) { await ParametersSetHandler.InvokeAsync(null); } } }

3. Add the following script in `index.html`:
4. Consume the component in the `Counter.cshtml` page like so:

@inject IJSRuntime jsRuntime

@functions{ protected async Task ParametersSetHandler() { var result = await jsRuntime.InvokeAsync("getVal"); Console.WriteLine($"Result: {result}"); } }


5. Run the app and navigate to `/counter` page

**Result:**
The browser is stuck, like being in an endless loop, never finishing the task.

**Additional:**
Adding a `debugger;` before the `return 1;` in the javascript code, shows that the code is called again and again.
SteveSandersonMS commented 5 years ago

@stavroskasidis I don't think your repro code is related to this issue. The code you've posted has a bug that will cause an infinite loop: each time MyComponent calls ParametersSetHandler.InvokeAsync(null), that will cause the owner of ParametersSetHandler (i.e., its parent) to re-render. Its parent will then re-render the child, which immediately tells the parent to re-render again, and so on.

stavroskasidis commented 5 years ago

@SteveSandersonMS Oh... ok, that makes sense. Sorry for the inaccurate report then.

However what would be a workaround for this case, having an EventCallback that is executed inside OnParametersSetAsync? Isn't it easy to fall into this trap?

SteveSandersonMS commented 5 years ago

However what would be a workaround for this case

An EventCallback is to notify a component that an event has occurred and it should re-render. Since that doesn't seem to be what you want in this case, don't use an EventCallback.

If you have further questions about this, could you please file them in a separate issue? That will help keep this one on topic.

SteveSandersonMS commented 5 years ago

I've tracked this down. It's among the changes we made for circuit lifetime and prerendering. What happens is:

Proposed solution: StartCircuit should not wait for quiescence. It should only wait for the initial render.

For example, we change this line of code: https://github.com/aspnet/AspNetCore/blob/master/src/Components/Server/src/Circuits/CircuitHost.cs#L116 so that it doesn't await, but rather just kicks off the AddComponentAsync task in the background (and deals with error handling asynchronously by some typical mechanism).

@javiercn @pranavkm @rynowak Do any of you have any concerns or disagreement with this solution?

SteveSandersonMS commented 5 years ago

Actually the right fix might just be to guarantee we never pause the SignalR message loop.

rynowak commented 5 years ago

Since we have our own sync context I agree with all of the above.

The reason to pause the message loop is to guarantee ordering. We implement ordering ourselves.

davidfowl commented 5 years ago

I was going to comment on this but I was on vacation 😄. Sorry you had to spend your time investigating this @SteveSandersonMS. There's an issue in SignalR to allow a maximum number of concurrent invocations (it should really be more than 1), that should help solve this issue.

cc @anurse @BrennanConroy @halter73

javiercn commented 5 years ago

Proposed solution: StartCircuit should not wait for quiescence. It should only wait for the initial render.

Agree. It should not wait at all, and deal with errors in the way we do in other places. We should just wrap the task to get notified and that's it.

Once StartCircuit has been called the connection must be up and running already so UpdateDisplay should work just fine.

analogrelay commented 5 years ago

There's an issue in SignalR to allow a maximum number of concurrent invocations (it should really be more than 1), that should help solve this issue.

https://github.com/aspnet/AspNetCore/issues/5351

If this is important for Razor Components, please let us know so we can prioritize appropriately.

SteveSandersonMS commented 5 years ago

Turns out that in general we don't block the SignalR message loop. In all cases except one we just hand off to the circuit's dispatcher and the call is synchronous from SignalR's perspective.

The one bad case was StartCircuit which did not do this. I've fixed this in #8863 by making CircuitHost.Initialize synchronous. The upstream caller is free to continue handling other incoming messages as soon as CircuitHost.Initialize has dispatched the "init" logic to the renderer's sync context, because that's then guaranteed to run and complete before anything else gets handled.

SteveSandersonMS commented 5 years ago

@davidfowl Thanks for the info. I wasn't regarding this as a SignalR issue since, even if SignalR did allow concurrent hub method invocations, we'd have had a different problem where you could have received incoming JSInterop calls before the circuit was flagged as started. The underlying false assumption is now fixed in #8863 so neither problem occurs.

Totally agree it would be good for SignalR to have the flexibility to handle parallel messages in general though.

Bernardo-Castilho commented 5 years ago

Has this been fixed yet? I can reproduce this using the latest public version.

This returns a Task that never ends: var cnt = JSRuntime.InvokeAsync("interop.primes", n);

This does not return at all, no matter where I call it from. var cnt = await JSRuntime.InvokeAsync("interop.primes", n);

Unless I am missing something, this means I can't get results back from JavaScript at all.

cosXsinX commented 4 years ago

Actually to understand what happen when trying to launch a task on Blazor you should understand JavaScript execution lifecycle.

To sum up, any task in a JavaScript environment in the context of the browser (Safari, Chrome , Edge, ...) cannot be executed when the current task is running. That means you are dead locking when trying to wait the initiated task by JSRuntime.InvokeAsync<...>(“functionName”,...).AsTask().Wait() in a “synchronous” Blazor function.

The solution is to always perform those task calls in an async containing function and call StateIsChanged() function to say to the browser that the model was updated and that it should refresh the DOM

gbukauskas commented 4 years ago

I got the same problem calling Google Maps API in component's Dispose method (I need to remove markers on map). I solved the problem setting limit of waiting time:

public void Dispose()
{
    try
    {
        var tasks = new Task[2]
        {
            Task.Run(async () => await GoogleMapInterop.RemoveAllMarkers()),
            Task.Run(async () => await GoogleMapInterop.RemoveAllPaths())
        };
        Task.WaitAll(tasks, TimeSpan.FromSeconds(1));
    }
    catch (Exception)
    {
        // Ignore any bugs
    }
}

Error occurs after Refresh page (F5). There is no workaround thus I had to hide it.

springy76 commented 4 years ago

Doesn't Blazor support IAsyncDisposable? Task.WaitAll feels so aged.

gbukauskas commented 4 years ago

This method is called from Dispose: I need to call GooleMapsAPI for removing markers and paths from map. All works fine except page refresh (F5). Do you know another way? Please tell it me.

chucker commented 4 years ago

Doesn't Blazor support IAsyncDisposable? Task.WaitAll feels so aged.

Not yet.

gbukauskas commented 4 years ago

And where is problem? One can use any operator or expression unless it marked with 'obsolete'. It would be very strange if someone will state that 'foreach' is too old and only asynchronous version of the operator is legal. I proposed the solution write your one if it works better.

2019-12-17, an, 16:17 Sören Nils Kuklau notifications@github.com rašė:

Doesn't Blazor support IAsyncDisposable? Task.WaitAll feels so aged.

Not yet. https://github.com/aspnet/AspNetCore/issues/9960

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/aspnet/AspNetCore/issues/8274?email_source=notifications&email_token=AADHFL7NGZBVEK4RDS5CMEDQZDNPFA5CNFSM4G4LVJM2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEHCQGFI#issuecomment-566559509, or unsubscribe https://github.com/notifications/unsubscribe-auth/AADHFL7LLUAJEUOKOOM6R73QZDNPFANCNFSM4G4LVJMQ .

-- Gediminas Bukauskas SKYPE: gediminasbuk Phone: +370 37 793622 Mobile: +370 611 31090 E-mail: gbukauskas@gmail.com

pranavkm commented 4 years ago

Hi, it looks like you are posting on a closed issue/PR/commit!

We're very likely to lose track of your bug/feedback/question unless you:

  1. Open a new issue
  2. Explain very clearly what you need help with
  3. If you think you have found a bug, include detailed repro steps so that we can investigate the problem

Thanks!