threads icon indicating copy to clipboard operation
threads copied to clipboard

memory.atomic.wait32 on the main browser thread

Open juj opened this issue 3 years ago • 17 comments

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)

juj avatar Jan 15 '21 12:01 juj

@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.

conrad-watt avatar Jan 15 '21 12:01 conrad-watt

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

juj avatar Jan 15 '21 13:01 juj

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.

conrad-watt avatar Jan 15 '21 20:01 conrad-watt

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.

juj avatar Jan 15 '21 21:01 juj

"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?

lars-t-hansen avatar Jan 17 '21 14:01 lars-t-hansen

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.

dtig avatar Jan 20 '21 22:01 dtig

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.

daxpedda avatar Feb 17 '24 10:02 daxpedda

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.

tlively avatar Feb 27 '24 02:02 tlively

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?".

daxpedda avatar Feb 27 '24 22:02 daxpedda

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.

tlively avatar Feb 27 '24 23:02 tlively

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.

daxpedda avatar Feb 27 '24 23:02 daxpedda

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?

tlively avatar Feb 27 '24 23:02 tlively

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.

conrad-watt avatar Feb 28 '24 03:02 conrad-watt

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.

rossberg avatar Feb 28 '24 06:02 rossberg

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).

dschuff avatar Feb 28 '24 17:02 dschuff

@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.

conrad-watt avatar Feb 28 '24 17:02 conrad-watt

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.

eqrion avatar Feb 28 '24 18:02 eqrion