Turn off shell by default #2306
Draft
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…e-dotnet-runtime into nagilson-no-shell
some of these are not necessary bc they should only run on unix but you never know
Member
Author
|
Ah so only spawn has boolean|string for shell, not exec.... facepalm |
only spawn can use shell = false spawn uses the direct string. escaping with " " causes it to not find the executable. need to confirm with weird path names that caused issues before but program files seems to work. setx "" parsing also no longer functions properly. noticed that --list-sdks --arch is missing the arch value so fixed that as well
I don't believe we ever use that.
nagilson
commented
Jul 1, 2025
| return []; | ||
| } | ||
|
|
||
| const findSDKsCommand = CommandExecutor.makeCommand(`"${existingPath}"`, ['--list-sdks', '--arch', requestedArchitecture]); |
Member
Author
There was a problem hiding this comment.
Does this break Linux/Mac scenarios in the shell? I think we need to look at this from a broader context.
nagilson
commented
Jul 1, 2025
| options.shell ??= true; | ||
| options.shell ??= os.platform() === 'win32' ? false : true; | ||
| // ^ Windows systemcalls (node.js process library uses process_wrap which uses process.cc which makes system calls) | ||
| // Windows seems to resolve the PATH by default in a system call. Unix system calls do not. |
Member
Author
There was a problem hiding this comment.
Can we just respect shell and let the shell do this?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What's this
This turns off the 'shell' when running commands for us in vscode. This provides a huge performance boost.
CICD Logs with Spawn:
--info 27.90. 29.21. 26.21. 2.68. (error) 27.
--list-runtimes 30.39. 2.32. (error) 31.
Without Spawn:
--list-runtimes 50. 45. 32 (error).
--info: 46. 41. 32. (error)
Explanation and Context
When spawning a process in node.js, it uses a shell. The shell takes a lot of time. The shell defaults:
Windows Default:
process.env.ComSpecorcmd.exeif undefined.Unix Default:
/bin/shHere's what doesn't work without a shell:
&(To make a background process)/* ?Globbing to get wild cards> << >RedirectionI/O|Piping - Chain 1 command output to the next.Shell Environment Variablesthat aren't set on the system but instead a profile that's launched with the SHELL"file"escaping.process.envis used as env by default regardless of shell.Now, here's a rough list of everything we call:
Fun part:
process.spawnmakes a system call.https://github.com/nodejs/node/blob/5fe78006834011621969a76b4f2d98c0e0039b33/deps/uv/src/unix/process.c#L966
https://github.com/nodejs/node/blob/5fe78006834011621969a76b4f2d98c0e0039b33/src/process_wrap.cc#L348C13-L348C21
https://github.com/nodejs/node/blob/5fe78006834011621969a76b4f2d98c0e0039b33/src/env_properties.h#L171
It looks like system calls on Windows resolve the PATH by default without a shell, but they do not on Unix.
Node process uses
CreateProcessWandexecve.Windows:
CreateProcessA function (processthreadsapi.h) - Win32 apps | Microsoft Learn
“The directories that are listed in the PATH environment variable [are searched].”
https://github.com/libuv/libuv/blob/b00c5d1a09c094020044e79e19f478a25b8e1431/src/win/process.c#L1067
Unix:
execve(2) - Linux manual page
[Use other] standardised variants of this function provided by libc, including ones that search the PATH environment variable.