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

Workers keep running longer than expected #152

Open
Schaeff opened this issue Jun 1, 2022 · 7 comments · Fixed by firstbatchxyz/hollowdb#42
Open

Workers keep running longer than expected #152

Schaeff opened this issue Jun 1, 2022 · 7 comments · Fixed by firstbatchxyz/hollowdb#42

Comments

@Schaeff
Copy link

Schaeff commented Jun 1, 2022

While using mocha to test some snarkjs setup and proof generation, the test suite ends up hanging after all tests pass.
I checked why that was the case using why-is-node-running in my after function and the result points to worker threads in snarkjs:

# WORKER
node:internal/async_hooks:201                                                        
node:internal/worker:185                                                             
/node_modules/web-worker/cjs/node.js:111       - const worker = new threads.Worker(__filename, {
/node_modules/ffjavascript/build/main.cjs:5244 - tm.workers[i] = new Worker__default["default"](workerSource);
/node_modules/ffjavascript/build/main.cjs:6538 - const tm = await buildThreadManager(params.wasm, params.singleThread);
/node_modules/ffjavascript/build/main.cjs:6635 - const curve = await buildEngine(params);
/node_modules/snarkjs/build/main.cjs:53        - curve = await ffjavascript.buildBn128();
/node_modules/snarkjs/build/main.cjs:1435      - const curve = await getCurveFromQ(q);
/node_modules/snarkjs/build/main.cjs:4082      - const {curve, power} = await readPTauHeader(fdPTau, sectionsPTau);

In the tests to this repo I found the following:

    before( async () => {
        curve = await getCurveFromName("bn128");
//        curve.Fr.s = 10;
    });
    after( async () => {
        await curve.terminate();
        // console.log(process._getActiveHandles());
        // console.log(process._getActiveRequests());
    });

If calling curve.terminate() is the solution to close the workers, is there a way to do the same thing when using, say, snarkjs.groth16.prove(zkeyPath, witnessPath)?

Apologies if I'm missing something obvious, my javascript is a bit rusty!

Cheers

@phated
Copy link
Contributor

phated commented Jun 1, 2022

Which version of snarkjs is this happening on? I did work in more recent versions of ffjavascript that should have solved this.

@Schaeff
Copy link
Author

Schaeff commented Jun 1, 2022

Thanks for the quick response! package-lock.json has 0.4.19

@phated
Copy link
Contributor

phated commented Jun 23, 2022

@Schaeff I'm still looking into this, but I did remember that ffjavascript attaches the curves to a global object. So if you are using bn128, you could do globalThis.curve_bn128.terminate()

I'm on the fence if snarkjs should terminate the curves itself, since some code might do multiple operations with the same curve and we don't want to re-initialize it. Maybe we could terminate them only in the CLI or maybe behind a flag. Thoughts?

@Schaeff
Copy link
Author

Schaeff commented Jun 28, 2022

This worked, thanks! I do not have a strong opinion, I just wanted the tests to end cleanly. Maybe documenting this detail better would be enough? This can be closed as far as I am concerned.

@biscuitdey

This comment was marked as duplicate.

@erhant
Copy link

erhant commented Jul 5, 2023

I also had this problem with Jest and I would like to note that --detectOpenHandles does not detect this problem. I was just --forceExit'ing until I saw the solution here. As a snippet, just add:

afterAll(async () => {
  // you can @ts-ignore if you are using ts-jest or such
  await globalThis.curve_bn128.terminate();
});

@SnickerChar
Copy link

Was experiencing this exact problem and started looking at the issues. I'm glad there's a fix available, and indeed, Jest's --detectOpenHandles does not detect this!

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 a pull request may close this issue.

5 participants