[Win32 Signals] Add term and ctrl-c signal handlers #13954

davinci26 · 2020-11-10T01:02:23Z

Signed-off-by: Sotiris Nanopoulos sonanopo@microsoft.com

Part 1 of #13188. Adds support for ctrl+c and ctrl+break for Envoy on Windows.

The implementation for this following:

On platform_impl we register a CtrlHandler which runs on a separate thread.
On signal_impl we register a read event reader.

Thread (1) and (2) communicate via a socket pair and the event is handled on Windows the same way as it is handled on POSIX

Risk Level: Low (Windows Only)
Testing: Manual testing + integration tests
Docs Changes: N/A
Release Notes: N/A

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

davinci26 · 2020-11-10T01:02:39Z

@envoyproxy/windows-dev

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

wrowe

This approach is otherwise looking good to me. In the shutdown/logoff cases, I've found that returning immediately from the CtrlHandler causes the typical "now attempt to kill" reaction immediately from the OS, so a delay is usually helpful.

include/envoy/common/platform.h

test/exe/outofproc_main_test.py

.bazelrc

source/server/server.cc

antoniovicente

Thanks for the continued help to improve cross platform support.

include/envoy/common/platform.h

source/server/server.cc

davinci26 · 2020-11-12T16:51:11Z

I am adding a /wait tag as I am working more with the automated test. @sunjayBhatia was right and the ctrl+break is caught from the test regardless of the signal handling. I am working on adding ctrl + c signals in the tests but it is a bit tricky.

davinci26 · 2020-11-12T17:06:08Z

/wait

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

ci/run_clang_tidy.sh

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

davinci26 · 2020-11-30T19:57:32Z

@sunjayBhatia thanks for the suggestion! It worked.

davinci26 · 2020-11-30T22:07:29Z

/retest

repokitteh-read-only · 2020-11-30T22:07:34Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #13954 (comment) was created by @davinci26.

see: more, trace.

davinci26 · 2020-11-30T22:48:23Z

@wrowe and @antoniovicente PTAL when you get some time

antoniovicente

Change looks pretty much ready to merge. Just a few minor questions. Sorry for the delays in review, this one really went to the back burner.

source/common/event/win32/signal_impl.cc

source/exe/win32/platform_impl.cc

source/common/event/win32/signal_impl.h

test/exe/win32_outofproc_main_test.cc

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

davinci26 · 2020-12-02T22:59:48Z

/retest

repokitteh-read-only · 2020-12-02T22:59:52Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #13954 (comment) was created by @davinci26.

see: more, trace.

antoniovicente

Thanks!

davinci26 · 2020-12-04T17:27:18Z

@envoyproxy/senior-maintainers can I also get a review from one of you!

source/common/event/win32/signal_impl.cc

source/exe/win32/platform_impl.cc

.bazelrc

source/exe/win32/platform_impl.cc

source/server/server.cc

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

davinci26 · 2020-12-05T00:40:07Z

@ggreenway I addressed most of the comments and I am looking for some further clarification is some comments that I have left open. Thanks for the code review!

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

ggreenway

LGTM

* master: (41 commits) event: Remove a source of non-determinism by always running deferred deletion before post callbacks (envoyproxy#14293) Fix TSAN bug in integration test (envoyproxy#14327) tracing: Add hostname to Zipkin config. (envoyproxy#14186) (envoyproxy#14187) [conn_pool] fix use after free in H/1 connection pool (envoyproxy#14220) lua: update deprecated lua_open to luaL_newstate (envoyproxy#14297) extension: use bool_flag to control extension link (envoyproxy#14240) stats: Factor out creation of cluster-stats StatNames from creation of the stats, to save CPU during xDS updates (envoyproxy#14028) test: add scaled timer integration test (envoyproxy#14290) [Win32 Signals] Add term and ctrl-c signal handlers (envoyproxy#13954) config: v2 transport API fatal-by-default. (envoyproxy#14223) matcher: fix UB bug caused by dereferencing a bad optional (envoyproxy#14271) test: putting fake upstream config in a struct (envoyproxy#14266) wasm: use Bazel rules from Proxy-Wasm Rust SDK. (envoyproxy#14292) docs: fix typo (envoyproxy#14237) dependencies: allowlist CVE-2018-21270 to prevent false positives. (envoyproxy#14294) typo in redis doc (envoyproxy#14248) access_loggers: removed redundant dep (envoyproxy#14274) fix http2 flaky test (envoyproxy#14261) test: disable flaky xds_integration_test. (envoyproxy#14287) http: add functionality to configure kill header in KillRequest proto (envoyproxy#14288) ... Signed-off-by: Michael Puncel <mpuncel@squareup.com>

Sotiris Nanopoulos added 7 commits November 6, 2020 19:22

unix builds

c9939d9

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

fix format 1

ab8b2e2

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

format vol2

2819000

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

Win32 changes + tests

3774592

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

add tests

f55a385

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

fix format

f5ae7e0

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

fix pyformat

c5f267f

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

antoniovicente requested review from wrowe and antoniovicente November 10, 2020 22:06

antoniovicente assigned wrowe and antoniovicente Nov 10, 2020

Sotiris Nanopoulos added 5 commits November 10, 2020 15:06

fix typo

4159136

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

working through build errors

709cbac

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

fix with runfiles

3158e46

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

use the same if conditions everywhere

2a16309

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

fix format + deps

e9fde7f

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

wrowe suggested changes Nov 11, 2020

View reviewed changes

include/envoy/common/platform.h Outdated Show resolved Hide resolved

sunjayBhatia reviewed Nov 11, 2020

View reviewed changes

test/exe/outofproc_main_test.py Outdated Show resolved Hide resolved

sunjayBhatia suggested changes Nov 11, 2020

View reviewed changes

.bazelrc Show resolved Hide resolved

sunjayBhatia reviewed Nov 11, 2020

View reviewed changes

source/server/server.cc Outdated Show resolved Hide resolved

antoniovicente reviewed Nov 12, 2020

View reviewed changes

include/envoy/common/platform.h Outdated Show resolved Hide resolved

source/server/server.cc Outdated Show resolved Hide resolved

repokitteh-read-only bot added the waiting label Nov 12, 2020

PR comments and robust testing

598b933

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

repokitteh-read-only bot removed the waiting label Nov 18, 2020

Sotiris Nanopoulos added 3 commits November 17, 2020 21:33

format changes

8cce5c9

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

order includes

44d4602

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

fix include order vol 2

4d900be

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

sunjayBhatia reviewed Nov 30, 2020

View reviewed changes

ci/run_clang_tidy.sh Outdated Show resolved Hide resolved

fix typo

6cc663b

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

davinci26 mentioned this pull request Nov 30, 2020

Windows: reverse_bridge_integration_test is flaky #14218

Closed

antoniovicente reviewed Dec 1, 2020

View reviewed changes

Change from map to array

c378f5f

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

davinci26 mentioned this pull request Dec 2, 2020

[Events] Add an file event wakeup method with socketpair #14244

Open

Spelling

7550099

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

davinci26 requested a review from antoniovicente December 3, 2020 18:57

antoniovicente previously approved these changes Dec 3, 2020

View reviewed changes

ggreenway self-assigned this Dec 4, 2020

ggreenway requested changes Dec 4, 2020

View reviewed changes

fix pr comments by greg

30bbdfa

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

davinci26 dismissed antoniovicente’s stale review via 30bbdfa December 5, 2020 00:38

Sotiris Nanopoulos added 4 commits December 4, 2020 17:26

fix spelling mistake

a28a4f0

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

Merge remote-tracking branch 'upstream/master' into processManagement

85e19e3

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

fix allocation + add panic

63e7ced

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

remove double signal entry

1f1e211

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

ggreenway approved these changes Dec 7, 2020

View reviewed changes

ggreenway merged commit 9e2df02 into envoyproxy:master Dec 8, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Win32 Signals] Add term and ctrl-c signal handlers #13954

[Win32 Signals] Add term and ctrl-c signal handlers #13954

davinci26 commented Nov 10, 2020

davinci26 commented Nov 10, 2020

wrowe left a comment

antoniovicente left a comment

davinci26 commented Nov 12, 2020

davinci26 commented Nov 12, 2020

davinci26 commented Nov 30, 2020

davinci26 commented Nov 30, 2020

repokitteh-read-only bot commented Nov 30, 2020

davinci26 commented Nov 30, 2020

antoniovicente left a comment

davinci26 commented Dec 2, 2020

repokitteh-read-only bot commented Dec 2, 2020

antoniovicente left a comment

davinci26 commented Dec 4, 2020

davinci26 commented Dec 5, 2020

ggreenway left a comment

[Win32 Signals] Add term and ctrl-c signal handlers #13954

[Win32 Signals] Add term and ctrl-c signal handlers #13954

Conversation

davinci26 commented Nov 10, 2020

davinci26 commented Nov 10, 2020

wrowe left a comment

Choose a reason for hiding this comment

antoniovicente left a comment

Choose a reason for hiding this comment

davinci26 commented Nov 12, 2020

davinci26 commented Nov 12, 2020

davinci26 commented Nov 30, 2020

davinci26 commented Nov 30, 2020

repokitteh-read-only bot commented Nov 30, 2020

davinci26 commented Nov 30, 2020

antoniovicente left a comment

Choose a reason for hiding this comment

davinci26 commented Dec 2, 2020

repokitteh-read-only bot commented Dec 2, 2020

antoniovicente left a comment

Choose a reason for hiding this comment

davinci26 commented Dec 4, 2020

davinci26 commented Dec 5, 2020

ggreenway left a comment

Choose a reason for hiding this comment