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

feat: add --expose-gc flag #4

Merged
merged 5 commits into from
Jan 2, 2024
Merged

Conversation

anurag-roy
Copy link
Contributor

Addresses #3

@anurag-roy anurag-roy changed the title Add gc flag feat: add --expose-gc flag Jan 1, 2024
@mcollina
Copy link
Owner

mcollina commented Jan 1, 2024

Can you add it to the docs?

@anurag-roy
Copy link
Contributor Author

I was trying out a few different ways to achieve this.

Since we are using run from node:test here and not spawning a new process, we have two options:

1. Enable gc dynamically (without passing flags to node)

import { setFlagsFromString } from 'node:v8';
import { runInNewContext } from 'node:vm';

if (args.values['expose-gc'] && typeof global.gc !== 'function') {
 setFlagsFromString('--expose-gc');
 global.gc = runInNewContext('gc');
}

I tried this, but was unable to use gc inside my tests

2. Spawn a node child process where we pass the --expose-gc flag

if (args.values['expose-gc'] && typeof global.gc !== 'function') {
 try {
   await execa('node', ['--expose-gc', ...process.argv.slice(1)], {
     stdio: 'inherit',
     env: {
       ...process.env
     }
   })
   process.exit(0)
 } catch (error) {
   process.exit(1)
 }
}

This one does work, but I'm not sure if this is the best way. Any pointers? @mcollina

@mcollina
Copy link
Owner

mcollina commented Jan 1, 2024

I think you can set it to process.env.NODE_OPTIONS before calling run

@anurag-roy
Copy link
Contributor Author

No that will not work, since it isn't a node option it's a v8 flag.

$ node --v8-options

--expose-gc (expose gc extension)
        type: bool  default: false

And passing it as NODE_OPTIONS gives this error

node: --expose-gc is not allowed in NODE_OPTIONS

@mcollina
Copy link
Owner

mcollina commented Jan 1, 2024

I recommend you to open an issue on nodejs to add the possibility to pass nodeOptions to run.

Until that happens, using execa is ok.

@anurag-roy anurag-roy marked this pull request as ready for review January 1, 2024 16:30
Copy link
Owner

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@mcollina mcollina merged commit 87be751 into mcollina:main Jan 2, 2024
2 of 6 checks passed
@mcollina
Copy link
Owner

mcollina commented Jan 2, 2024

I've made a mistake and landed this while tests are failing on Node v21. Could you send a follow-up pr to fix?

@anurag-roy
Copy link
Contributor Author

Yes sure, let me check why they are failing on v21

@anurag-roy
Copy link
Contributor Author

Hey @mcollina you might want to take a look at this, something very weird is happening.

All tests are failing on v21.5, but passing on v21.4. When we are running node borp.js through execa, it seems as if none of the tests inside that child process are running. This is the output from execa

TAP version 13
1..0
# tests 0
# suites 0
# pass 0
# fail 0
# cancelled 0
# skipped 0
----------|---------|----------|---------|---------|-------------------
File      | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s
----------|---------|----------|---------|---------|-------------------
All files |       0 |        0 |       0 |       0 |
----------|---------|----------|---------|---------|-------------------
# todo 0
# duration_ms 8.481937

Do you have any idea what might be the issue?

@mcollina
Copy link
Owner

mcollina commented Jan 2, 2024

Here is the fix: #5

@anurag-roy
Copy link
Contributor Author

Awesome! All tests passing now!

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.

2 participants