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

[browser] Enable threads in Wasm SDK #85109

Merged
merged 4 commits into from
Apr 24, 2023
Merged

[browser] Enable threads in Wasm SDK #85109

merged 4 commits into from
Apr 24, 2023

Conversation

maraf
Copy link
Member

@maraf maraf commented Apr 20, 2023

@maraf maraf added arch-wasm WebAssembly architecture area-Build-mono labels Apr 20, 2023
@maraf maraf added this to the 8.0.0 milestone Apr 20, 2023
@maraf maraf self-assigned this Apr 20, 2023
@ghost
Copy link

ghost commented Apr 20, 2023

Tagging subscribers to 'arch-wasm': @lewing
See info in area-owners.md if you want to be subscribed.

Issue Details

null

Author: maraf
Assignees: maraf
Labels:

arch-wasm, area-Build-mono

Milestone: 8.0.0

@lambdageek
Copy link
Member

How do I try this out?

@maraf
Copy link
Member Author

maraf commented Apr 20, 2023

The same story as for webcil. Until SDK PR lands and makes through installer, you need a lot of manual patching. I can show you tomorrow if you want.

@lewing
Copy link
Member

lewing commented Apr 20, 2023

wait for dotnet/sdk#31912 to have an installer build then you can use wbt on this pr

@maraf
Copy link
Member Author

maraf commented Apr 20, 2023

wait for dotnet/sdk#31912 to have an installer build then you can use wbt on this pr

That one is not enough. This is needed dotnet/sdk#31519

@lewing
Copy link
Member

lewing commented Apr 21, 2023

dotnet/installer#16197

@lambdageek
Copy link
Member

lambdageek commented Apr 22, 2023

I tried this one the same way as #84977 (comment) but I don't see dotnet.worker.js in wwwroot/_framework if I do dotnet run.

With dotnet publish it's there

@lambdageek
Copy link
Member

lambdageek commented Apr 22, 2023

Getting some blazor errors about blazor-internal not being loaded yet.

Seems like some of the blazor async code is running on worker threads instead of the main thread. no that's not it. the message is from the main thread.

azor.webassembly.js:1 Error: Assert failed: ES6 module blazor-internal was not imported yet, please call JSHost.ImportAsync() first.
Lt @ blazor.webassembly.js:1
worker.onmessage @ dotnet.8.0.0-dev.pry02bjaoa.js:11

This seems to happen pretty much as soon as I click around between pages when using the threaded runtime. threads don't actually need to be used. I think it's maybe something from the NavMenu, probably this one:

https://github.com/dotnet/aspnetcore/blob/d07f377b6c4c72f44a0eb2810c8abf1ae3fabd40/src/Components/WebAssembly/WebAssembly/src/Services/InternalJSImportMethods.cs#L77

blazor-internal is supposed to be set up with setModuleImports

https://github.com/dotnet/aspnetcore/blob/d07f377b6c4c72f44a0eb2810c8abf1ae3fabd40/src/Components/Web.JS/src/Platform/Mono/MonoPlatform.ts#L337

So it seems like maybe setModuleImports isn't working right on the main thread with the threaded runtime for some reason.

Ah interesting. if I set a breakpoint just before we do that "ES6 module ${js_module_name} was not imported yet, please call JSHost.ImportAsync() first." error, I see that I'm actually in a worker thread (!!)

So somewhere the blazor code ends up running some async code on a new thread after all. and then setModuleImports hasn't happened on the worker, so the module isn't loaded there.

I think it might be

https://github.com/dotnet/aspnetcore/blob/d07f377b6c4c72f44a0eb2810c8abf1ae3fabd40/src/JSInterop/Microsoft.JSInterop/src/Infrastructure/DotNetDispatcher.cs#L118

I'm not sure that task.ContinueWith(..., TaskScheduler.Current) would respect the synchronization context - I think it always queues up a work item for the threadpool.

@lambdageek
Copy link
Member

Also the blazor weather forecast page from the default template breaks because http fetching has a ConfigureAwait(false) somewhere. @pavelsavara, I think we need to fix that HTTP client thing that you found

   Unhandled exception rendering component: The JavaScript object can be used only on the thread where it was created.
System.InvalidOperationException: The JavaScript object can be used only on the thread where it was created.
   at System.Runtime.InteropServices.JavaScript.JSObject.AssertThreadAffinity(Object )
   at System.Net.Http.BrowserHttpInterop.AbortResponse(JSObject )
   at System.Net.Http.WasmFetchResponse.Dispose()
   at System.Net.Http.BrowserHttpContent.Dispose(Boolean )
   at System.Net.Http.HttpContent.Dispose()
   at System.Net.Http.HttpResponseMessage.Dispose(Boolean )
   at System.Net.Http.HttpResponseMessage.Dispose()
   at System.Net.Http.Json.HttpClientJsonExtensions.<<FromJsonAsyncCore>g__Core|4_0>d`2[[bl1.Pages.FetchData.WeatherForecast[], bl1, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null],[System.Text.Json.JsonSerializerOptions, System.Text.Json, Version=8.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51]].MoveNext()
   at bl1.Pages.FetchData.OnInitializedAsync()

@pavelsavara
Copy link
Member

pavelsavara commented Apr 24, 2023

Also the blazor weather forecast page from the default template breaks because http fetching has a ConfigureAwait(false) somewhere. @pavelsavara, I think we need to fix that HTTP client thing that you found

Here is the draft, but I need to try on top of your branch. #85244

I wonder if also user code would have to contain ConfigureAwait(true) on all lines with await keyword. If so, that means massive inconvenience even for our HTTP unit test suite

Edit: ConfigureAwait(true) is default behaviour

@lambdageek
Copy link
Member

@maraf Any reason this is still draft? I think this PR is working, even if the libraries and blazor need a bit of work.

@maraf maraf marked this pull request as ready for review April 24, 2023 13:40
@lambdageek
Copy link
Member

Fingerprinting stuff is working. I see fetches for dotnet.worker.8.0.0-dev.p6hue5g3ye.js and the webworkers are starting properly.

@maraf
Copy link
Member Author

maraf commented Apr 24, 2023

Failures are unrelated

@maraf maraf closed this Apr 24, 2023
@maraf maraf reopened this Apr 24, 2023
@maraf maraf merged commit 46f8f1e into dotnet:main Apr 24, 2023
@maraf maraf deleted the WasmSdkThreads branch April 24, 2023 19:54
@ghost ghost locked as resolved and limited conversation to collaborators May 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-wasm WebAssembly architecture area-Build-mono
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants