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.

Sign up for GitHub

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

Jump to bottom

[blazor][wasm] Dispatch rendering to main thread #48991

Closed
wants to merge 17 commits into from
Closed

[blazor][wasm] Dispatch rendering to main thread #48991

wants to merge 17 commits into from

Conversation

pavelsavara
Copy link
Member

@pavelsavara pavelsavara commented Jun 23, 2023 •
edited
Loading

This PR implements WebAssemblyDispatcher which will dispatch the call to Component.InvokeAsync back to main thread, where all the rendering/JS interop/networking could happen.

Context: with <WasmEnableThreads>true</WasmEnableThreads> we could use thread pool in C#.
Any async continuations with .ConfigureAwait(false) will likely run there.

Added new src/Components/WebAssembly/testassets/ThreadingApp

  • which is almost vanilla Blazor Wasm template, but with threads enabled.
  • On Counter page it uses timer and thread pool continuation to increase the count and then InvokeAsync to render it.
  • On FetchData page we test that Http client could be also used from thread pool.

Contributes to dotnet/runtime#85592
Fixes #48768

@pavelsavara pavelsavara added area-blazor Includes: Blazor, Razor Components feature-blazor-wasm This issue is related to and / or impacts Blazor WebAssembly labels Jun 23, 2023
@pavelsavara pavelsavara added this to the .NET 8.0 milestone Jun 23, 2023
@pavelsavara pavelsavara self-assigned this Jun 23, 2023
This was referenced Jun 20, 2023
Closed
Closed
@pavelsavara
Copy link
Member Author

image

@pavelsavara pavelsavara marked this pull request as ready for review June 27, 2023 15:09
@pavelsavara pavelsavara requested a review from a team as a code owner June 27, 2023 15:09
@mkArtakMSFT mkArtakMSFT modified the milestones: .NET 8.0, 8.0 Jun 29, 2023
@pavelsavara pavelsavara added this to the 8.0 milestone Jul 9, 2023
@pavelsavara
Copy link
Member Author

@mkArtakMSFT could we get this reviewed and merged for the next preview ?

javiercn
javiercn reviewed Jul 10, 2023
View reviewed changes
src/Components/WebAssembly/WebAssembly/src/Rendering/WebAssemblyDispatcher.cs
Comment on lines +71 to +85
self.Send((_) =>
{
try
{
body();
}
catch (Exception ex)
{
exc = ex;
}
}, null);
if (exc != null)
{
throw exc;
}
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't this block? My understanding is that this runs the callback inline, isn't it?

I'm trying to make sense of how this works when you are in a background thread and try to dispatch back to the "main" thread.

My understanding is that the contract for send must block the thread.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, this will block the sender thread. If this is the main thread, it would just invoke it.

@pavelsavara
feedback
a44a60f
@pavelsavara
Merge branch 'main' into wasm_mt_dispatcher
1c7c442 
# Conflicts:
#	AspNetCore.sln
#	src/Components/Components.slnf
#	src/Components/ComponentsNoDeps.slnf
@pavelsavara
Merge branch 'main' into wasm_mt_dispatcher
e4e38e0
@pavelsavara pavelsavara removed the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Jul 11, 2023
@pavelsavara
Copy link
Member Author

I think that CI failure is unrelated to this PR. @javiercn are there further questions ?

SteveSandersonMS
SteveSandersonMS reviewed Jul 14, 2023
View reviewed changes
src/Components/WebAssembly/WebAssembly/src/Rendering/WebAssemblyDispatcher.cs
}

_context!.InvokeAsync(workItem);
return Task.CompletedTask;
Copy link
Member

Choose a reason for hiding this comment

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

This looks like a mistake, unless I'm confused.

Shouldn't this return the task returned by _context.InvokeAsync, instead of Task.CompletedTask?

Copy link
Member Author

Choose a reason for hiding this comment

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

It will invoke the void SynchronizationContextExtension.InvokeAsync and so there is no Task. Also it's blocking synchronous call.

Copy link
Member

Choose a reason for hiding this comment

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

More details about the expected behavior in https://github.com/dotnet/aspnetcore/pull/48991/files#r1263731418

SteveSandersonMS
SteveSandersonMS reviewed Jul 14, 2023
View reviewed changes
src/Components/WebAssembly/WebAssembly/src/Rendering/WebAssemblyDispatcher.cs
_context = SynchronizationContext.Current;
}

public override bool CheckAccess() => SynchronizationContext.Current == _context || _context == null;
Copy link
Member

Choose a reason for hiding this comment

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

Could you explain the || _context == null part of this check?

I would have thought that if SynchronizationContext.Current == null, then we'd only consider ourselves "having access" if the original _context was also null. So it seems like the logic would be more correct if the || _context == null part of this check was removed.

SteveSandersonMS
SteveSandersonMS reviewed Jul 14, 2023
View reviewed changes
src/Components/WebAssembly/WebAssembly/src/Rendering/WebAssemblyDispatcher.cs
throw exc;
}
return value!;
}
Copy link
Member

@SteveSandersonMS SteveSandersonMS Jul 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The logic around async and exception handling looks strange to me. At least, it will behave very differently from how RendererSynchronizationContext does.

  • Dispatcher.InvokeAsync(Action)
    • With WebAssemblyDispatcher, it blocks the caller until the action is fully complete. If the action throws, then it re-throws instead of returning any task.
    • With RendererSynchronizationContextDispatcher, it doesn't block the caller, and returns a Task that completes when the action is complete. If the action throws, the Task is faulted.
  • Dispatcher.InvokeAsync(Func<Task>)
    • With WebAssemblyDispatcher, it blocks the caller while the dispatch is in process (up until the recipient yields), and then returns the Task returned by the recipient. If the recipient throws synchronously, then it re-throws instead of returning any task.
    • With RendererSynchronizationContextDispatcher, it does not block the caller, and returns a a Task that completes when the dispatch and recipient's Task is complete. If the recipent throws synchronously, the returned Task is faulted.

Are these differences in behavior intentional? My guess is we should behave the same as RendererSynchronizationContextDispatcher/RendererSynchronizationContext.

Copy link
Member

Choose a reason for hiding this comment

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

Similarly please see how RendererSynchronizationContext deals with unhandled exceptions by calling OnUnhandledException on the Dispatcher.

@ghost
Copy link

ghost commented Jul 21, 2023

Looks like this PR hasn't been active for some time and the codebase could have been changed in the meantime.
To make sure no conflicting changes have occurred, please rerun validation before merging. You can do this by leaving an /azp run comment here (requires commit rights), or by simply closing and reopening.

@ghost ghost added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Jul 21, 2023
@pavelsavara pavelsavara mentioned this pull request Jul 24, 2023
Merged
@wtgodbe wtgodbe modified the milestones: 8.0, 8.0.0 Oct 3, 2023
@ghost ghost added the area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework label Oct 3, 2023
This was referenced Dec 4, 2023
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@radical radical radical left review comments

@lambdageek lambdageek lambdageek left review comments

@SteveSandersonMS SteveSandersonMS SteveSandersonMS left review comments

@campersau campersau campersau left review comments

@javiercn javiercn Awaiting requested review from javiercn

Assignees

@pavelsavara pavelsavara

Labels
area-blazor Includes: Blazor, Razor Components area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework feature-blazor-wasm This issue is related to and / or impacts Blazor WebAssembly pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun
Projects
None yet
Milestone
8.0.0
Development

Successfully merging this pull request may close these issues.

Integrate with WebAssembly multithreading
8 participants
@pavelsavara @radical @lambdageek @SteveSandersonMS @campersau @javiercn @wtgodbe @mkArtakMSFT