Dangling tailwind-cli process when using Phoenix Framework

[natrys]

What version of Tailwind CSS are you using?

For example: v3.1.8

What build tool (or framework if it abstracts the build tool) are you using?

Phoenix v1.6.11 (latest) with its tailwind plugin v0.1.8.

What version of Node.js are you using?

N/A. The phoenix tailwind plugin uses the tailwind-cli binary from this repo directly.

What operating system are you using?

Linux

Describe your issue

I believe this issue was originally fixed by Jose Valim's PR which added a listener on stdin. If stdin was closed, it was deemed that the parent process must have died, so tailwind process should clean itself up.

However following a reported issue in turborepo, @thecrypticace added a fix that only watches stdin closing when on TTY, citing esbuild's solution as inspiration. However, the esbuild solution was to listen to stdin closing when NOT on TTY because that's how frameworks like Phoenix spawns background processes like esbuild or tailwind, which sounds sensible to me. In any case, I believe this fix has now caused a regression in Phoenix.

While I think esbuild's solution of stdin listening when not on TTY is the correct approach and will fix this issue for Phoenix (though I haven't tested it), unfortunately I don't know what that will mean for turborepo.

[thecrypticace]

Does Phoenix have the ability to control launched processes? I would expect it to close any child processes before it exits.

You are absolutely right that the fix is the reverse of the one in esbuild but there was a reason for this. The general idea about mentioning it was that it checks if it's a tty or not. Not that it listens for stdin close when it is not a TTY. If you launch a process generally you do NOT have a TTY. But, in these situations, you also generally have direct control over the process so it can be exited by the program that launched it by sending the appropriate signal (a SIGTERM should do it).

The problem here that were solving is:

• A process has launched the CLI (turoborepo, phoenix)
• That process is NOT an interactive shell (so it does not have a TTY) (turborepo, phoenix)
• And it did NOT wire up stdin (turborepo)
• It kills the process at the end (turborepo, idk about phoenix)

This leads to the code we have for our fix. At least in the esbuild case it would also fail running in turborepo if it didn't explicitly close processes — which I believe it does otherwise they'd have the same problem (dangling processes). I would expect that existing the Pheonix server would kill any processes it launched before closing as well.

Unfortunately, at least in Node.js, there does not appear to be a distinction between "I didn't connect stdin" and "I connected it but then it was closed". It's just immediately closed if it has no data. I'm also unsure if this distinction exists at all generally. I'm not sure what the best way to fix this is because, afaik, the only possible fix for this will break anything that doesn't use stdin.

[natrys]

Does Phoenix have the ability to control launched processes?

Based on this comment by Jose Valim, I presume it doesn't.

And it did NOT wire up stdin (turborepo)

I know you will want to strive for maximal compatibility with everything, but maybe that is a bit of a problem? As far as I understand using stdin is the de-facto conventional solution to this problem. Using signal, while not uncommon, is fraught with problems like:

Relying on kill signals can be very dangerous because, if the parent process dies due to an unexpected reason (segfault, no memory, etc), it may leave orphan child processes.

So perhaps my naive view point here is that turborepo should switch to using stdin.

[natrys]

Although, as noted in that thread:

if you wait for stdin to be closed then it can no longer run as a background process

Which could be an issue for some runtimes? I am not sure what's the best fix here is either, other than introducing a flag at the expense of increased cli noise.

[thecrypticace]

I did a bit of poking around about how tools handle stdin closing. It seems like several of them do sometimes but how to enable this behavior differs considerably:

• webpack: only with flag
• Brunch: only with flag
• Vue CLI: only with flag
• Vite: when not CI & not middleware mode
• PostCSS CLI: always
• ESBuild: not TTY
• Parcel: Not implemented
• Dart Sass: Not implemented (technically reverted pending change to: not TTY)
• docker events: Not implemented; (aside: Docker ALSO does not wire up stdin to processes by default. You have to specify that you want stdin available)
• unix cat command: always
• unix tail command: Does not appear to be implemented
• vim: Not implemented
• Python: Always
• Swift: Always
• Turborepo: Not implemented

So even with standard unix/linux tools support for this is very inconsistent and has certain conditions making figuring this out much less clear cut. The one consistent thing is that it seems all the shells exit when stdin closes.

The unfortunate side is that there is no correct answer here and all these tools have slightly different behavior ranging from never to only when {condition} to only when not {condition} to always. Basically every possible option. :/

I do think we need to change this so this is what I see as some possible paths forward:

1. Always listen for stdin close unless --ignore-stdin flag is passed: Requires changes to users of turborepo (sorta matches the idea of esbuild and vite)
2. Never listen for stdin close unless the input is - or a --stdin flag is passed: Requires changes to phoenix (matches webpack, vue CLI, Dart Sass (eventually), Brunch)
3. Only do this for TTY: Requires changes to users of turborepo (sorta matches the idea of esbuild and vite)

I'll also note that I verified that esbuild's watcher does not work with Turborepo and that there is an open issue with them about forwarding stdin: vercel/turborepo#1235

I'll discuss this internally a bit more to see if we can come to a consensus on what we want to do here.

[thecrypticace]

So @reinink and I spent some time today on this and we found two things:

1. Turborepo changed their official Tailwind example to use concurrently: fix(with-tailwind): dev script fails to build tailwindcss vercel/turborepo#1898
2. Concurrently fixes the problem with Turborepo not connecting to stdin

As such we've reverted the change and are suggesting people use concurrently to work around this problem. This fix will be in our next tagged release (and our insiders build in a few minutes)

Thanks for helping us through this one! ✨