sigproxy: return after closing the channel #5036
Conversation
openshift-ci-robot
added
approved
| @@ -30,6 +30,7 @@ func ProxySignals(ctr *libpod.Container) { | |||
| if err := syscall.Kill(syscall.Getpid(), s.(syscall.Signal)); err != nil { | |||
There was a problem hiding this comment.
Wait, what is this even doing? Why are we double-sending the signal to ourself on errors?
There was a problem hiding this comment.
From git-blame, it looks like an attempt to make C-c kill Podman if sig-proxy fails because the container is stopped. I don't know if this is a good idea, or necessary at all, but it's probably not harmful
There was a problem hiding this comment.
I had the very same question and found https://github.com/containers/libpod/pull/1086/files#r202198031.
There was a problem hiding this comment.
But I am not sure about the correctness after all
There was a problem hiding this comment.
Should this be the container?
There was a problem hiding this comment.
This makes (some) sense to me as is — as long as the container is running, attempts to gracefully terminate Podman are redirected to gracefully terminating the container. (The downside is that if the container is uncooperative, it’s difficult to terminate Podman.)
If the container dies, and we find out here, we need to forward that one signal to ourselves so that it is not lost, and then we terminate the proxy and let the defaults play out.
(Pedantically, at the time of StopCatch, several other signals may be queued in sigBuffer, and we throw them away. For the typical SIGINT/SIGTERM case this doesn’t matter much.)
There was a problem hiding this comment.
Tests are green now. Mind dropping a LGTM if you agree to the changes?
|
LGTM |
|
Could we add a comment similar to Miloslav's in the code to avoid future head scratching? |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mtrmac, vrothberg The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
When stopping signal handling (e.g., to properly handle ^C) we are also closing the signal channel. We should really return from the go-routine instead of continuing and risking double-closing the channel which leads to a panic. Fixes: containers#5034 Signed-off-by: Valentin Rothberg <rothberg@redhat.com>
Good point! I added a comment a repushed. |
|
LGTM, thanks for the comment. |
|
/lgtm |
openshift-ci-robot
added
do-not-merge/hold
|
/hold cancel |
openshift-ci-robot
removed
the
do-not-merge/hold
When stopping signal handling (e.g., to properly handle ^C) we are also
closing the signal channel. We should really return from the go-routine
instead of continuing and risking double-closing the channel which leads
to a panic.
Fixes: #5034
Signed-off-by: Valentin Rothberg rothberg@redhat.com