error.code is not always available

[veselov]

Node version (or tell us if you're using electron or some other framework):

4.x

ShellJS version (the most recent version/Github branch you see the bug on):

0.7.4

Operating system:

Linux FC23

Description of the bug:

Experienced this when running some scripts under mocha:

Example ShellJS command to reproduce the error:

Unknown

Error:

/tmp/shelljs_67e6b2ac42f07ecaa8c5:4

fs.writeFileSync("/tmp/shelljs_2d310a64fef8dfba8311", err ? err.code.toString() : '0');
^

TypeError: Cannot read property 'toString' of undefined
at /tmp/shelljs_67e6b2ac42f07ecaa8c5:4:71
at ChildProcess.exithandler (child_process.js:220:5)
at emitTwo (events.js:87:13)
at ChildProcess.emit (events.js:172:7)
at maybeClose (internal/child_process.js:827:16)
at Process.ChildProcess._handle.onexit (internal/child_process.js:211:5)
exec: internal error
/usr/lib/node_modules/jenkins-mocha/lib/jenkins.js:86
shell.exit(shell.exec(command.join(' ')).code);
^

TypeError: Cannot read property 'code' of null
at module.exports (/usr/lib/node_modules/jenkins-mocha/lib/jenkins.js:86:45)
at Object. (/usr/lib/node_modules/jenkins-mocha/bin/jenkins.js:2:26)
at Module._compile (module.js:409:26)
at Object.Module._extensions..js (module.js:416:10)
at Module.load (module.js:343:32)
at Function.Module._load (module.js:300:12)
at Function.Module.runMain (module.js:441:10)
at startup (node.js:139:18)
at node.js:968:3

The crafted shell file (/tmp/shelljs_67e6b2ac42f07ecaa8c5) is not available.

[veselov]

This may be nodejs problem. The callback argument is:

{"cmd":"/bin/sh -c '/home/vps/ws/Trunomi/trulink-messaging-service/node_modules/.bin/istanbul' cover --dir /home/vps/ws/Trunomi/trulink-messaging-service/artifacts/coverage -- '/home/vps/ws/Trunomi/trulink-messaging-service/node_modules/jenkins-mocha/node_modules/.bin/_mocha' --reporter '/home/vps/ws/Trunomi/trulink-messaging-service/node_modules/jenkins-mocha/node_modules/spec-xunit-file/index.js' --colors 'unit_test.js'"}

And it is an instance of Error. The doc language may suggest the error code doesn't have to be set:

The error.code property will be the exit code of the child process while error.signal will be set to the signal that terminated the process. Any exit code other than 0 is considered to be an error.

But neither code nor signal properties are there.

The parent process seems to be terminating with SIGTERM, and it's shelljs that is killing it.

Same happens on NodeJS v6 (6.9.1)

[nfischer]

@veselov you're saying that since .signal is not there, .code should likewise not be present? Hmmm... sounds strange to me. I would expect .code to always be reported.

Would it be sufficient to use 1 instead of .code when it isn't present?

The parent process seems to be terminating with SIGTERM, and it's shelljs that is killing it.

Yup, shelljs currently kills the process. Please see #483. If you'd like to help, I would love to review a pull request.

Could you take a look at #524? It's going to be an alternative to exec(), and I want to make sure it won't have this problem.

[nfischer]

Opening this up for community contributions. Before this can be closed:

• Protect against an undefined err.code (see the line mentioned in the report). We can default the code to 1
• Make sure feat: new alternative to exec #524 has a similar fix (if it's still vulnerable, just comment on that PR and I'll fix it)

If we fix just the first bullet point, we can cut a patch release in v0.7. No need to get rid of process.exit() calls yet, those can be removed when we fix #483.

[veselov]

@veselov you're saying that since .signal is not there, .code should likewise not be present? Hmmm... sounds strange to me. I would expect .code to always be reported.

I'm saying that it seems to be NodeJS issue because neither .code nor .signal is present, and NodeJS suggests at least one should be.

Would it be sufficient to use 1 instead of .code when it isn't present?

I would diagnose this first. If there is a reason to believe that this only happens when shelljs kills the child (I haven't seen it happen in other cases), then I'd use 127.

Yup, shelljs currently kills the process. Please see #483. If you'd like to help, I would love to review a pull request.

Are you saying something happens in shelljs that makes it die, and kill the children? And that's what I'm seeing? I'm not seeing any output from shelljs that indicates that it's having any kind of a problem.

[nfischer]

Oh, I misunderstood. I thought you meant that shelljs was killing the current node process, not the child process spawned by shell.exec().

If I misunderstood you, then that issue is unrelated to this.