use close not exit process event

[akosyakov]

exit event does not mean that all output was delivered, close means it: https://nodejs.org/api/child_process.html#child_process_event_close

The 'close' event is emitted when the stdio streams of a child process have been closed. This is distinct from the 'exit' event, since multiple processes might share the same stdio streams.

vscode-jsonrpc uses close (https://github.com/microsoft/vscode-languageserver-node/blob/dd78cba181136e057cfcc4bb7bc17c7c78877648/jsonrpc/src/messageWriter.ts#L84) so it does not cause issues for filesystem watching, but our raw processes can miss output sometimes

[akosyakov]

I've reviewed all places. In most of cases listening to exit is harmless, since we don't do anything with stdout in exit event handler.

There are 2 places which look incorrect:

• sometimes we can exit too early, before all stdout content is processed:

  theia/dev-packages/electron/scripts/post-install.js

  Line 35 in 2bfbe4a

  subprocess.once('exit', (code, signal) => {

• and raw-process, here it can break if a client expects all content from stdout to be delivered in onExit callback. In practice, it is not a case, most of clients using process.stdout.on('close', ...) already. The only client which looks suspicious is ProcesTask, it can continue emitting new lines after emitting on exit event. We should clarify what is the contract, i.e. whether stdout can deliver after exit event, can clients rely onExit event for it or should they use stdout.on('close' instead.

cc @marcdumais-work @marechal-p @elaihau

[paul-marechal]

From my understanding, it was implied that stdio buffers wouldn't necessarily be empty when the process exits. Just like "raw" child processes using Node's API. Did you expect it to be different?

[paul-marechal]

https://nodejs.org/docs/latest-v10.x/api/child_process.html#child_process_event_close looking at the documentation, it depends what people are looking after. I think it would make more sense to fire both exit and close events so that people can choose from, depending on their use case?

[paul-marechal]

Also, the code you linked in VS Code seems to be related to streams. So far, I also thought we were supposed to hook onto a process stream close event ourselves.

[akosyakov]

What about terminal process implementation? node-pty apparently has only exit event, it will trigger both close and exit process events?

I'm fine with adding a new event like in Node.js apis or using existing process.stdout.on('close', ...). More important to me that:

• docs for onExit says that stdout still can produce data after this event
• figure out whether ProcessTask should be changed to fire task exit event on process.stdout.on('close', ...) instead of onExit like right now
• fix

  theia/dev-packages/electron/scripts/post-install.js

  Line 35 in 2bfbe4a

  subprocess.once('exit', (code, signal) => {

  to use close instead of exit.