Skip to content

Forward stop signals to tailwindcss watcher#621

Open
jordan-brough wants to merge 1 commit into
rails:mainfrom
jordan-brough:sigterm-handling
Open

Forward stop signals to tailwindcss watcher#621
jordan-brough wants to merge 1 commit into
rails:mainfrom
jordan-brough:sigterm-handling

Conversation

@jordan-brough
Copy link
Copy Markdown

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.

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
Copy link
Copy Markdown
Member

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
Copy link
Copy Markdown
Author

@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
Copy link
Copy Markdown
Author

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. In that instance we had two cases and were able to use exec in one case, but still needed to use signal handling in the other.

@jordan-brough
Copy link
Copy Markdown
Author

@flavorjones any thoughts?
This PR keeps the existing approach. We still spawn tailwind and block until it exits (system is just spawn+wait).
The only difference is that we now forward INT and TERM, the way a parent process generally should. INT usually made it through already because terminals send Ctrl-C to the whole process group, but TERM just hasn't been handled yet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants