Abort proxy when stdin gets closed #3638
Conversation
internal/command/proxy/proxy.go
Outdated
| // Note that we don't do this when stdin is TTY, because that prevents | ||
| // the process from being moved to a background job on Unix. | ||
| // See https://github.com/brunch/brunch/issues/998. | ||
| func watchStdinAndAbortOnClose(ctx context.Context) { |
There was a problem hiding this comment.
I changed only the fly proxy specifically, but there are a few that could probably use the same logic, like fly machine api-proxy and fly redis proxy. I can enable the watching there too, just let me know what is the best place to put this function to share across commands.
There was a problem hiding this comment.
hi @jonatanklosko, sorry I haven't replied, I saw it days ago and took a step back because my first reaction was "what a gross hack" and preferred to give it some time to settle in my mind 😄
I mean, it is kind of clever thing to do but also could be a source of gotcha moment for someone that doesn't expect the proxy to terminate out of nowhere. Are you aware of any tool that does this trick?
I think we can work towards merging it anyways but:
- Instead of making it the default, can this terminate-on-stdin-close mode be behind a command line flag?
- Instead of calling
os.Exit(), can it cancel the context used by the proxy to gracefully stop it?
There was a problem hiding this comment.
@dangra no worries, it seems like a hack, but many command line tools actually do that, like cat, fsevents, esbuild, webpack, postcss.
A flag sounds good to me, that's what webpack does.
Should I share and apply this to other proxies, or do you prefer to keep it for this one specifically?
can it cancel the context used by the proxy to gracefully stop it?
How would the cancel look like? :)
There was a problem hiding this comment.
A flag sounds good to me, that's what webpack does.
sgtm!
Should I share and apply this to other proxies, or do you prefer to keep it for this one specifically?
what other proxies do you refer to?
How would the cancel look like? :)
I realize this a nitpick and won't make much difference for this case, but I'd derive the context passed to proxy.Connect() using context.WithCancelCause(); and then call the cancel function instead of os.Exit().
That should allow the proxy to gracefully shutdown instead of abruptly exit.
There was a problem hiding this comment.
what other proxies do you refer to?
This is fly proxy, I noticed the proxy is also used in fly machine api-proxy and fly redis proxy, so I wonder if all of these should have the flag for consistency.
There was a problem hiding this comment.
FWIW I only need this for fly proxy, so I'm happy having the flag just here.
There was a problem hiding this comment.
Ok! I added the flag and context cancel, let me know if there's anything else :)
|
Kindly ping :) |
|
Good stuff @jonatanklosko . thanks :) |
|
Fantastic, thanks for the help! |
There is no guarantee that a OS process spawning
fly proxy ...will terminate it, however the stdin is always closed when the parent terminates. In order to avoid zombie processes, we can watch stdin and when it gets closed we terminate the program. The original motivation is spawning fly proxy from Erlang VM.For more context see a similar PR on esbuild: evanw/esbuild#1449.