Stop reading from stdin after programmatic API finishes

[brandonchinn178]

Fixes #252 by explicitly calling .pause() on the input stream after all commands have finished. This seems to be the only way to fix the problem, and there doesn't seem to be a way to reset the process.stdin state completely to the state before concurrently ran (see NodeJS docs).

Refactored some parts of the code in order to make this change more drop-in:

• Added BaseHandler class that all handlers extend from
• Add onFinish to BaseHandler which runs after all commands have finished
• Add FakeHandler to tests that extend BaseHandler instead of ad-hoc handler objects in concurrently.spec.js
• Use an actual Readable in input-handler tests instead of EventEmitter

The CLI has not changed in this PR, but the programmatic API now has two new options:

• handleInput: an alias for the old inputStream: process.stdin. Just thought this would be nice, since 99% of the time, inputStream is set to process.stdin
• pauseInputStreamOnFinish: Some people might have a use-case where they want to read from process.stdin further after running concurrently programmatically. In this rare instance, they'd have to explicitly set this option to false, or explicitly call process.stdin.resume() afterwards. I can't imagine this being a common use-case, so I think this explicitness is a good trade-off.

Repro steps

1. Write start.js as specified in Programmatic usage errors on Ctrl+C when specifying process.stdin #252, except replace require('concurrently') with require('.'). Put this file in the root of the repo.
2. node start.js + Ctrl-C should no longer hang / wait for Ctrl-D

[brandonchinn178]

@kimmobrunfeldt It seems like Node 8 reached EOL on Jan 1. Can we remove Node 8 from the CI pipeline? Apparently Promise.finally() was added in Node 10

[gustavohenke]

Hey @brandonchinn178 👋
I've maintaining the package more than Kimmo actually -- I've bumped the minimum version to 10 on master.
Can you rebase and try again?

[gustavohenke]

Hey! I'm sorry that I took this long to look at your PR.
Your analysis is very good, and it may be unveiling the reason for some other issues reported in that the processes hang forever!

[gustavohenke]

Rather than pausing, WDYT if this called .unref() instead?
We can then get rid of one the pauseInputStreamOnFinish option.

[brandonchinn178]

unref is only a function on net.Socket, but not readable Stream, so it doesn't work if stdin is a file.

$ concurrently ... < foo
(node:51227) UnhandledPromiseRejectionWarning: TypeError: process.stdin.unref is not a function
...

We could update the docs to say "inputStream is either net.Socket or readable Stream" and then have this clean up check if the unref function exists and calls that if it does, but I think that adds too much complexity. Thoughts?

[gustavohenke]

Ah yes, of course it couldn't be this simple 🤦 OK. Need to think a bit more.

[gustavohenke]

Hmm, I think we could be smart and pause the input stream if it has no more listeners and it either

• was paused before; or
• was not flowing before.

Although, since I haven't been able to give much attention to concurrently, I'd say just introducing the option is probably best.

WDYT?

[brandonchinn178]

It's been so long since I worked on this PR, honestly, I can't provide any more insights. (it seems like you are similarly distracted as well) If you're suggesting merging the PR as-is, that would be preferable by me. Are there any more blocking concerns you'd like me to address first?

[gustavohenke]

Just the handler interface, as per #253 (comment)

[gustavohenke]

WDYT if handle returned an optional cleanup function, along with the mapped commands?
Having an onFinish method kinda implies that the instance is single use.

[brandonchinn178]

Personally, I like this better:

• It keeps handle as a pure transformation Commands => Commands
• It makes the reduce logic a bit more complicated in the main function
• It might also be a breaking change, if any third-party APIs write their own handlers
  • The only way to not make it a breaking change would be to allow handle to return either Commands or { commands, cleanup }, which is additional complexity for the caller to handle

But it's your call

[gustavohenke]

For this one I think it's better to introduce a breaking change.
As you correctly stated, third parties can pass their own handlers - and it could become a pain point to manage state across multiple runs of the same instance.

Some other uses I can see for this cleanup function:

• Removing the RxJS subscription (which also removes the event from the inputStream, though doesn't make the process exit if it's stdin)
• Removing listeners on signals in the KillOnSignal handler (which can also prevent the process from exiting)

[brandonchinn178]

Yeah, that makes sense. I can update the handle api if you want. Are there any docs that I'd need to update as well?

[gustavohenke]

Nope, no docs on creating custom handlers to this date 😞

[brandonchinn178]

@gustavohenke any progress on this?

[brandonchinn178]

@gustavohenke How's that?

[gustavohenke]

Looks very good to me! Thanks for doing this 🏆