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

memory.atomic.wait32 on the main browser thread #174

Open
juj opened this issue Jan 15, 2021 · 17 comments
Open

memory.atomic.wait32 on the main browser thread #174

juj opened this issue Jan 15, 2021 · 17 comments

Comments

@juj
Copy link

juj commented Jan 15, 2021

Writing some unit tests for Emscripten, I today realize that Firefox and Chrome behavior differ when one issues a memory.atomic.wait32 on the main thread.

If one issues e.g.

  (drop
   (memory.atomic.wait32 offset=12
    (local.get $0)
    (i32.const 0)
    (i64.const 135790)
   )
  )

on the main thread,

  • Firefox will raise an exception "TypeError: waiting is not allowed on this thread"
  • Chrome will immediately and silently return without performing the wait

Tried to search through the spec to find the reference for wait instruction at https://webassembly.github.io/threads/core/syntax/instructions.html#syntax-instr-atomic-memory , but was unable to figure out which is the correct behavior, or if both are?

(I am aware of Atomics.waitAsync, this question is not about that, but just about what should happen if one does attempt a memory.atomic.wait32 on the main thread)

@conrad-watt
Copy link
Collaborator

@juj does the FireFox error occur at instantiation-time, or at run-time?

My knee-jerk preference would be for it to be a run-time trap to wait on the main thread, analogous to waiting on unshared memory, but I don't think it's been explicitly discussed.

@juj
Copy link
Author

juj commented Jan 15, 2021

It is a runtime exception, not an instantiation time exception.

@conrad-watt
Copy link
Collaborator

I've proposed a quick agenda item at the next CG meeting to try and work out if there's a behaviour we can align on.

My understanding is that this is consistently an exception in the JavaScript API, so my hope is we can agree to make this a runtime error in Wasm too.

@juj
Copy link
Author

juj commented Jan 15, 2021

There may perhaps be a small advantage in having atomic.wait perform a no-op and immediately return when called on main thread, namely that may allow sharing same wasm code across main thread and web workers (e.g. a spinlock that uses short yield waits in the inner loop to throttle?), instead of having to write two different versions of the same function (one with yield sleep, another one without) for main thread and web workers separately.

@lars-t-hansen
Copy link

"perhaps" and "small advantage" suggest we need a stronger use case for this kind of behavior change? Or a flag, or a different instruction that allows you to specify which behavior you want?

@dtig
Copy link
Member

dtig commented Jan 20, 2021

Following up on my AI from the CG meeting, this seems like a fairly low risk change to make and we can change the behavior on Chrome to be a runtime trap.

@daxpedda
Copy link

I was wondering if this exception could be cought in Wasm, but unfortunately both Chrome and Firefox don't seem to support it.

Would it be possible to define failure as a catchable Wasm exception instead?


(module
  (import "e" "m" (memory 1 1 shared))
  (func (export "w") (param i64) (result i32)
    try (result i32)
      i32.const 0
      i32.const 0
      local.get 0
      memory.atomic.wait32
    catch_all
      i32.const 3
    end
  )
)
const wasmInstance = new WebAssembly.Instance(wasmModule, { e: { m: new WebAssembly.Memory({ initial: 1, maximum: 1, shared: true }) } })
const { w } = wasmInstance.exports
try { console.log('success: ' + w(BigInt(1000))) } catch (error) { console.log('error: ' + error) }

Chrome output:

error: RuntimeError: Atomics.wait cannot be called in this context

Firefox output:

error: TypeError: waiting is not allowed on this thread

Expected 3 instead.

@tlively
Copy link
Member

tlively commented Feb 27, 2024

In principle we could add exception-throwing variants of these instructions to complement the trapping variants, but it would be quite a bit of work to add to the spec. A workaround you could use today would be to wrap your waits in JS calls that can catch the traps.

@daxpedda
Copy link

In principle we could add exception-throwing variants of these instructions to complement the trapping variants, but it would be quite a bit of work to add to the spec.

My understanding (and after consulting the spec) is that neither trapping nor exception-throwing is specified in the spec. So my suggestion was coming from a place of "if we specify it, could we do it this way?".

@tlively
Copy link
Member

tlively commented Feb 27, 2024

Ah, if you're wondering if we can update this threads proposal to throw instead of trap, then no, it is too late for that. This proposal is already a phase 4 (i.e. finished except for being merged into the spec) and has been shipping in browsers for years. We would have to introduce new throwing variants in a separate, new proposal.

@daxpedda
Copy link

Ah, if you're wondering if we can update this threads proposal to throw instead of trap, then no, it is too late for that.

Please correct me if I'm wrong, but AFAICS the proposal currently specifies neither trapping or throwing.

@tlively
Copy link
Member

tlively commented Feb 27, 2024

Looking at the spec document itself, I don't see where it specifies that the instructions trap on the main thread, but it should be somewhere since that's what they do in practice. @conrad-watt, can you point us to where this is specified?

@conrad-watt
Copy link
Collaborator

conrad-watt commented Feb 28, 2024

I had intended this to fall under the implementation-defined limits spec language, which only allows an implementation to "terminate that computation and report an embedder-specific error to the invoking code" rather than throw any kind of catchable exception in Wasm. But I think there are some missing pieces for this specific restriction - and there should also be a line in the JS-API hooking into the agentCanSuspend() functionality. I'll fix this.

Sorry that the state of the spec has caused confusion, but because implementation-specific runtime limitations are currently spec'd to terminate Wasm execution, and browsers have (in "Web reality") aligned on this restriction at phase 4, there's no longer scope to change things. We'd be open to adding new mechanisms for this in future - there's also desire for throwing versions of other trapping instructions like i32.div so we could possibly solve all these problems in a generic way.

I do see that Firefox is throwing a TypeError and Chrome is throwing a RuntimeError. @eqrion would it be possible to change SpiderMonkey's behaviour to a RuntimeError? This would be more consistent with our JS spec for runtime limitations.

@rossberg
Copy link
Member

As @conrad-watt said, this is a restriction specific to the likes of JS/Web embeddings that introduce the notion of a "main" thread. Wasm itself does not have that restriction, nor does it know different classes of threads.

@dschuff
Copy link
Member

dschuff commented Feb 28, 2024

Another possibility for the place to specify this would be the Web API spec (as opposed to the JS API spec) since as @rossberg mentioned, it's only relevant in JS embeddings that introduce the notion of a "main" thread, which AFAIK applies to the web but not e.g. node.js (e.g. the section of the HTML spec on agents mentions that only worker agents allow blocking JS Atomics APIs).

@conrad-watt
Copy link
Collaborator

@dschuff I believe the ECMAScript agentCanSuspend() hook already covers the embedding-specific behaviour, so I think the JS API is the right place for the restriction to live so long as we correctly reference this hook.

@eqrion
Copy link
Contributor

eqrion commented Feb 28, 2024

I do see that Firefox is throwing a TypeError and Chrome is throwing a RuntimeError. @eqrion would it be possible to change SpiderMonkey's behaviour to a RuntimeError? This would be more consistent with our JS spec for runtime limitations.

Sure, I filed a bug. This was caused by us just throwing the same error that happens on the JS side of things in some shared code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

9 participants