js-promise-integration icon indicating copy to clipboard operation
js-promise-integration copied to clipboard

Add way to query whether suspender will trap?

Open hoodmane opened this issue 1 year ago • 3 comments

The spec says the following:

The WebAssembly function returned by WebAssembly.Function is a function whose behavior is determined as follows: ... 3. Traps if suspender's state is not Active[caller] for some caller 4. Lets frames be the stack frames since caller 5. Traps if there are any frames of non-suspendable functions in frames

It's reasonably easy for us to manually track whether our suspender is in state Active[caller]. On the other hand, it's much harder to know if there are non-suspendable functions on the stack, especially in more complex code. We can't ask forgiveness because there is no way to recover from a trap from within wasm. It would be helpful if there were a way to ask for permission so that we could avoid the trap ahead of time.

My specific use case is as follows:

pyodide.runPythonSyncifying(`
from asyncio import sleep, ensure_future
from ctypes import CFUNCTYPE, c_int

def f():
   print(1)
   ensure_future(sleep(1)).syncify()
   print(2)

# works perfectly:
f()

# Force a trampoline call through JavaScript. Fatally fails because 
# 5. Traps if there are any frames of non-suspendable functions in frames
ctypes_f = CFUNCTYPE(None)(f)
ctypes_f()
`);

I would like to fix it to raise a Python exception instead. You can try it out in the debug console here in a browser with JSPI enabled: https://pyodide.org/en/stable/console.html

hoodmane avatar Feb 17 '24 20:02 hoodmane

I am afraid that I don't believe that it would be high priority to implement the kind of test you are asking for. However, priorities have a habit of reflecting shifting times.

fgmccabe avatar Feb 20 '24 17:02 fgmccabe

I don't believe that it would be high priority to implement the kind of test you are asking for

I have the same impression but it can't hurt to ask. My guess is that a lot of other people with complex JSPI use cases may have similar problems, so maybe once there is more user feedback in favor of it... Of course we are super excited about the JSPI even without this improvement.

It seems to me that this clearly shouldn't get an instruction but if a mechanism like wasm builtin imports is added, it shouldn't be too hard to spec a function that tests for this.

hoodmane avatar Feb 20 '24 20:02 hoodmane

It does feel to me also that for use cases beyond the obvious uses (e.g. waiting for async fetch) there are likely to be other cases where people want to use this functionality for things where call stacks are complex and it is hard to track what is on there without instrumenting everything. Especially when the error drops you out of the wasm stack without being catchable. Especially with emscripten and binaryen stuff that mixes JavaScript and wasm I wouldn't be surprised if there are weird edge cases that we need to know about.

joemarshall avatar Jun 23 '24 22:06 joemarshall

I'd like to voice a +1 here, from the perspective of the Scala.js toolchain.

Essentially our requirement is similar to the original one: we would like to expose illegal attempts to call a Suspending function as catchable Scala.js exceptions, rather than traps.

Our toolchain, at least when compiled in the "debug" mode that checks everything, currently guarantees that the Wasm code it produces never traps. In any situation where a trap might occur (think null dereferencing for example), we test and throw a catchable exception instead. This is a very strong guarantee that ensures, notably, that testing frameworks can catch mistakes without crashing themselves, for example!

If we add JSPI to our language features, however, we basically have to break that guarantee!

Could we perhaps have a way to throw an EH exception rather than trapping? It could even be a JavaScript exception, which would make sense to me, since we are talking to a JavaScript API after all.

Perhaps this should even be the default way to report failures here? If not, maybe an options argument to new WebAssembly.Suspending would allow us to configure the failure mode?

"myImport": new WebAssembly.Suspending(fun, { failureMode: 'exception' }),

?

sjrd avatar Oct 30 '24 10:10 sjrd

@sjrd one thing that helps is that with the new catch_all_ref it's now possible to do:

try
    call $suspending_func
catch_all_ref
    call $update_error
    throw_ref
end

and then $update_error can be a JS function that inspects the error to see if it's a can't suspend through JS frames trap and if so convert it into an exception that your other code knows how to handle. If you do this consistently whenever you call a suspending function, then there's no risk that the error actually came from further down the stack.

This is an imperfect solution because if you call into other peoples' code and they trigger this trap, the error may be unrecoverable and we are at risk of continuing on with a corrupted runtime. Maybe by inspecting the stack in the error, we can confirm that the current frame is the bottom-most in the error's stack.

hoodmane avatar Oct 30 '24 11:10 hoodmane

Even catch_all_ref cannot catch traps. I just tested on Chrome. The engines enforce this with a dedicated exception for trap exceptions, so that Wasm can never catch them.


Also, a catch_all_ref gives you an exnref, not an externref. You can't pass that to JS. You can only use it to rethrow the exception with throw_ref. To catch a JS exception as an externref you need a catch $jstag where $jstag is an import of WebAssembly.JSTag (but again, that cannot catch a trap).

sjrd avatar Oct 30 '24 13:10 sjrd

Right, traps are unrelated to exceptions (even though they are converted to JS exceptions on the outside, when Wasm is embedded in JS). They are fatal program failures that abruptly terminate Wasm execution.

rossberg avatar Oct 30 '24 15:10 rossberg

Interesting, thanks for explaining! I kept thinking of this as a potential fallback if I can't devise a good enough heuristic to decide whether a trap may occur but obviously hadn't tried to implement it yet. It would also be really helpful to have a way to either ask permission to do a call_indirect or to recover from the signature mismatch trap, but js-type-reflection goes a long way to helping with that issue.

hoodmane avatar Oct 30 '24 15:10 hoodmane

There is an agenda item in the 11/04/24 meeting of the stacks subgroup that refers to this issue.

fgmccabe avatar Oct 31 '24 20:10 fgmccabe

Are we allowed to join that meeting, or do we have to specifically be part of the stacks subgroup to do so? I found a Zoom link publicly available on the meetings repo, but that seems like a poor excuse to just barge in without asking.

sjrd avatar Nov 01 '24 10:11 sjrd

The primary requirement is that you are a member of the CG. However, we often have visitors; so you can join as a visitor (if you are not a member of the CG).

fgmccabe avatar Nov 01 '24 15:11 fgmccabe

I'm a member of the CG, even if I rarely join the meetings. I'll attend this one, then. Thank you.

sjrd avatar Nov 01 '24 16:11 sjrd

A follow up: it seems that all other WebAssembly.RuntimeErrors represent non-recoverable situations (i.e., traps). I also do not feel that issuing a JavaScript TypeError is appropriate for this situation. So, going forward: either we have the first recoverable RuntimeError or go with the original request here: of adding a predicate (e.g., IsSafeToSuspend). Personally, I don't like either solution. In the longer term, I believe that it is entirely possible that the wasm community may adopt a mechanism for recovering from traps (e.g., to allow test runners to work properly). But, there is no guarantee about that.

Thoughts?

fgmccabe avatar Nov 08 '24 17:11 fgmccabe

A follow up: it seems that all other WebAssembly.RuntimeErrors represent non-recoverable situations (i.e., traps). I also do not feel that issuing a JavaScript TypeError is appropriate for this situation. So, going forward: either we have the first recoverable RuntimeError or go with the original request here: of adding a predicate (e.g., IsSafeToSuspend). Personally, I don't like either solution. In the longer term, I believe that it is entirely possible that the wasm community may adopt a mechanism for recovering from traps (e.g., to allow test runners to work properly). But, there is no guarantee about that.

Thoughts?

How about adding a WebAssembly.SuspendError class here [1]?

[1] https://webassembly.github.io/spec/js-api/index.html#error-objects

eqrion avatar Nov 08 '24 17:11 eqrion

It is still a 'first'. But, better than using RuntimeError.

fgmccabe avatar Nov 08 '24 17:11 fgmccabe

I still think the IsSafeToSuspend query would be cleanest to use because with a try/catch chance of catching a failed suspend further down the stack. But I'm happy to see any fix =)

hoodmane avatar Nov 09 '24 11:11 hoodmane

Have landed change to spec (&V8): we throw a SuspendError instead of trapping.

fgmccabe avatar Dec 16 '24 23:12 fgmccabe