Exec node in exec mode can’t kill process when standard shell is dash

[johanneskropf]

On some linux distributions /bin/sh is symlinked to /bin/dash. In this case when a command is run from node-red with the exec node in exec mode the command will be spawned in a dash shell. When a kill command is send to the exec node or it reaches its configured timeout it will only kill the dash shell and the process itself will be orphaned. It will keep running and still return its result at whenever time it finishes rendering the kill and timeout options ineffective in that circumstance.
The responsibility of the dash shell or its incompatibility with the way that node.js child.process sends the signal can be shown by changing the shell that is spawned.
There is an option to specify which shell to use in the child.process exec command. If this is set to /bin/bash everything works as would be expected and on timeout or kill the shell and the spawned process get terminated.
I’m not sure how this could be fixed as this seems to be happening due to inconsistencies in how the different shells behave and how libuv the underlying library used by node handles signals. Some issues that shed more light on this are:
nodejs/node#4432
nodejs/node#2098
libuv/libuv#836

Edit: - See discussion forum for where this came from - https://discourse.nodered.org/t/exec-node-timeout-not-working-in-exec-mode/28040

[johanneskropf]

would it be plausible to do something like the following:

var fs = require('fs'); //add require fs in the exec node

than in the exec part of the code add those two line:

var execOpt = {encoding:'binary', maxBuffer:10000000}; //build the exec options as a separate variable
if (fs.existsSync('/bin/bash') && process.platform === 'linux') { execOpt.shell = '/bin/bash'; } //add the shell argument for bash if it exists and it’s linux

and change the exec call to:

child = exec(cl, execOpt, function (error, stdout, stderr) { //using the execOpt object

[dceejay]

Yes - looks plausible - I would reverse the order of the test so we only do the existsSync if we are on Linux to help performance - but yes, def worth testing.
I also think (as you suggested on discourse) some words in the docs may be useful.

[johanneskropf]

Ill have a go at a pull request than 😄