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

[WASM] Fix async/await in config loading #54652

Merged
merged 3 commits into from
Jun 24, 2021
Merged

[WASM] Fix async/await in config loading #54652

merged 3 commits into from
Jun 24, 2021

Conversation

Daniel-Genkin
Copy link
Contributor

@Daniel-Genkin Daniel-Genkin commented Jun 23, 2021 •
edited
Loading

Fixed the async loading of mono-config.json to fix issue #54643

@ghost
Copy link

ghost commented Jun 23, 2021

Tagging subscribers to this area: @CoffeeFlux
See info in area-owners.md if you want to be subscribed.

Issue Details

Removed the async/await from mono-config.json loading and reverted to using callbacks as the async caused race conditions as emscripten does not await async functions.

Fixes issue: #54643

Author: Daniel-Genkin
Assignees: -
Labels:

area-VM-meta-mono
Milestone: -

@thaystg thaystg requested review from kg and lewing June 23, 2021 22:47
@thaystg thaystg requested a review from radekdoulik June 23, 2021 22:48
@thaystg thaystg linked an issue Jun 23, 2021 that may be closed by this pull request
[wasm] Uncaught TypeError: Cannot set property 'loaded_cb' of null #54643
Closed
@lewing
Copy link
Member

lewing commented Jun 23, 2021

I think we should figure out why addRunDependency is failing here blazor uses it successfully from Module.preRun in multiple places
https://github.com/dotnet/aspnetcore/blob/4dc663a0d46467908f5463d5004b0ae574648f4a/src/Components/Web.JS/src/Platform/Mono/MonoPlatform.ts#L561

@kg
Copy link
Member

kg commented Jun 23, 2021

I think we should figure out why addRunDependency is failing here blazor uses it successfully from Module.preRun in multiple places
https://github.com/dotnet/aspnetcore/blob/4dc663a0d46467908f5463d5004b0ae574648f4a/src/Components/Web.JS/src/Platform/Mono/MonoPlatform.ts#L561

My guess would be that as I suggested in comments on the earlier PR, because removeRunDependency instantly invokes the callback once it's ready, the async function hasn't returned yet so the config has not been stored in the appropriate place by the time the other code looks for it (that won't happen until the next turn of the event loop)

@lewing
Copy link
Member

lewing commented Jun 23, 2021 •
edited
Loading

My guess would be that as I suggested in comments on the earlier PR, because removeRunDependency instantly invokes the callback once it's ready, the async function hasn't returned yet so the config has not been stored in the appropriate place by the time the other code looks for it (that won't happen until the next turn of the event loop)

yeah, I think we can follow the pattern of returning a promise and awaiting that when setting the config (like blazor does)

@lambdageek
Copy link
Member

There's now a (logical) conflict with #54568 which added a new HotReload functional test - which at the moment has the async runtime.js stuff. (But see #54622 - the last commit there sets things up for the old-style mono-config.js)

@Daniel-Genkin
Copy link
Contributor Author

Daniel-Genkin commented Jun 24, 2021 •
edited
Loading

This latest push undoes the previous 4 commits and reverts the async/await. However (thanks to Larry) this approach also seems to work

@Daniel-Genkin Daniel-Genkin changed the title [WASM] Remove async/await from config loading [WASM] Fix async/await in config loading Jun 24, 2021
lewing
lewing reviewed Jun 24, 2021
View reviewed changes
Copy link
Member

@lewing lewing left a comment

Choose a reason for hiding this comment

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

Needs to fix the hot reload test as well

@kg
Copy link
Member

kg commented Jun 24, 2021

This new solution looks good to me

@lewing lewing added the arch-wasm WebAssembly architecture label Jun 24, 2021
@ghost
Copy link

ghost commented Jun 24, 2021

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

Issue Details

Fixed the async loading of mono-config.json to fix issue #54643

Author: Daniel-Genkin
Assignees: radical
Labels:

arch-wasm, area-VM-meta-mono
Milestone: -

radekdoulik
radekdoulik reviewed Jun 24, 2021
View reviewed changes
Copy link
Member

@radekdoulik radekdoulik left a comment

Choose a reason for hiding this comment

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

LGTM. It just needs to wait for #54622 or cherry-pick the 15fc697

@lambdageek
Copy link
Member

LGTM. It just needs to wait for #54622 or cherry-pick the 15fc697

This PR doesn't need to wait for 54622 (that PR is on the preview6 branch, not main) - looks like Daniel picked up the new test from main and updated it the same as the othre functional tests. Thanks Daniel!

@Daniel-Genkin
Copy link
Contributor Author

LGTM. It just needs to wait for #54622 or cherry-pick the 15fc697

This PR doesn't need to wait for 54622 (that PR is on the preview6 branch, not main) - looks like Daniel picked up the new test from main and updated it the same as the othre functional tests. Thanks Daniel!

Yes that's exactly what I did haha :)

@lewing
Copy link
Member

lewing commented Jun 24, 2021

Test failures look infrastructure related and this issue is causing CI instability.

@lewing lewing merged commit ea1707c into dotnet:main Jun 24, 2021
thaystg added a commit to thaystg/runtime that referenced this pull request Jun 24, 2021
@thaystg
Merge remote-tracking branch 'origin/main' into thays_componentize_de…
06c3999 
…bugger2

* origin/main: (107 commits)
  Disable MacCatalyst arm64 PR test runs on staging pipeline (dotnet#54678)
  [WASM] Fix async/await in config loading (dotnet#54652)
  Fix for heap_use_after_free flagged by sanitizer (dotnet#54679)
  [wasm] Bump emscripten to 2.0.23 (dotnet#53603)
  Fix compiler references when building inside VS (dotnet#54614)
  process more TLS frames at one when available (dotnet#50815)
  Add PeriodicTimer (dotnet#53899)
  UdpClient with span support (dotnet#53429)
  exclude fragile tests (dotnet#54671)
  get last error before calling a method that might fail as well (dotnet#54667)
  [FileStream] add tests for device and UNC paths (dotnet#54545)
  Fix sporadic double fd close (dotnet#54660)
  Remove Version.Clone from AssemblyName.Clone (dotnet#54621)
  [wasm] Enable fixed libraries tests (dotnet#54641)
  [wasm] Fix blazor/aot builds (dotnet#54651)
  [mono][wasm] Fix compilation error on wasm (dotnet#54659)
  Fix telemetry for Socket connects to Dns endpoints (dotnet#54071)
  [wasm] Build static components; include hot_reload in runtime (dotnet#54568)
  [wasm][debugger] Reuse debugger-agent on wasm debugger (dotnet#52300)
  Put Crossgen2 in sync with dotnet#54235 (dotnet#54438)
  ...
@ghost ghost locked as resolved and limited conversation to collaborators Jul 24, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@radekdoulik radekdoulik radekdoulik approved these changes

@lewing lewing lewing approved these changes

@marek-safar marek-safar Awaiting requested review from marek-safar

@kg kg Awaiting requested review from kg

Assignees

@radical radical

Labels
arch-wasm WebAssembly architecture area-VM-meta-mono
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[wasm] Uncaught TypeError: Cannot set property 'loaded_cb' of null
6 participants
@Daniel-Genkin @lewing @kg @lambdageek @radekdoulik @radical