ceifa / wasmoon

A real lua 5.4 VM with JS bindings made with webassembly
MIT License
449 stars 29 forks source link

Awaiting multiple asynchronous calls in callback function #105

Open rkristof opened 6 months ago

rkristof commented 6 months ago

I'm using the workaround from the readme to allow using await in callback functions. It works well when there is only one asynchronous call inside the callback function. But if there are more, it will produce an infinite loop after the first call is finished. I managed to find a solution for this, so I thought I would post it here. Also I'm not very experienced with coroutines, so maybe you could tell me if my solution is acceptable.

Basically what I did was to extend the condition at the end of the local "step" function.

Before:

if safe and result == Promise.resolve(result) then
    result:finally(step)
else
    step()
end

After:

if safe and (result == Promise.resolve(result) or result ~= nil) then
    result:finally(step)
else
    step()
end
ceifa commented 3 months ago

Here is how I use it: https://github.com/ceifa/demoon/blob/main/src/std.lua It works great for me, do you have a minimal repro for this infinite loop?

gudzpoz commented 3 months ago

I don't know if this is the same issue, but I managed to trigger a use-after-free issue when using multiple async calls grouped by Promise.all:

import { LuaFactory } from 'wasmoon';

const factory = new LuaFactory();
const L = await factory.createEngine();

const sleep = (ms) => new Promise((r) => setTimeout(r, ms));
L.global.set('sleep', sleep);

const results = await Promise.all([
  L.doString('sleep(400):await(); return 1'),
  sleep(100)
    .then(() => L.doString('sleep(500):await(); return 2')),
  sleep(200)
    .then(() => L.doString('sleep(3000):await(); return 3')),
  sleep(300)
    .then(() => L.doString('sleep(1000):await(); collectgarbage(); return 4')),
]);

console.log(results);

Basically, the threadIndex tracked does not guarantee to point to the same thread after the other thread manipulates the stack.

https://github.com/ceifa/wasmoon/blob/87c783a8d0bf65afeda933fa47d8dc996be53bb3/src/engine.ts#L81

https://github.com/ceifa/wasmoon/blob/87c783a8d0bf65afeda933fa47d8dc996be53bb3/src/engine.ts#L97

The code snippet above does the following:

  1. Creates four threads (Stack now: [T1, T2, T3, T4])
  2. T1 removes itself from stack (Stack now: [T2, T3, T4])
    • However, threadIndex tracked by T{N} is still N, which now points to T{N+1}.
  3. T2 ends and removes T3 from stack.
  4. T4 ensures to call collectgarbage(), which deletes T3 now that nowhere references it.
  5. T3 is freed but used by the return promise, causing a RuntimeError: memory access out of bounds.

Maybe luaL_ref should be used instead?

rkristof commented 3 months ago

It seems I cannot reproduce the issue anymore. I thought it would be easy to reproduce, but I've done some testing where I combined multiple callback functions with asynchronous calls, and the original workaround from the readme worked fine every time. I guess my case was either very specific (which I cannot recreate now), or I made some other mistake somewhere.