Skip to content

Prevent JS interop after circuit disposal. Fixes #32808#32901

Merged
SteveSandersonMS merged 5 commits into
mainfrom
stevesa/fix-async-disposal-and-jsinterop
May 25, 2021
Merged

Prevent JS interop after circuit disposal. Fixes #32808#32901
SteveSandersonMS merged 5 commits into
mainfrom
stevesa/fix-async-disposal-and-jsinterop

Conversation

@SteveSandersonMS
Copy link
Copy Markdown
Member

See more details about the problem at #32808 (comment). In summary, any component that attempts JS interop during disposal (for example, <Virtualize>) would hang until the JS call times out, preventing cleanup of any scoped resources for that period.

This PR changes how JS interop works after a circuit has begun disposal (which typically means the user has closed their browser tab or navigated away):

  • Before: JS interop calls would not get any response from the browser, so they'd just hang for a minute and eventually time out with an exception
  • After: JS interop calls will throw immediately using a new exception type (JSDisconnectedException)

I think we can regard this as not a breaking change because the calls would have eventually thrown anyway, and still do. It now just completes this process faster. The advantages of this change are:

  1. DI scopes no longer live for an extra minute before cleanup in this case (e.g., when you have an active <Virtualize> component and the user closes the tab). They now clean up immediately.
  2. Component authors who want to treat this as not-an-error now have a specific exception type they can catch and suppress. This is now done inside Virtualize so it won't even get logged as an error. This makes sense in cases where you were only trying to do JS interop to clean up browser-side state, and obviously don't care about that if the browser is already gone.

If we wanted, we could consider more dramatic options like:

  • Not treating JS interop calls as failing with exceptions if the browser is disconnected. I think that's a bad idea because (1) it would be hugely breaking, and (2) semantically it doesn't make sense - we won't know if the developer actually requires some info back from the browser or wants to know for sure whether a void call was received and completed.
  • Auto-suppressing JSDisconnectedException entirely from logs. I think that's a bad idea because it likely prevents developers from discovering that one was even happening. And then if their disposal logic looked like DoJsCleanUp(); DoDotNetCleanUp();, they would not realise that the DoDotNetCleanUp part never executes. I think developers need to know this is an error and it's up to them to try/catch if they want to control how it's handled.

@SteveSandersonMS SteveSandersonMS requested a review from a team May 21, 2021 13:20
@SteveSandersonMS SteveSandersonMS requested a review from Pilchie as a code owner May 21, 2021 13:20
@Pilchie Pilchie added the area-blazor Includes: Blazor, Razor Components label May 21, 2021
Comment thread src/Components/Server/src/Circuits/RemoteJSRuntime.cs
Copy link
Copy Markdown
Contributor

@mkArtakMSFT mkArtakMSFT left a comment

Choose a reason for hiding this comment

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

Comment thread src/Components/Server/src/Circuits/RemoteJSRuntime.cs Outdated
Comment thread src/JSInterop/Microsoft.JSInterop/src/JSDisconnectedException.cs Outdated
@pranavkm pranavkm added the api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews label May 21, 2021
@ghost
Copy link
Copy Markdown

ghost commented May 21, 2021

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

  • The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
  • The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
  • Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

SteveSandersonMS and others added 2 commits May 21, 2021 18:04
Co-authored-by: Pranav K <prkrishn@hotmail.com>
Comment thread src/Components/Server/src/Circuits/RemoteJSRuntime.cs
Comment thread src/Components/Server/src/Circuits/RemoteJSRuntime.cs Outdated
Comment thread src/Components/Server/src/Circuits/RemoteJSRuntime.cs
Co-authored-by: Tanay Parikh <TanayParikh@users.noreply.github.com>
Copy link
Copy Markdown
Member

@javiercn javiercn left a comment

Choose a reason for hiding this comment

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

Changes look great!

Do we need to do anything for calls that might already be in-flight?

Comment thread src/JSInterop/Microsoft.JSInterop/src/JSDisconnectedException.cs Outdated
@BrennanConroy BrennanConroy added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews labels May 24, 2021
@SteveSandersonMS
Copy link
Copy Markdown
Member Author

Do we need to do anything for calls that might already be in-flight?

I considered it, but didn't think it was desirable because:

(1) it's not required to solve any known problem (the call will eventually time out anyway, and isn't keeping the circuit open)
(2) it's more likely to affect people with existing code in ways we haven't predicted
(3) it's trickier to implement since we would have to change a bunch of things about how the timeouts get triggered or get surfaced as exceptions

Overall since it's not required to solve the current problem here, I wasn't inclined to expand the scope of the solution. We could do it in the future if a sufficient need arises.

Comment thread src/JSInterop/Microsoft.JSInterop/src/JSDisconnectedException.cs Outdated
@SteveSandersonMS SteveSandersonMS merged commit 2ccd97a into main May 25, 2021
@SteveSandersonMS SteveSandersonMS deleted the stevesa/fix-async-disposal-and-jsinterop branch May 25, 2021 13:34
@ghost ghost added this to the 6.0-preview6 milestone May 25, 2021
@pos777
Copy link
Copy Markdown

pos777 commented May 28, 2021

Hello @SteveSandersonMS,

I am using the sample from the #32808 (comment) issue with .NET v5.0.6.
If I open the 'Virtualize test' page, then close it, and wait for a while, I get the TaskCanceledException:
image

Assume, I'm developing a custom Virtualize component. Currently, I have to write something like this to avoid this exception:

public async ValueTask DisposeAsync()
{
    try
    {
        if (_selfReference != null)
        {
            await _jsRuntime.InvokeVoidAsync($"{JsFunctionsPrefix}.dispose", _selfReference);
        }
    }
    catch (**TaskCanceledException**)
    {
        // If the browser is gone, we don't need it to clean up any browser-side state
    }
}

Will this code break under .NET 6?
Since the JSDisconnectedException is a new class and it is not available in .NET 5, should I create two versions of the custom Virtualize component? One with the TaskCanceledException processing for .NET 5, and the other with the JSDisconnectedException processing for .NET 6.

Thanks,
Ilya

@ghost
Copy link
Copy Markdown

ghost commented May 28, 2021

Hi @pos777. 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

api-approved API was approved in API review, it can be implemented area-blazor Includes: Blazor, Razor Components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants