Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Node.JS bindings Worker threads segfault reproduction #510

Merged
merged 2 commits into from
Jul 10, 2022

Conversation

perrin4869
Copy link
Contributor

I added a minimal test that reproduces the segfault issue. Unfortunately using the global stack didn't fix the issue
I'm afraid that maybe the go runtime might have internal resources that cannot be cleaned, in which case this issue could maybe not ever be solved? Really hope not...
Now, I also noticed if the module is loaded in both the main thread and in the worker thread, the application does not segfault and the tests pass beautifully (i.e, instead of const { config, string } = await import('@tdewolff/minify'), do import { config, string } from '@tdewolff/minify')

@codecov-commenter
Copy link

Codecov Report

Merging #510 (a01c7d6) into master (55b8f83) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #510   +/-   ##
=======================================
  Coverage   83.16%   83.16%           
=======================================
  Files          25       25           
  Lines        6678     6678           
=======================================
  Hits         5554     5554           
  Misses       1071     1071           
  Partials       53       53           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 55b8f83...a01c7d6. Read the comment docs.

@tdewolff
Copy link
Owner

tdewolff commented Jul 4, 2022

Thanks for the test case, it segfaults here too. I'll look into finding the root cause

@tdewolff
Copy link
Owner

tdewolff commented Jul 4, 2022

Been trying to use https://www.npmjs.com/package/segfault-handler but it requires the require syntax and when changing it to the import syntax it doesn't "self-register". I've tried a couple of things to get it to work but with little luck...

@tdewolff
Copy link
Owner

tdewolff commented Jul 7, 2022

Update, running the following script of CommonJS also generates the segmentation fault:

const { Worker, isMainThread, workerData, parentPort } = require('node:worker_threads');

if (isMainThread) {
    let input = "<html><span style=\"color:#ff0000;\">A  phrase</span>";
    const worker = new Worker(new URL('file:///home/taco/go/src/github.com/tdewolff/minify/bindings/js/test/worker.js'), {
        workerData: input,
    });

    let expected = "<html><span style=color:red>A phrase</span>";
    let output = new Promise((resolve, reject) => {
        worker.on('message', resolve);
        worker.on('error', reject);
        worker.on('exit', (code) => {
            if (code !== 0)
            reject(new Error(`Worker stopped with exit code ${code}`));
        });
    })
} else {
    const { string } = require('@tdewolff/minify');
    string("text/html", workerData);
}

However, using the segfault-handler as mentioned above, the segmentation fault is not caught! :-S

@tdewolff tdewolff closed this in eeb7be4 Jul 10, 2022
@tdewolff
Copy link
Owner

I've found it, I had to add a clean up hook and call os.Exit(0) explicitly!

@perrin4869
Copy link
Contributor Author

Thanks!!! This is working perfectly here: dotcore64/rollup-plugin-tdewolff-minify#7
Maybe it would be a good idea to merge this instead of closing to keep the regression test?

@tdewolff tdewolff reopened this Jul 10, 2022
@tdewolff tdewolff marked this pull request as ready for review July 10, 2022 16:33
@tdewolff tdewolff merged commit 2eed2f6 into tdewolff:master Jul 10, 2022
@perrin4869
Copy link
Contributor Author

Ah just need to update

"version": "2.11.11",
to 2.12.0 to deploy to npm :)

@tdewolff
Copy link
Owner

That ought to happen automatically in the Makefile that we invoke from the workflow. Just now I'm running the build script: https://github.com/tdewolff/minify/actions/runs/2645307256

@perrin4869
Copy link
Contributor Author

Dammit this is actually not a good solution, os.Exit terminates the node process:

https://github.com/dotcore64/rollup-plugin-tdewolff-minify/runs/7272307346?check_suite_focus=true

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants