Dropping the cleanup callback, and whether to handle SIGINT

[joliss]

Currently, the main use case for the cleanup callback is deleting temporary directories (example). With the upcoming new API, tmp dir handling will be centralized, so this will no longer be necessary.

I'm wondering if we can drop cleanup from the API. We can still resurrect it later if necessary, thanks to the versioned (feature-flagged) API scheme.

Thinking about use cases for cleanup, the following come to mind:

• Persistent caches: @timmfin suggested that for persistent caches[1], you might want to dump data into the persistent cache on exit. But perhaps it's OK (performance-wise) to update the cache after each rebuild - in that case we wouldn't need a cleanup callback.
• Subprocesses: If a plugin needs to talk to a subprocess that keeps running across rebuilds, my understanding is that it's not necessary to manually kill the subprocess at cleanup time, because it will be killed automatically by default. See child_process.spawn#detached and setpgid(2); my testing confirms that this is the case on Linux.

So I think it might be OK to drop cleanup. Are there other use cases for cleanup that I'm missing?

[1] We don't have first-class support for persistent caches at the moment, but we might add this in the future.

---

The motivation for dropping cleanup is that I'd like to avoid having a piece of API that is essentially unused.

Another reason why I'd like to get rid of cleanup is that handling SIGINT (Ctrl+C) is tricky. This is perhaps only a semi-related issue, but I still wanted to bring it up:

• The SIGINT handler causes a delay: If you hit Ctrl+C in the middle of a rebuild, the handler will only run after the current synchronous thread of execution has finished, so that there is a delay between hitting Ctrl+C and Node actually exiting. This can take several seconds; see e.g. https://github.com/ember-cli/ember-cli/issues/3324. This problem can be alleviated but not eliminated by doing setImmediate in various places.

  If we could eliminate the handler entirely, Node would terminate immediately on Ctrl+C. Of course, Broccoli would leave all its temporary files in place, so this would require that the next Broccoli instance detects that there are old tmp directories (e.g. /tmp/broccoli.12345, where there is no running process with PID 12345), and clean them up. I'm not sure if this is sensible, but it would be cool if we could make it work.

• It's hard (though maybe not impossible) to get into a consistent state: If we catch a SIGINT during a rebuild, we might be in the middle of an asynchronous operation. To do cleanup reliably, we'd have to let the currently-executing plugin finish its rebuild, and then call cleanup on all plugins. The current implementation is buggy and leaves behind tmp directories (#187).

[timmfin]

I'm glad to move my persistent cache work from cleanup to something per rebuild. But it really should be done after the rebuild had finished and not inline/synchronous with the current rebuild.

I'm already getting to the point where my builds are getting "slow", even in the ideal, nothing-has-changed case. This is because—in projects with thousands of files—the cost of merely walking over, stat-ing, and re-symlink-ing all of the files in an input tree is becoming significant. For one plugin/filter, that cost isn't too bad (maybe ~100ms IIRC) but when you have several filters composed together, things add up. So adding more per-rebuild/per-filter cost (the persistent data dumping) worries me.

And I assume that a new "post every rebuild" hook would have the same issues as cleanup does? (And you'd similarly be less interested to add that hook? :frowning:)

ps: FYI, to help alleviate these incremental build issues with large projects, I've tried implementing other various optimizations:

• Making broccoli-filter symlink directories instead of individual files where possible E.g. symlink directories that only contain files not to be processed by the plugin (note that code is tied up with my persistent fork and not cleanly separated (yet?).
• Making broccoli-filter symlink files that it processed but did not change. That prevents the cost of re-writing the content again (especially useful for filters that may or may not change the files content).
• A TieredCachingWriter that combines broccoli-caching-writer and broccoli-filter, so that both the whole output and individual files are cached (preventing the need to check and process every file if the whole dir hasn't changed).
• For for string input trees that point directly to an external directory, I've considered only relying on the watcher to detect file changes and never explicitly stat-ing those files/directories. Likely would require pretty major surgery to implement (though I've toyed with it deep in our custom setup).

Update: I haven't been using Broccoli for over a year, so please don't worry as much about my previous work/efforts.