Stdin support causing possible regression in 3.6.0

[voxpelli]

It looks like we are sometimes struck by a regression in 3.6.0, probably caused by #128, where we probably have been running concurrently in a place where it has received data on stdin, data that it has previously ignored.

That data seems to now be parsed, looking for : characters, and if one is found it's expected to be the position or name of the process to forward that input to and otherwise it's forwarded to the default process.

Previously none of that happened – no forwarding of stdin and no parsing of the stdin data. Forwarding such data as a default could therefore be considered a breaking change that requires a new major version and if released as a minor version, as now, it needs to be put behind a flag (like --forward-stdin).

Now that it has been released the most important thing is to make it work though.

I see two issues that needs fixing:

1. There's an exception that we're sometimes encountering:

TypeError: Cannot read property 'write' of null
    at Socket.process.stdin.on (<project-path>/node_modules/concurrently/src/main.js:306:26)
    at Socket.emit (events.js:180:13)
    at addChunk (_stream_readable.js:274:12)
    at readableAddChunk (_stream_readable.js:261:11)
    at Socket.Readable.push (_stream_readable.js:218:10)
    at Pipe.onread (net.js:581:20)

This could be silenced by changing if (target) { to if (target && target.stdin) { on line 306in:

https://github.com/kimmobrunfeldt/concurrently/blob/b4574e1b76a86410ad2b7cb14ae8f0c6c3a314b3/src/main.js#L304-L309

2. Forwarding stdin by default to the first process and, in the case of :-characters in the stdin, to whatever that resolves to, can break current setups and should either an opt-in or an opt-out so that one can easily fix setups that were working before, but isn't working now.

[wintercounter]

Same issue here. I have to explicit define --default-input-target to work at least for the default one.

EDIT

I logged out target and here is what I get:

ChildProcess {
  _events:
   { close: [Function: innerHandler],
     error: [Function: innerHandler] },
  _eventsCount: 2,
  _maxListeners: undefined,
  _closesNeeded: 1,
  _closesGot: 0,
  connected: false,
  signalCode: null,
  exitCode: null,
  killed: false,
  spawnfile: 'cmd.exe',
  _handle: Process { owner: [Circular], onexit: [Function], pid: 25712 },
  spawnargs:
   [ 'cmd.exe',
     '/s',
     '/c',
     '"node C:\\......\\node_modules\\jest-cli\\bin\\jest.js -o --watch --passWithNoTests --config=C:\\......\\config\\jest\\index.js"' ],
  pid: 25712,
  stdin: null,
  stdout: null,
  stderr: null,
  stdio: [ null, null, null ] }

I have a feeling that jest is causing the issue here somehow.

EDIT 2

I think my case is not related to the original topic even tho it's the same error. I think my issue is that I'm running concurrently with spawn.

spawn('node', [
	path.resolve(__dirname, 'node_modules/concurrently/src/main.js'),
	'-k',
	'-n',
	Object.keys(eco).join(','),
	...Object.values(eco).map(({command}) => `"${command.join(' ')}"`),
	'-r',
	'--default-input-target',
	Object.keys(eco)[0]
], { shell: true, stdio: 'inherit' })

See https://stackoverflow.com/questions/27786228/node-child-process-spawn-stdout-returning-as-null

I think it's the same issue. Should I open a new issue for this? Any idea for a workaround?

[gustavohenke]

Hey folks,
I'm so sorry for the oversight on detecting these kinds of problems! It should really have been a major release.

@voxpelli: you seem to have some good context of the problem, are you up for fixing this? I might not be so free any time soon.

@wintercounter: So processes spawned by concurrently are likely inheriting the I/O from the parent... so your fix could be as easy as changing the stdio option of your spawn call (note the emphasis).

[voxpelli]

@gustavohenke Sure, I can try to find some time for it. Which of the mentioned solution would you prefer? A new opt-out for stdin or changing it to an opt-in?

[wintercounter]

@gustavohenke I tried every possible combo, even tried to tweak concurrently's main.js, couldn't make it work. Stdin is either null or no output otherwise. Concurrently is spawing the process with inherit in raw mode (which is essential for most of the things I'm trying to use here) and from that point it cannot write to stdin on it's own. Docs should be updated to reflect that using -r will disable input forward and as @voxpelli stated, condition should be added to prevent errors.

Maybe with -r it would be possible to simply not have any custom code to handle commands but simply rely on node, inherint stdio and each process would get the input. Again, -r would completely disable that feature as it cannot work with it anyway.

EDIT
I have tested the above. I have webpack-serve and jest running alongside. Jest's output is fully swallowed while webpack's output appears fine.

EDIT 2
I managed to get output work but after first input everything acts like it's being completely frozen :S I can only close the terminal, CTRL+C doesn't work either. Oh well, i guess my cases are too much of an edge case to be address for a fix.

EDIT 3
Created a repo to test Jest. These are the only settings (simply inherit) that will correctly pass output. However after first input it dies. https://github.com/wintercounter/concurrently-jest-test

npm install
npm test

Please note I'm testing this on Windows 10

EDIT 4
I've tested on Mac, same issue.

I also added npm run test-concurrently-cli and npm run test-jest-cli
First one fails, last one runs fine. Seems like it's not related to spawning concurrently manually, issues with input are there even if I run concurrently from CLI.

(Sry for edits, I don't want to spam ur inbox).

EDIT 5
Investigation ended. See jestjs/jest#5017

[wintercounter]

@voxpelli Just to have something topic related here :)

if (target) {
    if (target.stdin) {
        // write
    }
}

With target && target.stdin it'll still log error when it shouldn't.

[gustavohenke]

Wow, nice investigation skills @wintercounter! Thanks for looking so deeply into this 😀

@voxpelli let's make it an opt-in, then I will cut a new major release, so that we don't get 2 breaking changes in a row...

[voxpelli]

I'll try to look at it asap after my deadline today 👍

[hyperknot]

I'm having problems with target.stdin.write(line): Cannot read property 'write' of null as well on 3.6.1. What was the latest stable version without this problem?

[gustavohenke]

v3.5.0 had no support for stdin, if that's fine for you @hyperknot 🙂

[hyperknot]

Thanks, 3.5.x works perfectly.

[gustavohenke]

Hiya folks! v4.0.0 is out with a breaking change relating to this! 🚢
You know have to specify --handle-input in order to opt into the input handling feature.