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

Subprocess should emit error if executable path doesn't exist in Windows #469

Closed
lynxtaa opened this issue Jun 10, 2021 · 15 comments
Closed

Comments

@lynxtaa
Copy link

lynxtaa commented Jun 10, 2021

Returned from execa subprocess should emit 'error' event if executable path doesn't exist in Windows. Just like spawn and execFile in child_process

Consider this code:

const { spawn } = require('child_process')
const execa = require('execa')

const p1 = spawn('wrong-path')
p1.on('error', err => {
	console.log(err) // logs ENONENT error
})

const p2 = execa('wrong-path')
p2.on('error', err => {
	console.log(err) // never called in Windows, but called in Linux
})
@lynxtaa lynxtaa changed the title Subprocess should emit error if executable path doesn't exist Subprocess should emit error if executable path doesn't exist in Windows Jun 10, 2021
@ehmicky
Copy link
Collaborator

ehmicky commented Jun 10, 2021

Hi @lynxtaa,

Thanks for reaching out.
Which version of execa and of Node.js are you using? I cannot reproduce this behavior.

This is an example with running your example on my machine on Windows, with latest execa and Node.js 16.0.0. Note: I replaced the second wrong-path by wrong-path-2 to differentiate them.

windows

A potential issue you might be experiencing is if you forgot to add a .catch() handler to the return value of execa. The return value is both a child process and a promise. On errors, that promise is rejected. Unhandled promise rejections make the process exit by default on Node.js, which might prevent further code (like the error event handler) from running.

Aside from this, execa actually calls spawn() under the hood and just returns the child process as is, so there should not be any difference of behavior.

@lynxtaa
Copy link
Author

lynxtaa commented Jun 10, 2021

@ehmicky I'm running Node@16.3.0 and execa@5.1.1.

A potential issue you might be experiencing is if you forgot to add a .catch() handler to the return value of execa. The return value is both a child process and a promise. On errors, that promise is rejected. Unhandled promise rejections make the process exit by default on Node.js, which might prevent further code (like the error event handler) from running.

I've tried running node --unhandled-rejections=warn t.js -- same issue

$ node  --unhandled-rejections=warn t.js
Error: spawn wrong-path ENOENT
    at Process.ChildProcess._handle.onexit (node:internal/child_process:282:19)
    at onErrorNT (node:internal/child_process:480:16)
    at processTicksAndRejections (node:internal/process/task_queues:83:21) {
  errno: -4058,
  code: 'ENOENT',
  syscall: 'spawn wrong-path',
  path: 'wrong-path',
  spawnargs: []
}

@ehmicky
Copy link
Collaborator

ehmicky commented Jun 10, 2021

Does t.js contain any additional code beside the sample above?
Considering I get a different behavior when running it myself on Windows (in both cmd.exe and Bash), I am curious which differences of environment might explain that we get different results? Is there any additional reason you think might explain those differences?

@lynxtaa
Copy link
Author

lynxtaa commented Jun 10, 2021

Does t.js contain any additional code beside the sample above?

No, that's all. I've reproduced this problem on three different Windows 10 machines running Node 16.3.0 and Node 14.17.0. In WSL in works okay. To be sure it has time to handle an error, I'm running this code:

const execa = require('execa')

const p = execa('wrong-path')
p.on('error', err => {
  console.log(err) // never called in Windows, but called in Linux
})

setTimeout(() => {}, 1000)

Still no luck

@ehmicky
Copy link
Collaborator

ehmicky commented Jun 10, 2021

Would it be possible to create a GitHub repository to reproduce this behavior, with a GitHub action running on Windows?
This could help us figure out what is happening. Thanks!

@lynxtaa
Copy link
Author

lynxtaa commented Jun 10, 2021

@ehmicky
Copy link
Collaborator

ehmicky commented Jun 10, 2021

Thanks a lot @lynxtaa, that's perfect 👍
On your machine, are you using Powershell? If so, could you try using cmd.exe instead?

@lynxtaa
Copy link
Author

lynxtaa commented Jun 10, 2021

I'm using cmd.exe

@lynxtaa
Copy link
Author

lynxtaa commented Jun 10, 2021

Running with cmd in CI. Same result:

asd

@lynxtaa
Copy link
Author

lynxtaa commented Jun 10, 2021

After further investigation I think the issue is with cross-spawn arguments parsing. Since then binary wrong-path doesn't exist cross-spawn will interpret it as a shell command. After adding console.log(parsed) here

execa/index.js

Line 83 in 9216ec8

spawned = childProcess.spawn(parsed.file, parsed.args, parsed.options);
and running on Windows:

{
  file: 'C:\\WINDOWS\\system32\\cmd.exe',
  args: [ '/q', '/d', '/s', '/c', '"wrong-path"' ], 
  options: {
    maxBuffer: 100000000,
    buffer: true,
    stripFinalNewline: true,
    extendEnv: true,
    preferLocal: false,

Running on Linux:

{
  file: 'wrong-path',
  args: [],
  options: {
    maxBuffer: 100000000,
    buffer: true,
    stripFinalNewline: true,
    extendEnv: true,
    preferLocal: false,

@lynxtaa
Copy link
Author

lynxtaa commented Jun 11, 2021

Related moxystudio/node-cross-spawn#16

@ehmicky
Copy link
Collaborator

ehmicky commented Jun 11, 2021

Thanks for digging into this @lynxtaa!

The actual issue seems to be the following: Node.js detects ENOENT on Windows by translating many Windows system error codes to ENOENT in libuv (link to code). When this happens, an error event is emitted on the child_process (link to code). However, node-cross-spawn sometimes wraps the command with cmd.exe in order to support shebangs. This makes the above Node.js behavior not work anymore. This is why you're experiencing different behaviors between Execa and child_process.

libuv has access to the Windows API system error codes. However, we cannot use those error codes userland (since they are abstracted away by libuv). Unfortunately, the solution implemented in node-cross-spawn, which relies on the exit code being 1 is not reliable as described in moxystudio/node-cross-spawn#104, which is why Execa avoids it.

@lynxtaa
Copy link
Author

lynxtaa commented Jun 12, 2021

@ehmicky Thanks you for this detailed explanation.

There are cases when we are running a subprocess for a very long time and awaiting a promise is undesirable. To detect a startup failure with execa on Windows I may use something like this for now:

await Promise.race([
  subprocess,
  new Promise(resolve => setTimeout(resolve, 1000))
])

But I still think some kind of error should be emitted when using on('error', ...), maybe not ENOENT but exiting with code 1 error which I can see, when using await execa(...). What do you think?

@ehmicky
Copy link
Collaborator

ehmicky commented Jun 12, 2021

I'm realizing your main problem might be: you want to handle process startup failures separately from post-startup failures. Am I correct to assume this?

If so, this problem would cover not only non-existing files but other types of startup failures as well. I actually have had that same problem myself when using Execa in other projects.

The approach you're describing works, but it relies on timeouts, i.e. it makes the code time-dependent. Also, it temporarily stops the code execution. I agree with you that there should ideally be a better way to handle startup failures.

For clarity, we should dissociate the following three types of startup failures.

First type

The first type of startup failures are triggered synchronously with child_process.spawn(). This includes syntax errors.

import { spawn } from 'child_process'

try {
  const subprocess = spawn('example', { cwd: 1 })
  return subprocess
} catch (error) {
  console.log(error)
}

Since Execa always returns a Promise, those require using Promise.catch() or await. The approach you're describing works perfectly for this purpose.

// `util.promisify(setTimeout)` can be used instead for Node <15.0.0
import { setTimeout } from 'timers/promises'

const subprocess = execa('example', { cwd: 1 })
try {
  await Promise.race([subprocess, setTimeout(0)])
  return subprocess
} catch (error) {
  console.log(error)
}

Second type

The second type of startup failures are triggered with the error event on startup. This includes the file not existing or not being executable.

import { spawn } from 'child_process'
import pEvent from 'p-event'

const subprocess = spawn('does_not_exist')
try {
  await pEvent(subprocess, { rejectionEvents: ['error'] })
} catch (error) {
  console.log(error)
}

Execa handles the error event in a new Promise() callback (link to code). Due to this, handling those errors actually requires doing setTimeout(0) twice, which is rather awkward.

const subprocess = execa('does_not_exist')
try {
  await Promise.race([subprocess, waitTwice()])
  return subprocess
} catch (error) {
  console.log(error)
}

const waitTwice = async function() {
  await setTimeout(0)
  await setTimeout(0)
}

Third type

The third type of startup failures are happening inside the process as it starts executing. This includes spawning a Node.js process which tries to require() a non-existing dependency.

Those are time-dependent since they are only triggered after the process has fully spawned and has started to execute. Based on this, it is difficult to separate those errors from post-startup errors. A timeout could be used. You could also detect specific errors using the exit code, terminal signal, error code or error message. However, both of those solutions are brittle.

A more reliable way could be to use some IPC mechanism (including but not limited to stdio: 'ipc') for the child process to notify its parent once it's been successfully started. However, this requires some implementation work. Also this only works when the child process is yours, not an external program.

Problem on Windows

Most of the time, non-existing files fall into the second type of startup failures. However, on Windows, under specific circumstances, Execa (via node-cross-spawn) wraps the command with cmd.exe. As a result, non-existing files then fall into the third type of startup failures, which makes them harder to handle. Unfortunately, that problem is hard to solve since removing cmd.exe would remove the support for shebangs on Windows.

@lynxtaa
Copy link
Author

lynxtaa commented Jun 14, 2021

I'm very grateful for this detailed write-up. Thank you, @ehmicky ❤️ !

Yeah, I've wanted to use execa to run GUI application and only check if it launched successfully (e.g. path to a binary file exists) and don't wait for it to exit since it may take a long time.

Previously I've used child_process.spawn with on('error') event listener but now I've discovered that spawn doesn't emit this event for startup failures (e.g. subprocess exits with a code 1). So using execa with Promise.race seems like a more reliable solution and I'm pretty happy about it

@lynxtaa lynxtaa closed this as completed Jun 14, 2021
facebook-github-bot pushed a commit to facebook/sapling that referenced this issue Nov 29, 2022
Summary:
I thought we could depend on execa to manage returning `ENOENT` for invalid command names, but apprently this doesn't always happen on windows.

By making this test work on windows, we show a more accurate "sl is not installed correctly" rather than "this is an invalid repository".

See this discussion for more information:
sindresorhus/execa#469 (comment)

Example of a user on windows not triggering ENOENT:
#282 (comment)

Instead, we should detect invalid commands on windows with exit code 1 (but only on windows).

The use of this detection is isolated to findRoot, which is where we determine if the command being used is valid. If some other issue triggered this code path, showing "this is not a valid sl command" is also a reasonable error message. Plus we still log the error for debugging purposes.

This may also help with #282.

Reviewed By: bolinfest

Differential Revision: D41595211

fbshipit-source-id: 7cff8eafd58f6da14bb85f4289eb25ddbcf5273c
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.

2 participants