Use tree-kill on exit to kill process trees spawned by children

[efokschaner]

Fixes #10
Supersedes #11

[efokschaner]

Retest this please.

[efokschaner]

Looks like "retest this please" is specific to Jenkins, doesn't work with Travis. The error in Travis makes me think a retry is warranted but I cannot trigger one. Any thoughts @arjunmehta ?

[arjunmehta]

Have you tested simply calling treeKill() without waiting for the callback to complete?
For example, would it be as simple as doing:

var treeKill = require('tree-kill');

...

if (exited === false) {
  // is the command sent and are processes killed without waiting for a callback?
  treeKill(spawnedProcess.pid);
}

[efokschaner]

Yes, you can see in the first of my three commits I tried this here f8be961
The node process stops as soon it returns to the event loop for the first time so the treeKill() doesn't get to do any of its work (I believe tree-kill begins with some async information gathering before it even gets to killing anything).

[arjunmehta]

Ah gotcha. The other thing to consider is that the tree-kill module has a "binary" cli tool included with the module (found in node_modules/.bin). Of course it's not an actual binary, but a script, but acts as an executable within any node script.

So you should be able to simply do:

childProcess.spawnSync('tree-kill', [spawnedProcess.pid])

I'd like to remove the complexity of needing to resolve the module path and execute a new node instance with --eval, or find a more elegant way of doing so. Hopefully the binary executable will allow this. Also, I'm not sure we'd need to handle output from this process (ie. no need to inherit stdio).

Please let me know your thoughts

[efokschaner]

Regarding the tree-kill binary, I considered that approach, however I don't believe that the node_modules system guarantees that bins of a sub-dependency would be placed in a specific location.
I did a test and while yarn and my relatively-up-to-date npm version seems to like to hoist everything up to the root node_modules, npm still exhibits the classical non-hoisting behaviour when you install modules globally. See this screenshot:

This means we'd need to "resolve" the path to the 'tree-kill' binary similar or identical to how we resolve its js for the purpose of launching node.

For this reason, I think invoking the js entry point directly, with the node executable that is already running, resolved using the module system's resolution mechanism, is the most robust approach with the least assumptions / complications.

I chose to inherit stdio so any errors emitted by tree-kill aren't completely silent. I'm not sure multiview should itself fail when the tree-kill has an issue, but at least they get reported.

LMK if you disagree with any of this.

[arjunmehta]

Ahh. Seems messy and unpredictable indeed. I'm just feeling like the solution to spawn command to run a module is not very clean and seems counter intuitive.

Looking into this more, I'm wondering if the beforeExit event is actually more suitable for this. It can handle asynchronous code:

process.on('beforeExit', function(code) {
  if (exited === false) {
    treeKill(spawnedProcess.pid, function(err) {
      if (err) {
        console.error(err);
      } 
    });
  }
});

[efokschaner]

I think splitting it out in to our own wrapper is reasonable and makes adding more code to the subprocess easier for sure, but it doesn't avoid module resolution challenges entirely.

When you run spawnSync(process.execPath, ['./standalone-treekill.js', ..., the cwd of our process (and the one inherited by the child) wont be the folder that contains standalone-treekill.js so you couldn't use the path './standalone-treekill.js'. However we can maybe use require.resolve() to get the path OR if you're wanting to avoid require.resolve() we can maybe use __dirname or __filename (and import.meta.url on newer node versions) to get the path to that script, then we can either set the child CWD or pass the full script path in the first param.

Thoughts?

[arjunmehta]

If tree-kill is a dependency of our project, multiview/node_modules/tree-kill exists.

We will need to use __dirname and path.resolve but node will always go by the order specified here to load the right module. It will load the module require'd from the main multiview node_modules location.

// assuming standalone-treekill.js in same directory as Spawn.js
var treeKillScriptPath = path.resolve(__dirname, './standalone-treekill.js');
spawnSync(process.execPath, [treeKillScriptPath, spawnedProcess.pid], { stdio: 'inherit' });

No need to import.meta.url. Executing the script with the right path should load the right dependency always.

[arjunmehta]

I realized now we're saying the same thing! Yes, I assumed we'd need to resolve the dirname and execute that. 👍

[efokschaner]

So if multiview is being used as a library and it's my-app/node_modules/multiview/node_modules/tree-kill, and the cwd is currently my-app. You're saying let's always run with the cwd multiview and then let node get tree-kill out of its node_modules? That sounds like it works, and would still work with hoisting.

[arjunmehta]

It’ll find the nearest one up the parent tree and should find it because it’ll be a dependency of multiview. In a way, it should resolve the same as if we were to require it in Spawn.js.

[arjunmehta]

@efokschaner Please rebase from master. It seems the version of nodeunit had an npm dependency error for node 5+.

I've updated it and also testing for newer versions of node.

[efokschaner]

Thanks for fixing the CI

[arjunmehta]

please use camelCase

[arjunmehta]

@efokschaner Please again rebase with master. I added eslint to the (admittedly meagre) test script to help adhere with code standards.

[efokschaner]

Will do!

[efokschaner]

I've incorporated all the discussion and tested on windows and macos.

[arjunmehta]

@efokschaner perfect! Thank you for this. I will publish a new version this coming weekend.

[efokschaner]

@arjunmehta just a friendly reminder to cut a release at some point :)

[arjunmehta]

@efokschaner done! Sorry for the delay!

https://github.com/arjunmehta/multiview/releases/tag/3.0.0
https://www.npmjs.com/package/multiview