Forward stop signals to tailwindcss watcher

[jordan-brough]

I was getting orphaned tailwind processes in dev when foreman got a sigterm. Foreman stops the Rails/rake process, but the tailwindcss watcher it spawned could keep running as an orphan.

This updates to have tailwindcss:watch spawn the CLI directly, trap INT and TERM, forward them to the child, and wait for the child to exit.

It also restores the previous signal handlers afterward so the task doesn't leave process-global traps changed.

[flavorjones]

Thanks for this! It's something I've heard complaints about from time to time.

I'd like to be careful before merging anything that resembles process management, mostly because it's complicated to do correctly, and it feels like a concern that shouldn't be owned by this gem. For context, see David's comment about a previous attempt at this: #347 (comment)

One idea raised in that pull request thread was the possibility of moving it into the Rails server itself (sprockets-style). I'm not very familiar with what's involved, but I think the path of making it a Rails feature might be more promising as a long-term solution.

I also know there's been a conversation going on in Foreman issues and pull requests for years (e.g., ddollar/foreman#780) discussing whether Foreman should send signals to process groups. So arguably you could also squint and say "this is a Foreman concern."

(Historical note: This all might be easier if the rake task was simply a light wrapper around an exec call to tailwindcss, however we found out the hard way the problem with that approach in #189.)

In any case, I'm not totally opposed to improving the "watch" process management, but I do want to be circumspect and make sure there's absolutely no other, better place to put this responsibility, first. Especially read David's comment and let me know what you think.

[jordan-brough]

@flavorjones thanks for reviewing 🙏

I think this is much smaller than #347. (The code looks larger than it is because I tried to cover edge cases)

PR 347 was a supervisor bolted onto the Rails server. (Server hooks, config stuff, pidfiles, fork/detach, liveness monitoring, exit hooks, ...)

This PR just makes tailwindcss-rails handle SIGTERM in addition to SIGINT.

i.e. it makes it a more well-behaved parent of the child it already has.
We use spawn instead of system now, but those use the same mechanism for launching a child.

It'd be great if foreman got that update to signal process groups, but I think this PR just makes the current approach more robust while we wait for that or a Rails process manager or something else.

[jordan-brough]

fwiw, I also contributed shakacode/shakapacker#888 earlier this year for shakapacker to likewise handle SIGTERM for the child it was spawning, for largely the same reasons. (That one was slightly more complicated).

[jordan-brough]

@dhh any thoughts? (@flavorjones referenced your previous comments)

tl;dr, imo: If you spawn a child, you should forward SIGTERM to it. That's all this PR does.

Unlike the previous #347, there are no threads, PIDs, forking, etc. Those might make sense, but this PR is a small bugfix, imo, for the current approach.

[flavorjones]

@jordan-brough Thanks for clarifying. Like I said, I'm in favor of fixing the problem, but I would very much like to make sure the complexity is in the right place and is the right shape.

(Just a side note, my open-source time is spread very thin across a large number of projects, and so my mental capacity for handling additional complexity is low right now. I appreciate your understanding of that constraint in this discussion.)

I reflected on my resistance to merge this PR as-is: the new method Commands.watch feels to me like it's unnecessarily mixing a process management concern into this gem, rather than keeping process management as a separate concern that the gem merely uses.

Would you be open to repackaging this code in a generic method that accepts env and command (like other Ruby process creation methods)? Then the change set would become:

• new file containing a new module with a singleton method like run_supervised (or ideally a better name!)
• one-line change in build.rake from system(env, *command) to Tailwindcss::Process.run_supervised(env, *command)

A reusable method like I've described cleanly separates the gem's concern of building the command line from the lower-level concern of managing the child process correctly; and it's something that might be a candidate for upstreaming into Active Support (because I believe there are other gems in the Rails ecosystem that need to do something similar).

Would you be open to this direction? I would appreciate your feedback, since you're probably more familiar than I am with the problem space.

[jordan-brough]

@flavorjones that sounds like a better split to me also, I've made that update. 👍

I named it like:

Tailwindcss::ProcessRunner.spawn_and_wait(env, *command)

but lmk if you think a different name would be better.

Thanks for your time on this 🙏

[jordan-brough]

FYI, running bundle install on main branch was resulting in this. I think from the recent version bump?

[flavorjones]

Yeah, that's my bad when I cut the last release. Feel free to include this change or ignore it for now, it'll get sorted eventually.

[jordan-brough]

I’m happy to leave it in if it’s fine for you

[flavorjones]

Brilliant, this looks great!

[flavorjones]

Shipped in v4.6.0! ❤️🙏

[jordan-brough]

Thank you for the attention and the merge!