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][mt] Do not allow blocking on the thread with JS interop: UI, JSWebWorker #76958

Closed
Tracked by #68162
lambdageek opened this issue Oct 12, 2022 · 11 comments
Closed
Tracked by #68162
Assignees
Labels
arch-wasm WebAssembly architecture area-VM-threading-mono os-browser Browser variant of arch-wasm
Milestone

Comments

@lambdageek
Copy link
Member

lambdageek commented Oct 12, 2022

Blocking synchronization primitives should continue to throw on the

  • UI, JSWebWorker threads
  • PNSE as they do in single-threaded Wasm

There should be opt-out MsBuild flag, for projects who need to risk it.

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Oct 12, 2022
@ghost
Copy link

ghost commented Oct 12, 2022

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

Issue Details

null

Author: lambdageek
Assignees: -
Labels:

area-System.Threading

Milestone: -

@lambdageek lambdageek changed the title Blocking synchronization primitives should continue to throw on the main thread as they do in single-threaded Wasm [wasm-mt] Do not allow blocking on the main thread Oct 12, 2022
@lambdageek lambdageek added area-VM-threading-mono and removed area-System.Threading untriaged New issue has not been triaged by the area owner labels Oct 12, 2022
@lambdageek lambdageek added the arch-wasm WebAssembly architecture label Oct 12, 2022
@lambdageek lambdageek added this to the 8.0.0 milestone Oct 12, 2022
@ghost
Copy link

ghost commented Oct 12, 2022

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

Issue Details

Blocking synchronization primitives should continue to throw on the main thread as they do in single-threaded Wasm

Author: lambdageek
Assignees: -
Labels:

arch-wasm, area-Threading-mono

Milestone: -

@lewing lewing modified the milestones: 8.0.0, 9.0.0 Jul 24, 2023
@pavelsavara pavelsavara changed the title [wasm-mt] Do not allow blocking on the main thread [browser][mt] Do not allow blocking on the main thread Nov 9, 2023
@pavelsavara pavelsavara added the os-browser Browser variant of arch-wasm label Nov 9, 2023
@pavelsavara
Copy link
Member

pavelsavara commented Nov 9, 2023

If we move out of UI thread, the scenario is calling synchronous JSExport across threads.
We could make it throw by default and opt-in into silent spin-wait.

@pavelsavara
Copy link
Member

We need to also prevent it for JSWebWorker threads because synchronous .Wait prevents HTTP/WS events, which could lead to deadlock.

cc @radekdoulik

@pavelsavara pavelsavara changed the title [browser][mt] Do not allow blocking on the main thread [browser][mt] Do not allow blocking on the thread with JS interop: UI, JSWebWorker Jan 14, 2024
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jan 21, 2024
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jan 31, 2024
@pavelsavara
Copy link
Member

pavelsavara commented Feb 1, 2024

Quick list of APIs, for which we need to validate that they internally use something we already addressed so far.
Or we need to address them too.

  • Task - WaitAll, WaitAny, InternalWait
  • WaitSubsystem - Wait
  • WaitableObject - .Wait, SatisfyWaitForAll, Wait_Locked
  • ThreadWaitInfo - .Wait, Sleep,
  • LowLevelLock - WaitAndAcquire
  • LowLevelMonitor - Wait, Acquire
  • LowLevelSpinWaiter - Wait
  • LowLevelLifoSemaphore - Wait
  • FileSystemWatcher - WaitForChanged
  • GC - WaitForFullGCApproach, WaitForFullGCComplete, WaitForPendingFinalizers
  • CancellationTokenSource - WaitForCallbackToComplete
  • ManualResetEventSlim - Wait
  • SemaphoreSlim - Wait
  • SynchronizationContext - Wait
  • WaitHandle - WaitOne, WaitAll
  • ManualResetEventWithAwaiterSupport - Wait
  • CountdownEvent - Wait
  • Interop.Sys.LowLevelMonitor_TimedWait & SystemNative_LowLevelMonitor_Wait

I'm sure I missed some places.

@pavelsavara
Copy link
Member

I'm thinking about the proper solution for synchronous calls from and to JavaScript.

A) synchronous [JSImport] call targeting the same thread JS.

  • we know that the this thread is with js interop and so it would not block.

B) synchronous [JSImport] call targeting the different thread JS.

  • we know that the target is also thread with js interop and so it would not block.

C) synchronous [JSExport] call targeting the same thread Managed.

  • we know that the target is also thread with js interop and so it would not block.

D) synchronous [JSExport] call targeting the different thread Managed.

  • this option doesn't exits yet, but it will become interesting when/if we have UI and deputy separated.
  • D1) if we throw PNSE on .Wait in deputy, we can make this call, because the target would not be blocking for long
  • D2) if we allow .Wait in deputy, we will have to throw on the [JSExport] boundary. This seems more useful to me, from perspective of legacy 3rd party MT codebases.

None of the JSExport/JSImport which return Task are problem, because they are just sending async message.

@pavelsavara
Copy link
Member

  • when thread is blocked, the emscripten __emscripten_thread_mailbox_await doesn't work because you need to yield to browser loop.
  • maybe we could call emscripten_proxy_execute_queue our self ?
  • promise resolved on UI, while deputy is blocked, could be propagated to managed via dedicated I/O thread.
    • Via complete_task -> CompleteTask -> tcs.TrySetResult
    • we need to make sure that continuations don't run on I/O thread.

@pavelsavara
Copy link
Member

If we allow blocking on the deputy thread, the interaction with sync JS interop would be also affected. Situation D2 above.

@pavelsavara
Copy link
Member

pavelsavara commented Mar 9, 2024

when you call sync JSExport from UI thread, the Console.WriteLine will deadlock and also any FS operation!

  • that should also throw PNSE
  • or we need to figure out how to let emscripten to server it's queue during wait for the sync call to resolve.

@pavelsavara
Copy link
Member

_emscripten_proxy_to_main_thread_js needs to throw when UI thread is blocked by calling synchronous JSExport

@pavelsavara
Copy link
Member

#99833 implemented the core part of this for JSThreadBlockingMode.ThrowWhenBlockingWait and JSThreadBlockingMode.WarnWhenBlockingWait

The high level expectations are described in

export const enum JSThreadBlockingMode {
/**
* Prevents synchronous JSExport from being called from JavaScript code in UI thread.
* On JSWebWorker synchronous JSExport always works.
* On JSWebWorker blocking .Wait always warns.
* This is the default mode.
*/
PreventSynchronousJSExport = "PreventSynchronousJSExport",
/**
* Allows synchronous JSExport to be called from JavaScript code also in UI thread.
* Inside of that call blocking .Wait throws PNSE.
* Inside of that call nested call back to synchronous JSImport throws PNSE (because it would deadlock otherwise in 100% cases).
* On JSWebWorker synchronous JSExport always works.
* On JSWebWorker blocking .Wait always throws PNSE.
*/
ThrowWhenBlockingWait = "ThrowWhenBlockingWait",
/**
* Allows synchronous JSExport to be called from JavaScript code also in UI thread.
* Inside of that call blocking .Wait warns.
* Inside of that call nested call back to synchronous JSImport throws PNSE (because it would deadlock otherwise in 100% cases).
* On JSWebWorker synchronous JSExport always works.
* On JSWebWorker blocking .Wait always warns.
*/
WarnWhenBlockingWait = "WarnWhenBlockingWait",
/**
* Allows synchronous JSExport to be called from JavaScript code, and allows managed code to use blocking .Wait
* .Wait on Promise/Task chains could lead to deadlock because JS event loop is not processed and it can't resolve JS promises.
* This mode is dangerous and not supported.
* Allows synchronous JSExport to be called from JavaScript code also in Main thread.
* Inside of that call nested call back to synchronous JSImport throws PNSE (because it would deadlock otherwise in 100% cases).
*/
DangerousAllowBlockingWait = "DangerousAllowBlockingWait",
}

Throwing in more scenarios is counter-productive.

We can improve it if we hear specific feedback about it.

@github-actions github-actions bot locked and limited conversation to collaborators May 16, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-wasm WebAssembly architecture area-VM-threading-mono os-browser Browser variant of arch-wasm
Projects
Status: Done
Development

No branches or pull requests

4 participants