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

Add exec to node commands in the binary #3965

Merged
merged 1 commit into from
Jul 19, 2017
Merged

Add exec to node commands in the binary #3965

merged 1 commit into from
Jul 19, 2017

Conversation

romandrahan
Copy link
Contributor

@romandrahan romandrahan commented Jul 19, 2017

Summary

After merging #1180 yarn binary executes node commands it these ways:

node "$basedir/yarn.js" "$@"
# and
nodejs "$basedir/yarn.js" "$@"

This leads to the fact, that yarn uses /bin/sh for running:

# ps aufx
USER       PID %CPU %MEM    VSZ   RSS TTY      STAT START   TIME COMMAND
root         8  0.0  0.0   4340   756 ?        S    08:08   0:00 /bin/sh /usr/local/bin/yarn test:unit
root        19  4.0  3.3 1083008 69008 ?       Sl   08:08   0:00  \_ node /opt/yarn/bin/yarn.js test:unit
root        29 28.6  7.4 1284500 152928 ?      Sl   08:08   0:06      \_ node /app/node_modules/.bin/karma start test/unit/karma.conf.js --single-run

It causes problems when Yarn is running in Docker container - it doesn't handle SIGTERM or any other signals.

To resolve this issue we can rewrite node commands to these:

exec node "$basedir/yarn.js" "$@"
# and
exec nodejs "$basedir/yarn.js" "$@"

With that, Yarn will be started directly without /bin/sh:

$ ps aufx
USER       PID %CPU %MEM    VSZ   RSS TTY      STAT START   TIME COMMAND
root         1  2.3  2.8 1070088 58524 ?       Ssl  08:56   0:00 node /usr/local/bin/yarn test:unit
root        14  0.0  0.0   4340   748 ?        S    08:56   0:00 sh -c karma start test/unit/karma.conf.js --single-run 
root        15 22.9  7.6 1286672 155688 ?      Sl   08:56   0:06  \_ node /app/node_modules/.bin/karma start test/unit/karma.conf.js --single-run

And SIGTERM became working.

Test plan

$ docker run node yarn test:unit
...
$ ^C
$ make: *** [test.unit] Error 130

Copy link
Member

@arcanis arcanis left a comment

Choose a reason for hiding this comment

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

That seems better indeed, I just hope it won't have any unforeseen effect. Thanks!

@arcanis arcanis merged commit acbb802 into yarnpkg:master Jul 19, 2017
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