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

[browser][MT] Schedule mono_wasm_resolve_or_reject_promise continuation to run asynchronously #99317

Merged
merged 1 commit into from
Mar 5, 2024
Merged

[browser][MT] Schedule mono_wasm_resolve_or_reject_promise continuation to run asynchronously #99317

merged 1 commit into from
Mar 5, 2024

Conversation

pavelsavara
Copy link
Member

Theory of this fix:

  • passing Task to JS as parameter of async JSImport with virtual JSHandle -2 allocated with AllocJSVHandle()
  • mono_wasm_resolve_or_reject_promise is executed before the mono_wasm_invoke_jsimport
  • both are async messages via mono_threads_wasm_async_run_in_target_thread and they could be deliverd in this order in case that the task is already resolved
  • so that task.ContinueWith(Complete, ... is executed synchronously from inside of ToJS(Task? value)

contributes to #98406

@pavelsavara pavelsavara added arch-wasm WebAssembly architecture area-System.Runtime.InteropServices.JavaScript os-browser Browser variant of arch-wasm labels Mar 5, 2024
@pavelsavara pavelsavara added this to the 9.0.0 milestone Mar 5, 2024
@pavelsavara pavelsavara requested a review from maraf March 5, 2024 17:45
@pavelsavara pavelsavara self-assigned this Mar 5, 2024
@ghost
Copy link

ghost commented Mar 5, 2024

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

Issue Details

Theory of this fix:

  • passing Task to JS as parameter of async JSImport with virtual JSHandle -2 allocated with AllocJSVHandle()
  • mono_wasm_resolve_or_reject_promise is executed before the mono_wasm_invoke_jsimport
  • both are async messages via mono_threads_wasm_async_run_in_target_thread and they could be deliverd in this order in case that the task is already resolved
  • so that task.ContinueWith(Complete, ... is executed synchronously from inside of ToJS(Task? value)

contributes to #98406

Author: pavelsavara
Assignees: pavelsavara
Labels:

arch-wasm, area-System.Runtime.InteropServices.JavaScript, os-browser
Milestone: 9.0.0

@pavelsavara pavelsavara marked this pull request as ready for review March 5, 2024 17:58
@pavelsavara pavelsavara requested a review from lewing as a code owner March 5, 2024 17:58
@lewing
Copy link
Member

lewing commented Mar 5, 2024

I like the theory

@build-analysis build-analysis bot mentioned this pull request Mar 5, 2024
Closed
@lewing lewing merged commit c89c23d into dotnet:main Mar 5, 2024
119 of 121 checks passed
@pavelsavara
Copy link
Member Author

I'm afraid it didn't help 😦
https://helixre107v0xdcypoyl9e7f.blob.core.windows.net/dotnet-runtime-refs-pull-99296-merge-aa17b7698c46456799/WasmTestOnBrowser-System.Runtime.InteropServices.JavaScript.BackgroundExec.Tests/1/console.f27a5995.log?helixlogtype=result

This was referenced Mar 6, 2024
Merged
Closed
@github-actions github-actions bot locked and limited conversation to collaborators Apr 7, 2024
@pavelsavara pavelsavara deleted the browser_mt_racing_task_jshandle branch September 2, 2024 15:34
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@lewing lewing lewing approved these changes

@maraf maraf Awaiting requested review from maraf

Assignees

@pavelsavara pavelsavara

Labels
arch-wasm WebAssembly architecture area-System.Runtime.InteropServices.JavaScript os-browser Browser variant of arch-wasm
Projects
None yet
Milestone
9.0.0
Development

Successfully merging this pull request may close these issues.
2 participants
@pavelsavara @lewing