rollup / rollup-watch

Fast incremental rebuilds with Rollup CLI
MIT License
91 stars 23 forks source link

<stdin> closing and parent process closing do not close `rollup --watch` #30

Closed Rich-Harris closed 7 years ago

Rich-Harris commented 7 years ago

Moved from https://github.com/rollup/rollup/issues/1117 by @OvermindDL1:

As per usual process connections when a connection is closed a process should close unless it is designed to remain open as a daemon or persistent process, however right now when rollup --watch is called, the hosting server/ide manages its processes by closing the stdin file descriptor (and in this case also dying right after), but rollup keeps on running, disconnected from anything but init itself, I have to find the (many over multiple runs of) running instances of rollup and kill it(/them) as I dev. If the current functionality is wanted, where it stays running even when its parent process and input file descriptors die, any chance of a parameter that makes it act like a normal process then so it can be properly launched/killed by IDE's and build servers?

Rich-Harris commented 7 years ago

@OvermindDL1 Have to confess I'm not entirely sure I understand the issue. Are you able to create a demo that illustrates how the behaviour differs from expectations? Thanks

OvermindDL1 commented 7 years ago

It is the same issue as here: https://github.com/brunch/brunch/issues/998

Specifically the Phoenix webserver is built on the EVM (Erlang Virtual Machine, current implementation is the BEAM/HiPE). It's external process communication was built on and follows the UNIX spec of indicating that a process should close by closing the connected STDIN of that process. Brunch had the same issue, however some of us are wanting to replace brunch/babel with a set of specific tools, rollup being, however to use rollup's watch it needs to respond to the standard UNIX way of indicating that it should close by terminating when STDIN is closed.

To be specific, Phoenix when stopped or refreshing or so will either close down entirely (thus causing the EVM to close STDIN of the connected processes) or will manually close the connected processes (which the EVM does by closing the STDIN pipe). There is no ability to send signals or anything else of the sort through an EVM Port (how it communicates with connected processes) and thus closing STDIN is the only method that it can send to signal that it is done with the connected process (which should then clean itself up and close, or keep running if it is an actual daemon or so). However when dev'ing in phoenix and starting it back up, another rollup-watch starts, until you end up with hundreds over the course of a day. :-)

thisconnect commented 7 years ago

Possibly related: I used this in a pre-electron app, so that the node process ends when the parent app is terminated.

function stopProcess(){
  // subprocess is a child_process.spawn instance
  subprocess.kill();
  process.exit();
}
// ctrl-c
process.on('SIGINT', stopProcess);

// killall node
process.on('SIGTERM', stopProcess);

// on error
process.on('uncaughtException', stopProcess);
thisconnect commented 7 years ago

process.exit([code]) takes an integer (defaulting to 0) https://nodejs.org/api/process.html#process_process_exit_code

Rich-Harris commented 7 years ago

Finally following this up, several months later 😬

I've definitely experienced the same issue, though I can't seem to reproduce it in a controlled environment. Does #55 seem like it might fix the problem?

OvermindDL1 commented 7 years ago

I've not checked the code but if you are doing this as well then it should work (perhaps behind a flag)

process.stdin.on('end' close);

An easy way to test it (untested) I'd think would be just to do:

echo | rollup-watch # Or whatever command you want

And see if rollup's watcher dies upon returning or if it is in an ephemeral state of never dying.

OvermindDL1 commented 7 years ago

For note, I've not been able to use rollup's watcher because of this so I just use my own filewatcher and just call rollup directly when files change, it would be nice to get rid of that. :-)

Rich-Harris commented 7 years ago

Thanks, have added that line. Rollup doesn't accept input from stdin (it needs an input file, because otherwise it doesn't know what imports are relative to), so I can't test it that way, but I can't see any danger to #55 so I'm inclined to release it and see what happens!

OvermindDL1 commented 7 years ago

Cool, I'll give it a try again when I get some time sometime. :-)

OvermindDL1 commented 7 years ago

Sorry for the delay, just tested, rollup's watcher never dies, I ended up with 10 copies of it running as I tested.

I also found a very easy way to test it:

$ $(sleep 2) | ./node_modules/.bin/rollup -c -w
bundling...
bundled in 40ms. Watching for changes...

And... it never died, eventually just had to hit Ctrl+c.

Here it is in brunch, working fine:

$ $(sleep 2) | ./node_modules/.bin/brunch watch --stdin

$

And it died after 2 seconds as expected due to it's stdin closing.

Here is the only code in brunch that reacts to the the --stdin cli switch: https://github.com/brunch/brunch/blob/36ba9bb4804e306b0a225d6ec75eddb231188d10/lib/watch.js#L181-L184

    if (cfg.stdin) {
      process.stdin.on('end', () => process.exit(0));
      process.stdin.resume();
    }

I've, honestly no clue what resume does (I'm primarily a C++/OCaml/Rust/Erlang programmer, not javascript), but perhaps that is the main difference (assuming the close function in rollup-watch works properly to kill the program). let me try adding it in manually to my local copy of rollup-watch, and testing:

$ $(sleep 2) | ./node_modules/.bin/rollup -c -w
bundling...
bundled in 44ms. Watching for changes...

$

And it closed properly, that appears to be the fix! :-) And yep, found the node docs for resume on a stream, it switches it to some kind of active push mode where instead of ignoring everything it instead passes it on to any registered handlers (else the handlers are ignored, like close is never called, it only needs to be called once in an entire program, earlier the better).

I'll get a PR ready, give me a few minutes...