Redo signal handling #854
Merged
Redo signal handling #854
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There looks to be a better approach here with how we start the process that avoids many other problems with the signals. It is platform dependent, but given how much better it is, we should use this.
If we set it to the foreground, it will have it's own process ground and receive signals directly without having to set up any proxying or forwarding in the CLI itself.
We need this on Windows as there doesn't seem to be a good other way to have the foreground process and handle signals correctly.
fatih
approved these changes
Apr 22, 2024
fatih
approved these changes
Apr 22, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
These changes rework how we do signal handling after #853. It turns out, trying to forward signals is not really what we want.
At the code here is that different platforms behave in different ways with how signals are handled. The specific cause of the remaining issue described in #850 is that on Linux and unix like platforms, the normal behavior on
ctrl+cis that all processes in the current group get sent actrl+c.If we forward signals, we receive the
ctrl+ctwice inside MySQL leading to weird behavior. But we also don't want to ignore the signals either, since a singleSIGINTsent topscaleis still something we want to act on. On Windows though, the current signal handling setup does seem to work as desired.So we have to split things up here per platform. On Unix like platforms, we can use https://pkg.go.dev/syscall#SysProcAttr and set
Foreground. That will implySetpgidas well, which means we get a new process group for the process we're spawning and it attaches it to the foreground. That means now MySQL has the foreground as an independent process group, so it gets the signals appropriately and it's as if we'd directly have started MySQL.On Windows we keep using the same signal handling logic mostly, but slightly simplified. Lastly, we fix the warning signal for prod branches since on Windows it looks like
⚠doesn't actually work.