runtime: treat CTRL_CLOSE_EVENT, CTRL_LOGOFF_EVENT, CTRL_SHUTDOWN_EVENT as SIGTERM on Windows #33311
Conversation
googlebot
added
the
cla: yes
|
This PR (HEAD: e184f33) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/c/go/+/187739 to see it. Tip: You can toggle comments from me using the |
|
Message from Gobot Gobot: Patch Set 1: Congratulations on opening your first change. Thank you for your contribution! Next steps: Most changes in the Go project go through a few rounds of revision. This can be During May-July and Nov-Jan the Go project is in a code freeze, during which Please don’t reply on this GitHub thread. Visit golang.org/cl/187739. |
|
Message from Alex Brainman: Patch Set 1: (2 comments) Thank you for working on this. Alex Please don’t reply on this GitHub thread. Visit golang.org/cl/187739. |
|
Message from Tianon Gravi: Patch Set 1: (2 comments) Thanks for the review! :D Please don’t reply on this GitHub thread. Visit golang.org/cl/187739. |
|
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
googlebot
added
cla: no
|
This PR (HEAD: eac2d56) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/c/go/+/187739 to see it. Tip: You can toggle comments from me using the |
|
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
googlebot
added
cla: yes
|
This PR (HEAD: 4e85db2) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/c/go/+/187739 to see it. Tip: You can toggle comments from me using the |
|
Message from Tianon Gravi: Patch Set 4: Commit message was updated. Please don’t reply on this GitHub thread. Visit golang.org/cl/187739. |
|
Message from Alex Brainman: Patch Set 6: (3 comments) Looks better now. Thank you very much. Alex Please don’t reply on this GitHub thread. Visit golang.org/cl/187739. |
|
Message from Alex Brainman: Patch Set 6: Ian, Are you OK with this CL mapping CTRL_CLOSE_EVENT, CTRL_LOGOFF_EVENT and CTRL_SHUTDOWN_EVENT onto syscall.SIGTERM ? That was what people suggested on #7479 but I know nothing about these things. Should we have wider discussion about this? Alex Please don’t reply on this GitHub thread. Visit golang.org/cl/187739. |
|
Message from Ian Lance Taylor: Patch Set 6:
I haven't really looked at the CL, but I'm fine with the idea of mapping those Windows events to SIGTERM (for the 1.14 release). Please don’t reply on this GitHub thread. Visit golang.org/cl/187739. |
|
Message from Tianon Gravi: Patch Set 6: (2 comments) Please don’t reply on this GitHub thread. Visit golang.org/cl/187739. |
|
This PR (HEAD: 318eda8) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/c/go/+/187739 to see it. Tip: You can toggle comments from me using the |
|
Message from Tianon Gravi: Patch Set 7: (1 comment) Please don’t reply on this GitHub thread. Visit golang.org/cl/187739. |
|
Message from Alex Brainman: Patch Set 7: (2 comments) Getting there. Thank you. Alex Please don’t reply on this GitHub thread. Visit golang.org/cl/187739. |
…NT as SIGTERM on Windows This matches the existing behavior of treating CTRL_C_EVENT, CTRL_BREAK_EVENT as a synthesized SIGINT event. See https://docs.microsoft.com/en-us/windows/console/handlerroutine for a good documentation source upstream to confirm these values. As for the usage of these events, the "Timeouts" section of that upstream documentation is important to note, especially the limited window in which to do any cleanup before the program will be forcibly killed (defaults typically 5s, but as low as 500ms, and in many cases configurable system-wide). These events are especially relevant for Windows containers, where these events (particularly `CTRL_SHUTDOWN_EVENT`) are one of the only ways containers can "gracefully" shut down (moby/moby#25982 (comment)). This was verified by making a simple `main()` which implements the same code as in `ExampleNotify_allSignals` but in a `for` loop, building a `main.exe`, running that in a container, then doing `docker kill -sTERM` on said container. The program prints `Got signal: SIGTERM`, then exits after the aforementioned timeout, as expected. Behavior before this patch is that the program gets no notification (and thus no output) but still exits after the timeout. Fixes golang#7479
|
This PR (HEAD: 9e05d63) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/c/go/+/187739 to see it. Tip: You can toggle comments from me using the |
|
Message from Tianon Gravi: Patch Set 8: (1 comment) Updated! Please don’t reply on this GitHub thread. Visit golang.org/cl/187739. |
|
Message from paul deauna: Patch Set 8: Then>resume where safe Please don’t reply on this GitHub thread. Visit golang.org/cl/187739. |
|
Message from Alex Brainman: Patch Set 8: Run-TryBot+1 Please don’t reply on this GitHub thread. Visit golang.org/cl/187739. |
|
Message from Gobot Gobot: Patch Set 8: TryBots beginning. Status page: https://farmer.golang.org/try?commit=efd7cf48 Please don’t reply on this GitHub thread. Visit golang.org/cl/187739. |
|
Message from Gobot Gobot: Patch Set 8: TryBot-Result+1 TryBots are happy. Please don’t reply on this GitHub thread. Visit golang.org/cl/187739. |
|
Message from Alex Brainman: Patch Set 8: This CL LGTM. Thank you very much. I will submit this CL once Go tree opens for submission as per https://groups.google.com/d/msg/golang-dev/UQOSRilopzw/Xtth3MQmAgAJ Feel free to ping me here, so I don't forget. Alex RELNOTE=yes Please don’t reply on this GitHub thread. Visit golang.org/cl/187739. |
|
Message from Tianon Gravi: Patch Set 8:
Friendly ping? :) (https://groups.google.com/d/msg/golang-dev/YMw7aBRvk-4/z-t2bIaaAwAJ) Please don’t reply on this GitHub thread. Visit golang.org/cl/187739. |
…NT as SIGTERM on Windows This matches the existing behavior of treating CTRL_C_EVENT, CTRL_BREAK_EVENT as a synthesized SIGINT event. See https://docs.microsoft.com/en-us/windows/console/handlerroutine for a good documentation source upstream to confirm these values. As for the usage of these events, the "Timeouts" section of that upstream documentation is important to note, especially the limited window in which to do any cleanup before the program will be forcibly killed (defaults typically 5s, but as low as 500ms, and in many cases configurable system-wide). These events are especially relevant for Windows containers, where these events (particularly `CTRL_SHUTDOWN_EVENT`) are one of the only ways containers can "gracefully" shut down (moby/moby#25982 (comment)). This was verified by making a simple `main()` which implements the same code as in `ExampleNotify_allSignals` but in a `for` loop, building a `main.exe`, running that in a container, then doing `docker kill -sTERM` on said container. The program prints `Got signal: SIGTERM`, then exits after the aforementioned timeout, as expected. Behavior before this patch is that the program gets no notification (and thus no output) but still exits after the timeout. Fixes #7479 Change-Id: I2af79421cd484a0fbb9467bb7ddb5f0e8bc3610e GitHub-Last-Rev: 9e05d63 GitHub-Pull-Request: #33311 Reviewed-on: https://go-review.googlesource.com/c/go/+/187739 Run-TryBot: Alex Brainman <alex.brainman@gmail.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Alex Brainman <alex.brainman@gmail.com>
|
Message from Alex Brainman: Patch Set 8: Code-Review+2
Thank you for reminding me. I will try and submit your change. Alex Please don’t reply on this GitHub thread. Visit golang.org/cl/187739. |
|
This PR is being closed because golang.org/cl/187739 has been merged. |
…NT as SIGTERM on Windows This matches the existing behavior of treating CTRL_C_EVENT, CTRL_BREAK_EVENT as a synthesized SIGINT event. See https://docs.microsoft.com/en-us/windows/console/handlerroutine for a good documentation source upstream to confirm these values. As for the usage of these events, the "Timeouts" section of that upstream documentation is important to note, especially the limited window in which to do any cleanup before the program will be forcibly killed (defaults typically 5s, but as low as 500ms, and in many cases configurable system-wide). These events are especially relevant for Windows containers, where these events (particularly `CTRL_SHUTDOWN_EVENT`) are one of the only ways containers can "gracefully" shut down (moby/moby#25982 (comment)). This was verified by making a simple `main()` which implements the same code as in `ExampleNotify_allSignals` but in a `for` loop, building a `main.exe`, running that in a container, then doing `docker kill -sTERM` on said container. The program prints `Got signal: SIGTERM`, then exits after the aforementioned timeout, as expected. Behavior before this patch is that the program gets no notification (and thus no output) but still exits after the timeout. Fixes golang#7479 Change-Id: I2af79421cd484a0fbb9467bb7ddb5f0e8bc3610e GitHub-Last-Rev: 9e05d63 GitHub-Pull-Request: golang#33311 Reviewed-on: https://go-review.googlesource.com/c/go/+/187739 Run-TryBot: Alex Brainman <alex.brainman@gmail.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Alex Brainman <alex.brainman@gmail.com>
…NT as SIGTERM on Windows This matches the existing behavior of treating CTRL_C_EVENT, CTRL_BREAK_EVENT as a synthesized SIGINT event. See https://docs.microsoft.com/en-us/windows/console/handlerroutine for a good documentation source upstream to confirm these values. As for the usage of these events, the "Timeouts" section of that upstream documentation is important to note, especially the limited window in which to do any cleanup before the program will be forcibly killed (defaults typically 5s, but as low as 500ms, and in many cases configurable system-wide). These events are especially relevant for Windows containers, where these events (particularly `CTRL_SHUTDOWN_EVENT`) are one of the only ways containers can "gracefully" shut down (moby/moby#25982 (comment)). This was verified by making a simple `main()` which implements the same code as in `ExampleNotify_allSignals` but in a `for` loop, building a `main.exe`, running that in a container, then doing `docker kill -sTERM` on said container. The program prints `Got signal: SIGTERM`, then exits after the aforementioned timeout, as expected. Behavior before this patch is that the program gets no notification (and thus no output) but still exits after the timeout. Fixes golang#7479 Change-Id: I2af79421cd484a0fbb9467bb7ddb5f0e8bc3610e GitHub-Last-Rev: 9e05d63 GitHub-Pull-Request: golang#33311 Reviewed-on: https://go-review.googlesource.com/c/go/+/187739 Run-TryBot: Alex Brainman <alex.brainman@gmail.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Alex Brainman <alex.brainman@gmail.com>
This matches the existing behavior of treating CTRL_C_EVENT, CTRL_BREAK_EVENT as a synthesized SIGINT event.
See https://docs.microsoft.com/en-us/windows/console/handlerroutine for a good documentation source upstream to confirm these values.
As for the usage of these events, the "Timeouts" section of that upstream documentation is important to note, especially the limited window in which to do any cleanup before the program will be forcibly killed (defaults typically 5s, but as low as 500ms, and in many cases configurable system-wide).
These events are especially relevant for Windows containers, where these events (particularly
CTRL_SHUTDOWN_EVENT) are one of the only ways containers can "gracefully" shut down (moby/moby#25982 (comment)).This was verified by making a simple
main()which implements the same code as inExampleNotify_allSignalsbut in aforloop, building amain.exe, running that in a container, then doingdocker kill -sTERMon said container. The program printsGot signal: SIGTERM, then exits after the aforementioned timeout, as expected. Behavior before this patch is that the program gets no notification (and thus no output) but still exits after the timeout.Fixes #7479