emscripten-core / emscripten

Emscripten: An LLVM-to-WebAssembly Compiler
Other
25.9k stars 3.32k forks source link

Unable to overwrite worker's onmessage when using MODULARIZE #20192

Open miloszmaki opened 1 year ago

miloszmaki commented 1 year ago

Consider this example, which creates a new worker thread. The worker overwrites its onmessage to handle custom messages. Then the main thread sends a custom message to the worker.

#include <cstdio>
#include <emscripten.h>
#include <thread>

int main() {
  printf("hello from main\n");

  std::thread t{[](){
    EM_ASM({
        console.log("hello from thread");
        const def_onmsg = self.onmessage;
        self.onmessage = (e) => {
                console.log("handling message:", e, e.data, e.data.cmd);
                if (e["data"]["cmd"] != "custom1" && e["data"]["cmd"] != "custom2") {
                        def_onmsg(e);
                }
        };
        // self.onmessage({"data": {"cmd": "custom1", "id": 1}}); // this works always
    });
  }};

  EM_ASM({
  // setTimeout(()=>{ // doesn't help either
    const threads = Module["PThread"].pthreads;
    const id = Object.keys(threads)[0];
    const worker = threads[id];
    worker.postMessage({"cmd": "custom2", "id": 2}); // this fails with MODULARIZE
  // },1000);
  });

  return 0;
}

It works properly ("custom2" message gets handled by the worker) when I build with: emcc main.cpp -o test.html --std=c++20 -pthread -s PTHREAD_POOL_SIZE_STRICT=0

However, it stops working (the console prints error "worker.js received unknown command custom2") when I build with: emcc main.cpp -o test.html --std=c++20 -pthread -s PTHREAD_POOL_SIZE_STRICT=0 -s MODULARIZE=1 -s EXPORT_ES6=1

My output of emcc -v is:

emcc (Emscripten gcc/clang-like replacement + linker emulating GNU ld) 3.1.45 (ef3e4e3b044de98e1811546e0bc605c65d3412f4)
clang version 18.0.0 (https://github.com/llvm/llvm-project d1e685df45dc5944b43d2547d0138cd4a3ee4efe)
Target: wasm32-unknown-emscripten
Thread model: posix

Interestingly, it was working with some older versions (at least I checked 3.1.15).

sbc100 commented 1 year ago

I'm not sure we have ever officially support doing this kind of thing. We have had others ask about it in the past though so there are at least few folks who want to do it. If we do want to support it we should add some tests that do this so it doesn't break again.

We should also make some kind of official API for going from a pthread ID to a worker handle in JS I suppose?

miloszmaki commented 1 year ago

I see. That would be great if you decide to support this. I think that being able to establish the communication between workers is quite important. Do you know of any workarounds available currently? One I can think of, is to call some C++ function from JS and then do the thread communication on the C++ side. Do you think it's a good way to go?

sbc100 commented 1 year ago

I see. That would be great if you decide to support this. I think that being able to establish the communication between workers is quite important. Do you know of any workarounds available currently? One I can think of, is to call some C++ function from JS and then do the thread communication on the C++ side. Do you think it's a good way to go?

Normally pthread communication happens through shared memory. is there some reason you can't do that in this case? The fact that pthreads are implemented as web workers is kind of an implementation detail that ideally you would not rely on. Is there some reason you need to use postMessage rather than shared memory?

sbc100 commented 1 year ago

do the thread communication on the C++ side.

Yes, I would recommend doing your thread communication using the pthread APIs (or C++ thread APIs) and shared memory.

miloszmaki commented 1 year ago

Is there some reason you need to use postMessage rather than shared memory?

I'm capturing camera frames on the main thread and need to transfer the ownership of a VideoFrame to the worker thread, which handles the frame by processing its data and eventually calling close() on it. Not sure if this is possible using shared memory only.

sbc100 commented 1 year ago

Yes, I can see that in that case you would need to use postMessage.

@juj doesn't this sounds like a reasonable use case for postMessage or is there some way this can be done using offscreen canvas perhaps?

sbc100 commented 1 year ago

Wait, if you are processing the data using native code then don't you have to copy the data out of the VideoFrame and into linear memory anyway? Why not copy the data out on the main thread?

miloszmaki commented 1 year ago

Right, I'm copying the data for further processing in C++. However, I'm concerned about the efficiency and would like to offload work from the main thread if possible. I think I will need to try and profile the approach you suggested to know if it works for me.

juj commented 1 year ago

I'm not sure we have ever officially support doing this kind of thing.

I think we should support this. It helps composability with other libraries, and helps users utilize the existing constructs that they might have and know about. Alternatively, we should clearly document that the .onmessage parameter is off limits to end users.

I have tried to do this for the recent libraries, e.g. Wasm Workers already utilize addEventListener instead of .onmessage: https://github.com/emscripten-core/emscripten/blob/453e83a3a1b8b9836c136400a2fd2e981a033a4a/src/library_wasm_worker.js#L90-L99 even though it is larger code size.

We might code golf the size a bit for this in the future to use .onmessage with a hypothetical WORKERS_ARE_PRIVATE or something along those lines. There are too many tutorials and other articles that showcase modifying .onmessage that it's better to let users have it.

miloszmaki commented 1 year ago

Why not copy the data out on the main thread?

I tried this approach suggested by @sbc100 and it turns out to work quite well.

miloszmaki commented 1 year ago

However, I'm still a bit concerned with this new approach, since I need to do some kind of synchronization (e.g. mutex) which requires blocking on the main thread (not recommended, as pointed out in the documentation).

miloszmaki commented 2 months ago

@sbc100 @juj any updates on that? Is it possible to merge https://github.com/emscripten-core/emscripten/pull/20235 ?

sbc100 commented 1 month ago

I'd be OK with landing #20235 but I think we should add a test/example and some docs about how to do it in the blessed way.

juj commented 1 month ago

Sorry, I never quite had time to design the tests for that PR. I'll see if I can get a bit of time slotted for this soon.