[go: up one dir, main page]

Skip to content
This repository has been archived by the owner on Jan 6, 2021. It is now read-only.

On Linux cross-env returns exit code 0 when then child command is killed #150

Closed
esistgut opened this issue Oct 24, 2017 · 11 comments · Fixed by severest/retrobot#121 or mozilla-it/games.mozilla.org#1 · May be fixed by scatools-demo/desktop#19, scatools-demo/desktop#20 or scatools-demo/desktop#23

Comments

@esistgut
Copy link
  • cross-env version: 5.1.0
  • node version: 8.7.0
  • npm version: 5.4.2

What you did:
$ ./node_modules/.bin/cross-env TEST=test sleep 10 from first shell
$ killall sleep from second shell
$ echo $? 0 from the first shell

What happened:
cross-env returns with exit code 0.

Problem description:
cross-env should return with exit code 143.
This is problematic if cross-env runs inside npm scripts and the command exits for unexpected reasons (our case is a OOM kill) because the execution continues without reporting errors.

@kentcdodds
Copy link
Owner

Thanks for the report @esistgut! I'm happy to accept a PR that's tested 👍

@colthreepv
Copy link
colthreepv commented Oct 26, 2017

I have checked briefly the codebase.

The node.js child_process exit event does not give any exitcode when the subprocess dies. It gets invoked with code => null , signal => SIGTERM

Reading the node.js documentation on 'exit' it's clearly stated that:

The 'exit' event is emitted after the child process ends. If the process exited, code is the final exit code of the process, otherwise null. If the process terminated due to receipt of a signal, signal is the string name of the signal, otherwise null. One of the two will always be non-null.

A quick fix would be returning process.exit(1) when the exitcode is null, I suppose 143 might be an internal exitcode specific to linux, as it is so high numbered

@kentcdodds Regarding tests, how can it be done to make a test specific for linux platform? I am not sure yet this little fixup (but critical IMHO) can be testable with the actual runner

PS: full disclosure, i work with @esistgut

@kentcdodds
Copy link
Owner

Could you verify whether the exit code is platform specific?

@colthreepv
Copy link
colthreepv commented Oct 26, 2017

yes the exit code on child_process acts differently based on platforms

I tried finding how they handle process spawning (and errors) inside npm but It was inconclusive, for now

@kentcdodds
Copy link
Owner

Thanks for looking into that.

I don't think that I want to proceed before we see some prior art. What are other tools that spawn processes doing in situations like this?

I'm afraid that I don't have much time to dedicate to this problem, but I will look at and merge a pull request that's tested and has reference to somewhere else that's acceptably implementing things in the same way.

Thanks again for reporting the issue and for your investigation!

@colthreepv
Copy link

The biggest difference I noticed between cross-env and npm-script is that, the latter spawns an entire shell.

root@b663689708a8:/# ps aux
USER        PID %CPU %MEM    VSZ   RSS TTY      STAT START   TIME COMMAND
root          1  0.0  0.1  20252  3196 pts/0    Ss+  18:16   0:00 bash
root         84  0.0  0.1  20244  3224 pts/1    Ss   18:17   0:00 bash
root        121  0.0  0.1  20244  3216 pts/2    Ss   18:20   0:00 bash
root        166  6.3  1.9 1048176 38940 pts/2   Sl+  18:29   0:00 npm
root        176  0.0  0.0   4336   800 pts/2    S+   18:29   0:00 sh -c sleep 20
root        177  0.0  0.0   4236   648 pts/2    S+   18:29   0:00 sleep 20
root        178  0.0  0.1  17500  2072 pts/1    R+   18:29   0:00 ps aux

Killing the process (PID 177) would forward correctly the err codes and signals to the "father" shell, but if the "spawned shell" gets killed (PID 176) I get the exact same behaviour as cross-env: the exitcode gets lost, and the process interrupts abruptly.

Not suggesting a change here, I am merely adding details to the issue.
I am not sure how it is handled in the npm codebase, I tried diving again in their code but it's quite intricate

@kentcdodds
Copy link
Owner

Thanks for the investigation and details.

@bithavoc
Copy link
Contributor
bithavoc commented May 9, 2018

This is a huge issue, I'm currently using electron-react-boilerplate to build an electron app, the boilerplate depends on in this package to set environment variables.

The build command executes the following steps:

"build-renderer": "cross-env NODE_ENV=production node --trace-warnings -r babel-register ./node_modules/webpack/bin/webpack --config webpack.config.renderer.prod.js --colors",

I read in the console that the build process seem to have succeeded:

[0] npm run build-main exited with code 0
[1] npm run build-renderer exited with code 0

But it's not really true, the build-rendered failed but cross-env fails to report the failed exit status which makes the build process continue creating an installer with missing files so our desktop app doesn't load... it fails silently!!!. This is how the app looks like when cross-env fails to report the exit status and a build is included with missing files:

screen shot 2018-05-09 at 8 12 04 am

This is a huge issue.

How can we fix this @kentcdodds ?

@kentcdodds
Copy link
Owner

I don't know how to fix this off the top of my head, but you're welcome to investigate further and submit a pull request.

@bithavoc
Copy link
Contributor
bithavoc commented May 9, 2018

@kentcdodds found a solution, I'm testing it. I will send a PR soon.

bithavoc added a commit to bithavoc/cross-env that referenced this issue May 9, 2018
bithavoc added a commit to bithavoc/cross-env that referenced this issue May 9, 2018
bithavoc added a commit to bithavoc/cross-env that referenced this issue May 9, 2018
bithavoc added a commit to bithavoc/cross-env that referenced this issue May 9, 2018
@bithavoc
Copy link
Contributor
bithavoc commented May 9, 2018

@kentcdodds it's impossible to setup this package from GitHub due all scripting involved(specially with yarn), it doesn't work from a local folder either so please make sure to test it and publish it ASAP so I can fix it for my users.

If you want to see what's the issue with the binary when you try to use this module from local or github, try to use the binary from a project like this:

    "cross-env": "./my-local-fork-of-cross-env",

This doesn't work either:

    "cross-env": "https://github.com/kentcdodds/cross-env.git#<SOME COMMIT>",

it either doesn't find cross-env executable or cross-env has a permission issue and doesn't execute.

mrfelton added a commit to mrfelton/zap-desktop that referenced this issue Jun 15, 2018
The v5.2.0 includes (amongst other things) a fix for incorrectly reported
exit codes of child processes, which can prevent build build failures from
correctly halting a build, resulting in incomplete or invalid builds.

See kentcdodds/cross-env#150 (comment)
mrfelton added a commit to mrfelton/zap-desktop that referenced this issue Jun 15, 2018
The v5.2.0 includes (amongst other things) a fix for incorrectly reported
exit codes of child processes, which can prevent build build failures from
correctly halting a build, resulting in incomplete or invalid builds.

See kentcdodds/cross-env#150 (comment)
mrfelton added a commit to mrfelton/zap-desktop that referenced this issue Jun 15, 2018
The v5.2.0 includes (amongst other things) a fix for incorrectly reported
exit codes of child processes, which can prevent build build failures from
correctly halting a build, resulting in incomplete or invalid builds.

See kentcdodds/cross-env#150 (comment)
mrfelton added a commit to mrfelton/zap-desktop that referenced this issue Jun 15, 2018
The v5.2.0 includes (amongst other things) a fix for incorrectly reported
exit codes of child processes, which can prevent build build failures from
correctly halting a build, resulting in incomplete or invalid builds.

See kentcdodds/cross-env#150 (comment)
mrfelton added a commit to mrfelton/zap-desktop that referenced this issue Jun 15, 2018
The v5.2.0 includes (amongst other things) a fix for incorrectly reported
exit codes of child processes, which can prevent build build failures from
correctly halting a build, resulting in incomplete or invalid builds.

See kentcdodds/cross-env#150 (comment)
mrfelton added a commit to mrfelton/zap-desktop that referenced this issue Jun 15, 2018
The v5.2.0 includes (amongst other things) a fix for incorrectly reported
exit codes of child processes, which can prevent build build failures from
correctly halting a build, resulting in incomplete or invalid builds.

See kentcdodds/cross-env#150 (comment)
mrfelton added a commit to mrfelton/zap-desktop that referenced this issue Jun 15, 2018
The v5.2.0 includes (amongst other things) a fix for incorrectly reported
exit codes of child processes, which can prevent build build failures from
correctly halting a build, resulting in incomplete or invalid builds.

See kentcdodds/cross-env#150 (comment)
mrfelton added a commit to mrfelton/zap-desktop that referenced this issue Jun 15, 2018
The v5.2.0 includes (amongst other things) a fix for incorrectly reported
exit codes of child processes, which can prevent build build failures from
correctly halting a build, resulting in incomplete or invalid builds.

See kentcdodds/cross-env#150 (comment)
This was referenced Sep 13, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants