JS binding segfaults in worker thread

[tdewolff]

From #379: the worker test case fails due to a race condition. Even when calling an empty function in Go it fails. It gets stuck at worker.terminate() when calling runtime.GC() in the clean up function. Looks like something we need to take up with Go or with Node.

Ah no, the os.Exit is terminating the whole node process: https://github.com/dotcore64/rollup-plugin-tdewolff-minify/runs/7272307346?check_suite_focus=true It is exiting after the first test...

[tdewolff]

See #517

[perrin4869]

Yeah I completely agree with you, I was thinking that maybe the minifier may have some memory that need cleaning up internally, but I was also fearing that the go runtime has internal memory used by its runtime... since you mention that calling an empty function results in a segfault as well, it seems option number 2 is correct ><;; Might be worth to try out tinygo here? I don't know much about golang at all, just throwing out ideas ><;; btw, I think it might be best to remove the os.Exit(1) for now, since that will have adverse effects at this point :)

[perrin4869]

It feels really bad after all the hard work on the C++ bindings, but maybe worth it to look into wasm bindings? https://www.npmjs.com/package/golang-wasm

[perrin4869]

Maybe related: https://tinygo.org/docs/guides/webassembly/

[tdewolff]

Hm going the WASM way would be possible but I'm unsure about the implications.

I've added a sleep function to avoid races, it seems to work locally, can you verify?

[tdewolff]

Doesn't seem to help much, or only sometimes. I'm out of ideas, but honestly this is not the only library that fails with worker threads. Is this a common way to use NAPI libraries? How important is supporting worker threads?

[perrin4869]

I think it would be pretty common. NAPI libraries are usually used to perform CPU heavy tasks, and it's also critical to run them in separate threads to avoid blocking the event loop. Also it enables running this library in parallel...

[perrin4869]

But it's ok, it's not urgent to fix this :)

[tdewolff]

Hopefully answers to this issue might shed some light: https://github.com/nodejs/node-gyp/issues/2695

[tdewolff]

This has likely to do with a very similar issue with the Python binding, see https://github.com/tdewolff/minify/issues/535#issuecomment-1256793958

Basically, Go cannot be forked since it will only fork the main thread and not the GC thread. The forked threads from the main thread then hang when trying to garbage collect. For some reason, the fix is quite the inverse: for Python we should load the library in each thread (or use the spawning instead of the forking strategy). For JS we need to load the library globally and call its functions in each thread, apparently the await import of the same library does only initialize it once (the GC thread) but keeps running on different forked threads.

[tdewolff]

Maybe related to https://github.com/nodejs/node/issues/21783#issuecomment-429637117, see https://github.com/nodejs/node/blob/main/doc/api/addons.md#context-aware-addons and https://napi.inspiredware.com/special-topics/context-awareness.html