hpal
hpal copied to clipboard
Prevent bin process from crashing on sigint when sent during npm init dialog
Addresses #42
Now, on starting the npm init dialog, we ignore SIGINT on our main process, so when you press ctrl + C (or however else you fire off a SIGINT signal), as that dialog prompts you to do to quit at anytime, instead of the main process exiting, the SIGINT reaches the npm init child process spawned by the new command, which then exits.
Coverage remained the same at 100.0% when pulling f0126c5101fd66dfdebba97d63d99db78d4590cb on handle-sigint-npm-init into a1a642d1a91d3c1b98fa7bef88b3d6a623f44260 on master.
Coverage remained the same at 100.0% when pulling 319584eca4d4495d602efd1eec1318e63a8655de on handle-sigint-npm-init into a1a642d1a91d3c1b98fa7bef88b3d6a623f44260 on master.
@devinivy heyo! ran into some funkiness here due to signal-handling (or lack thereof) on Windows. To resolve, I'm thinking
- Skip the test that exercises the newly added
SIGINThandling on Windows only - Find someone (work, hapi-hour) who has a Windows machine, have them test running the
hpal newdirectly on this branch. I think my changes will actually work when run from a shell i.e. the test failing on Windows isn't actually a problem, but want to confirm
Assuming my changes are even on the right track, how does that plan sound to you?
For background / explanation of the above, I think / hope the following are true:
-
Assuming this conversation is still current, Windows doesn't understand how to handle
SIGINT-ing a child process, which I'd been doing to run this test in a previous commit. Windows handles this by exiting the child process immediately; any process event handlers within the subprocess are skipped and you land in theclosehandler set on theclisubprocess inrun-util.bin- as an aside, how I've setup that test to run — killing
bin's process group via negative pid — now won't work on Windows for a different reason, that Windows doesn't have an equivalent of Linux's process groups (or at least implements them incompatibly)
- as an aside, how I've setup that test to run — killing
-
The changes to the
newcommand might be fine on Windows, despite the test being hosed. It sounds like node, under the hood, maps pressing ctrl+c toSIGINTwhen received in a console environment -
Lastly, maybe unrelatedly, the Travis branch build on my latest commit failed due to an odd
EBUSYerror on a different test for thenewcommand: https://travis-ci.com/github/hapipal/hpal/jobs/502978134 I'm assuming this is unrelated / a phantom thing, since it didn't affect the PR build off the same commit, but I'll dig into it a bit more, see if Travis' docs speak to the issue at all- EDIT Just reran the branch build and it passed
@zemccartney apologies for neglecting this! I like that plan. But also— I'm down to land this as-is, since it seems like an improvement to me. Do you have any thoughts/guidance on it?
@devinivy ah! jeez, yea, I completely spaced here! But I believe someone at my office has a fairly current Windows machine now, I should be able to test this out there, report back. I could shoot to do that this week, which hopefully means just landing this as-is with the assurance that ctrl-c'ing out of the new dialog works as expected on Windows (though could drop in a TODO / peel off a separate issue if any further remediation's needed). Does that work ok?