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][wasm] Dispatch rendering to main thread (Net9) #52724

Merged
merged 18 commits into from
Jan 17, 2024

Conversation

pavelsavara
Copy link
Member

@pavelsavara pavelsavara commented Dec 11, 2023

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

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.

Notes:

  • server side RendererSynchronizationContext is running jobs on multiple threads
  • WASM MT JSSynchronizationContext is running jobs always in single thread
  • both RendererSynchronizationContextDispatcher and WebAssemblyDispatcher can synchronously run jobs out of order!
  • RendererSynchronizationContextDispatcher checks SynchronizationContext.Current which could propagate to multiple threads
  • WebAssemblyDispatcher checks explicit thread, because of the JS objects thread affinity.
  • both can throw exceptions synchronously or set exception to the Task
  • RendererSynchronizationContext has UnhandledExceptionEventHandler
  • WebAssemblyDispatcher always propagates the exception to the caller.

Contributes to dotnet/runtime#85592
Fixes #48768
Fixes dotnet/runtime#95547
Previous attempt #48991

@amcasey

This comment was marked as resolved.

@pavelsavara

This comment was marked as resolved.

@kg

This comment was marked as resolved.

@pavelsavara

This comment was marked as resolved.

@pavelsavara
Copy link
Member Author

What should happen with src\Components\WebView\Samples\PhotinoPlatform\src\PhotinoSynchronizationContext.cs ? Is that relevant to WASM ?

@pavelsavara

This comment was marked as resolved.

@amcasey
Copy link
Member

amcasey commented Dec 12, 2023

how did you find the stack trace ? I'm looking into CI here and can't find it.

There's an Artifacts tab in the AzDO view, but I'm not sure who has access. I pulled the trace out of a dump artifact.

@pavelsavara pavelsavara marked this pull request as ready for review January 3, 2024 16:37
pavelsavara and others added 2 commits January 16, 2024 12:29
Co-authored-by: Steve Sanderson <SteveSandersonMS@users.noreply.github.com>
@pavelsavara
Copy link
Member Author

pavelsavara commented Jan 16, 2024

In your new code, a lot of that sophistication (i.e., mapping of InvokeAsync semantics to sync context send/post semantics) is inside the dispatcher.
Could you clarify why it was better to put the sophistication inside the dispatcher inside of inside the sync context like the existing code? My guess is it's about not having to make JSSyncContext into public API.

The extra sophistication you speak about, should be compared with extra methods RendererSynchronizationContext.InvokeAsync which are not part of the regular SynchronizationContext API. That is Blazor specific extension.

I guess arguably the way to make this code more equivalent to the existing logic would be if you added an InvokeAsync to JSSynchronizationContext that handles most of this sophistication, and then WebAssemblyDispatcher would just call that.

I agree that the developer experience of using public API of SynchronizationContext is far from ideal in the world of async/await/Task.
That's what InvokeAsync solves for Blazor, in somewhat Blazor specific way.

Whether or not this is desirable depends most on who is most likely to want to modify this behavior in the future, and whether it's really Blazor-specific.

If we wanted to have better API for the SynchronizationContext + Task scenarios, that would be different project than this.
I also think it may have different API signatures than the public API of Blazor.

@pavelsavara
Copy link
Member Author

pavelsavara commented Jan 16, 2024

I'm not sure that's correct. RendererSynchronizationContextDispatcher does propagate exceptions to the caller by calling state.Completion.SetException(exception). However, what happens if that call throws? That is, if the caller's exception-handling continuation itself throws.

Do we know if this actually ever happens ?
WebAssemblyDispatcher uses TaskCompletionSource.SetException which in my opinion only throws if the same task was already completed/failed before.

We can use TaskCompletionSource.TrySetException, but I think we don't have scenario where multiple branches are executed.

It looks like WebAssemblyDispatcher doesn't have any equivalent to this. Is that right?

I think it doesn't need to be there.

But I can add extra try/catch layer to WebAssemblyDispatcher and forward it to UnhandledExceptionEventHandler
Should I do that, just to be on the safe side ? @SteveSandersonMS

@SteveSandersonMS
Copy link
Member

SteveSandersonMS commented Jan 16, 2024

To get a better sense of the compatibility level with existing code, I was hoping to try running the existing suite of E2E tests with the multithreaded wasm runtime. (Note that I only hope to do this as a one-off manual run, not as something to run on every build in CI). However I'm not sure what's required to enable that.

What I did, while using the code in this branch:

  • In src\Components\test\testassets\BasicTestApp\BasicTestApp.csproj, added <WasmEnableThreads>true</WasmEnableThreads>
  • In src\Components\WebAssembly\DevServer\src\Server\Startup.cs, inside Configure, added:
    app.Use(async (ctx, next) =>
    {
        // Browser multi-threaded runtime requires cross-origin policy headers to enable SharedArrayBuffer.
        ctx.Response.Headers.Append("Cross-Origin-Embedder-Policy", "require-corp");
        ctx.Response.Headers.Append("Cross-Origin-Opener-Policy", "same-origin");
        await next(ctx);
    });
  • Ran an existing Blazor WebAssembly E2E test (arbitrarily, I chose FormsTest.CanHaveModelLevelValidationErrors)

However at runtime the app doesn't start, as it hits these errors which appear in the browser console:

MONO_WASM [0xdbdfc-main]: Error in bindings_init Can't find method System.Runtime.InteropServices.JavaScript.JavaScriptExports.InstallMainSynchronizationContext
xt @ dotnet.runtime.9.0.0-alpha.1.24063.6.a0dyicyyig.js:3
Ad @ dotnet.runtime.9.0.0-alpha.1.24063.6.a0dyicyyig.js:3
(anonymous) @ dotnet.runtime.9.0.0-alpha.1.24063.6.a0dyicyyig.js:3
dotnet.runtime.9.0.0-alpha.1.24063.6.a0dyicyyig.js:3

MONO_WASM [0xdbdfc-main]: onRuntimeInitializedAsync() failed Can't find method System.Runtime.InteropServices.JavaScript.JavaScriptExports.InstallMainSynchronizationContext

Are there some additional steps needed to enable the multithreaded runtime properly? Do you know why it would be unable to find these methods?

@SteveSandersonMS
Copy link
Member

But I can add extra try/catch layer to WebAssemblyDispatcher and forward it to UnhandledExceptionEventHandler
Should I do that, just to be on the safe side ? @SteveSandersonMS

If we don't know of a specific reason why it would make a difference, then TBH I'd prefer not to add extra logic for that. If we need to tweak some low-level error handling behavior later then we can do so.

@pavelsavara
Copy link
Member Author

@SteveSandersonMS It matches what I did in src\Components\WebAssembly\testassets\ThreadingApp\ThreadingApp.csproj, so I'm not sure what's wrong. I guess you have installed the workload ?

Can't find method System.Runtime.InteropServices.JavaScript.JavaScriptExports.InstallMainSynchronizationContext

This means that you have non-MT assemblies from the runtime pack. The MT runtime pack comes with the workload.

Note there is --apply-cop-headers CLI flag to devserver

Maybe you need clean/rebuild the test app ?

@SteveSandersonMS
Copy link
Member

Maybe you need clean/rebuild the test app ?

I'll try. But if that fixes it, wouldn't it mean there's a framework/SDK bug? Developers should only have to run the "build" gesture to get an up-to-date output.

@javiercn
Copy link
Member

I'll try. But if that fixes it, wouldn't it mean there's a framework/SDK bug? Developers should only have to run the "build" gesture to get an up-to-date output.

If you are using 17.9.x beware of VS accelerated builds might be getting in the way.

@SteveSandersonMS
Copy link
Member

Update: yes, removing bin and obj did fix it. I don't know whether VS Accelerated Build is involved in causing this. Wasn't it disabled at the SDK level for WebAssembly projects?

@javiercn
Copy link
Member

javiercn commented Jan 16, 2024

@SteveSandersonMS yes, but I did that like last week, it probably hasn't reached here yet. Specially if the PR hasn't been rebased recently.

@SteveSandersonMS
Copy link
Member

SteveSandersonMS commented Jan 16, 2024

OK, so I've been running the existing E2E Blazor WebAssembly tests with the MT runtime enabled, and the great majority pass, but a few seem to hit problems:

  • SignalRClientTest all fail. Unsure why - it just seems not to get messages back from the server. Should SignalR Client do something differently to work with the MT runtime?
  • ThreadingAppTest.CounterPageCanUseThreads fails for me locally (Expected: Not "Current count: 0", Actual: "Current count: 0"). Maybe you already fixed it and I didn't get the latest code yet.
  • RoutingTest.ClickOnAnchorInsideSVGElementGetsIntercepted fails. Unclear how this is related to the MT runtime, but AFAIK it passes on the regular runtime.
  • ComponentRenderingTest.CanDispatchAsyncWorkToSyncContext fails:
    • Expected: First Second Third Fourth Fifth
    • Actual: First Third Second Fourth Fifth
    • This is the most concerning one since subtle ordering issues would be very tricky for developers to understand. Note that this test runs on both Server and WebAssembly to show they have the same behavior. I would hope the MT runtime works the same, but if it can't for some reason, we would definitely want to understand why.
    • UPDATE: Actually I think I understand this:
      • In the existing Server/WebAssembly cases, even if you explicitly marshal yourself off the sync context and onto a background thread, an InvokeAsync call will start its callback synchronously if the sync context is idle.
      • But for Multithreaded WebAssembly, if you explicitly marshal yourself off the sync context and onto a background thread, then InvokeAsync cannot start the callback synchronously regardless of whether the UI thread is idle, because it still has to marshal the call asynchronously to the UI thread.
      • Technically I suppose we could check if the UI thread is idle, and if so, block the caller until the callback yields. But that's really complicated and it's unclear why we'd want to offer this guarantee.
      • So in summary I suspect the difference in behavior for this particular test is by design. Do you agree?

Would you be able to check into what's going on, so we can be confident both the MT runtime and the new dispatching logic are all working consistently with our other platforms?

@pavelsavara
Copy link
Member Author

pavelsavara commented Jan 17, 2024

I'm glad you tested this @SteveSandersonMS, thank you.

SignalRClientTest all fail. Unsure why - it just seems not to get messages back from the server. Should SignalR Client do something differently to work with the MT runtime?

I expect this to fix or improve with dotnet/runtime#96618
I also created dotnet/runtime#97085 for us to cover it with integration test.
By design, there should not be anything special in Blazor or user code for that. Let's keep eye on this.

ThreadingAppTest.CounterPageCanUseThreads fails for me locally (Expected: Not "Current count: 0", Actual: "Current count: 0"). Maybe you already fixed it and I didn't get the latest code yet.

Yeah, I merged your suggestion from GH and only then added the button to trigger it in next commit.

* `ComponentRenderingTest.CanDispatchAsyncWorkToSyncContext` fails:
  * Expected: First Second Third Fourth Fifth
  * Actual:   First Third Second Fourth Fifth

Synchronous dispatch of RendererSynchronizationContext is meant to be just perf optimization, right ?
With behavioral consequences.

Making assumptions on if you are or are not on the right thread when you InvokeAsync in user code would be bad practice, I think.

Also in MT, you can't know if any other thread is making the queue busy at any time (X).

But I can imagine there are existing code-bases in the wild which work this way and it may bite them.

Technically I suppose we could check if the UI thread is idle, and if so, block the caller until the callback yields. But that's really complicated and it's unclear why we'd want to offer this guarantee.

That would have to be SynchronizationContext.Send instead of Post.
And how to detect it's idle ?
For the UI thread if could mean multiple things, because there are multiple queues of work.
Even JSSynchronizationContext queue could have items/jobs unrelated to Blazor in it.

A) If we wanted to check that JSSynchronizationContext queue of work is empty, we would have to have some API for it.
B) We could also keep track of pending item count on the new WebAssemblyDispatcher. It seems easy to do.

So in summary I suspect the difference in behavior for this particular test is by design. Do you agree?

I like consistency, which would mean B)

But I don't like the sync dispatch assumption especially because (X). That said I agree and prefer
C) we just fix the test.

@javiercn what do you think ?

I would like to note, that this PR is probably not the last bit.
I will be experimenting with way how to offload as many jobs to threadpool from the UI thread.
UI thread needs to stay idle for reasons.

There are 2 possible ways how I could do it.

  1. light-weight way. Hoping that it will be enough to fend off any deadlocks.
    • Blazor change in which we offload all UI events like onClick etc to thread pool.
    • Maybe something with continuations from HTTP/WS network events. Run continuations in the TP.
  2. the deputy design, which will completely prohibit you from running your main managed thread in the UI thread.
    • this will have broader architectural impact and also perf impact

How this is relevant:

  • there will be almost always cross-thread dispatch in WebAssemblyDispatcher with async processing by UI thread.
  • it will make the assumptions about which thread you are running on even more risky.

@pavelsavara
Copy link
Member Author

B) We could also keep track of pending item count on the new WebAssemblyDispatcher. It seems easy to do.

We would have to also prevent other threads from posting jobs in the meantime, as a race condition.

@SteveSandersonMS
Copy link
Member

Cool, thanks for your feedback.

[About SignalR Client] I expect this to fix or improve with dotnet/runtime#96618

Great. I'll ignore this aspect then, since it's not Blazor specific, and we will assume the runtime will deal with whatever's necessary to make SignalR Client continue working.

But I don't like the sync dispatch assumption especially because (X). That said I agree and prefer
C) we just fix the test.

Totally fine with me. We don't actually have to change the test because it's not going to run on the MT runtime. I only made a local change to enable a one-time run, but there's no plan to extend the CI run to do all the E2E tests on both single-threaded and multithreaded runtimes.

I agree overall that the synchronous-dispatch-if-idle behavior is best regarded as a perf optimization, not a guaranteed behavior. It totally makes sense for the MT runtime not to do that, because it would be so much more complicated for it.

Copy link
Member

@SteveSandersonMS SteveSandersonMS left a comment

Choose a reason for hiding this comment

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

Excellent work! Thanks for being so thorough with the details.

@pavelsavara pavelsavara merged commit b027cc3 into dotnet:main Jan 17, 2024
26 checks passed
@ghost ghost modified the milestones: .NET 8 Planning, 9.0-preview1 Jan 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arch-wasm area-blazor Includes: Blazor, Razor Components
Projects
None yet
5 participants