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

test_runner: do not spawn run in child processes to prevent infinite loops #48849

Closed
wants to merge 1 commit into from

Conversation

koh110
Copy link
Contributor

@koh110 koh110 commented Jul 20, 2023

Infinite loop occurs when the file in options.files has run.
I send this PR to avoid the risk of occuring infinite loop.

Ref: #48823

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/test_runner

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. test_runner Issues and PRs related to the test runner subsystem. labels Jul 20, 2023
@koh110 koh110 force-pushed the fix/test-runner-infinite-loop branch 2 times, most recently from 7db06c7 to 73e4933 Compare July 20, 2023 11:51
@koh110 koh110 force-pushed the fix/test-runner-infinite-loop branch from 73e4933 to 1b31fa1 Compare July 20, 2023 14:42
Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

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

I don't understand these changes - why would you both run inside the file and run with --test?

Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

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

Thinking about this a bit more I find this behavior pretty confusing to have a specific exception to the options file.

Even if we did this I'd expect the check to be "is this the options file" and not "is it a worker process".

I'll wait for @MoLow or @cjihrig to weigh in but I'm -0.5

@koh110
Copy link
Contributor Author

koh110 commented Jul 20, 2023

If run is included in the file given to files, I think it have risk to fork the process infinitely.
I think that there is a possibility that the user may give it in an unexpected way, such as when specifying it with glob.
So I thought that run called from run should not start.

In order to reduce the number of test files, I wrote the test in the same file as run, I think there is a similar problem when run calls run in anothor file.

@MoLow
Copy link
Member

MoLow commented Jul 20, 2023

I am also -1 on this change since this situation only happens when you explicitly run() in a directory including this file.
it can easily be worked-around by changing the filename, or by changing the files parameter.
if you don't want either of those, also explicitly checking for process.env.NODE_TEST_CONTEXT in userland will work

@cjihrig
Copy link
Contributor

cjihrig commented Jul 20, 2023

I would also prefer not to land this. It might be worth a small note in the options.files docs.

@koh110
Copy link
Contributor Author

koh110 commented Jul 22, 2023

How do you think docs should be changed? I'll do it when I have time.

@@ -17,6 +18,23 @@ describe('require(\'node:test\').run', { concurrency: true }, () => {
for await (const _ of stream);
});

it('should run without infinite loops (self path)', { timeout: 10000 }, async () => {
execFileSync(
Copy link

@Mifrill Mifrill Jul 28, 2023

Choose a reason for hiding this comment

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

Since execFileSync is a synchronous function, it blocks the event loop until the command execution is complete and returns the result as a Buffer or a string, depending on the encoding used, hence to check execution in case of infinite loops, I think should be better to apply await explicitly:

import { promisify } from 'util';
import { execFile } from 'child_process';

const execFilePromise = promisify(execFile);
Suggested change
execFileSync(
await execFilePromise(

@cjihrig
Copy link
Contributor

cjihrig commented Sep 27, 2023

There seems to be agreement that we should not land this. I'm going to close this out, but thanks for the PR.

@cjihrig cjihrig closed this Sep 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-ci PRs that need a full CI run. test_runner Issues and PRs related to the test runner subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants