Signal handling breaks parent code #1763
Comments
|
[ Quoting <notifications@github.com> in "[coredns/coredns] Signal handling b..." ]
Hi,
cloudflared imports github.com/coredns/coredns, which imports github.com/mholt/caddy.
It looks like coredns calls `TrapSignals()`, which installs its own signal handler for SIGTERM/SIGINT/etc, and since in UNIX/Linux there can only be one handler per process for these signals, that causes our own handlers in cloudflared to not get triggered.
The signal handler stuff in caddy makes sense for situations when it's being ran as a server, however when it's being used as a lib, it should probably be more mindful of what the parent code may choose to do with its own signal handlers.
For now, we have just commented out `trapSignalsCrossPlatform()` and some handlers in `trapSignalsPosix()` in the caddy package that we actually wanna handle in our own codebase. However, this is not a long-term solution. Ideally, we would be able to just pull coreDNS (and its transitive dependency caddy) from the origin and use it without having this issue.
Would it be possible to stop trapping signals in coredns?
If you use CoreDNS as library it shouldn't set TrapSignals() as this is only
done in coremain/run.go
Ah it seems we're picking this up in the metrics package (and we shouldn't)
plugin/metrics/setup.go: "github.com/coredns/coredns/coremain"
plugin/metrics/setup.go: buildInfo.WithLabelValues(coremain.CoreVersion, coremain.GitCommit, runtime.Version()).Set(1)
plugin/chaos/setup.go: // Set here so we pick up AppName and AppVersion that get set in coremain's init().
|
|
Should we fix this and submit a PR? |
|
Yes, but the metric is still useful, so please try to keep it.
…On Thu, 3 May 2018, 07:56 Silver, ***@***.***> wrote:
Should we fix this and submit a PR?
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#1763 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAVkW_OdSMbwi9ZSskWPc8APA2G8E4M4ks5tupwRgaJpZM4Twhv7>
.
|
|
Quick question -- why are so many of the dependencies (including Caddy) listed in the |
|
Build breaks if we do that. Hope vgo will fix all that
…On Thu, 3 May 2018, 08:20 Silver, ***@***.***> wrote:
Quick question -- why are so many of the dependencies (including Caddy)
listed in the ignored section of Gopkg.toml?
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#1763 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAVkW09JwZciW3jFtuakcR7rZoWswy-Zks5tuqHGgaJpZM4Twhv7>
.
|
|
Would you be happy if we moved |
|
Oh yeah that would work
…On Thu, 3 May 2018, 08:23 Silver, ***@***.***> wrote:
Would you be happy if we moved caddy.TrapSignals() from init() to Run()
in coremain/run.go?
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#1763 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAVkW8CO6KFUFv3l8c4xhdBnfbEt74iOks5tuqJ1gaJpZM4Twhv7>
.
|
sssilver
added a commit
to sssilver/coredns
that referenced
this issue
May 3, 2018
|
#1764 is ready for review. |
miekg
pushed a commit
that referenced
this issue
May 3, 2018
|
/close |
Jason-ZW
pushed a commit
to rancher/coredns
that referenced
this issue
Apr 17, 2019
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Hi,
cloudflared imports github.com/coredns/coredns, which imports github.com/mholt/caddy.
It looks like coredns calls
TrapSignals(), which installs its own signal handler for SIGTERM/SIGINT/etc, and since in UNIX/Linux there can only be one handler per process for these signals, that causes our own handlers in cloudflared to not get triggered.The signal handler stuff in caddy makes sense for situations when it's being ran as a server, however when it's being used as a lib, it should probably be more mindful of what the parent code may choose to do with its own signal handlers.
For now, we have just commented out
trapSignalsCrossPlatform()and some handlers intrapSignalsPosix()in the caddy package that we actually wanna handle in our own codebase. However, this is not a long-term solution. Ideally, we would be able to just pull coreDNS (and its transitive dependency caddy) from the origin and use it without having this issue.Would it be possible to stop trapping signals in coredns?
Many thanks.
The text was updated successfully, but these errors were encountered: