Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 35 commits into from
Dec 8, 2020
Merged

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

merged 35 commits into from
Dec 8, 2020

Conversation

davinci26
Copy link
Member

Signed-off-by: Sotiris Nanopoulos [email protected]

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

The implementation for this following:

  1. On platform_impl we register a CtrlHandler which runs on a separate thread.
  2. 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

Sotiris Nanopoulos added 7 commits November 6, 2020 19:22
unix builds
c9939d9 
Signed-off-by: Sotiris Nanopoulos <[email protected]>
fix format 1
ab8b2e2 
Signed-off-by: Sotiris Nanopoulos <[email protected]>
format vol2
2819000 
Signed-off-by: Sotiris Nanopoulos <[email protected]>
Win32 changes + tests
3774592 
Signed-off-by: Sotiris Nanopoulos <[email protected]>
add tests
f55a385 
Signed-off-by: Sotiris Nanopoulos <[email protected]>
fix format
f5ae7e0 
Signed-off-by: Sotiris Nanopoulos <[email protected]>
fix pyformat
c5f267f 
Signed-off-by: Sotiris Nanopoulos <[email protected]>
@davinci26
Copy link
Member Author

@envoyproxy/windows-dev

Sotiris Nanopoulos added 5 commits November 10, 2020 15:06
fix typo
4159136 
Signed-off-by: Sotiris Nanopoulos <[email protected]>
working through build errors
709cbac 
Signed-off-by: Sotiris Nanopoulos <[email protected]>
fix with runfiles
3158e46 
Signed-off-by: Sotiris Nanopoulos <[email protected]>
use the same if conditions everywhere
2a16309 
Signed-off-by: Sotiris Nanopoulos <[email protected]>
fix format + deps
e9fde7f 
Signed-off-by: Sotiris Nanopoulos <[email protected]>
wrowe
wrowe suggested changes Nov 11, 2020
View reviewed changes
Copy link
Contributor

@wrowe wrowe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
sunjayBhatia
sunjayBhatia suggested changes Nov 11, 2020
View reviewed changes
.bazelrc Show resolved Hide resolved
antoniovicente
antoniovicente reviewed Nov 12, 2020
View reviewed changes
Copy link
Contributor

@antoniovicente antoniovicente left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the continued help to improve cross platform support.

include/envoy/common/platform.h Outdated Show resolved Hide resolved
source/server/server.cc Outdated Show resolved Hide resolved
@davinci26
Copy link
Member Author

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
Copy link
Member Author

/wait

PR comments and robust testing
598b933 
Signed-off-by: Sotiris Nanopoulos <[email protected]>
Sotiris Nanopoulos added 3 commits November 17, 2020 21:33
format changes
8cce5c9 
Signed-off-by: Sotiris Nanopoulos <[email protected]>
order includes
44d4602 
Signed-off-by: Sotiris Nanopoulos <[email protected]>
fix include order vol 2
4d900be 
Signed-off-by: Sotiris Nanopoulos <[email protected]>
sunjayBhatia
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 <[email protected]>
@davinci26
Copy link
Member Author

@sunjayBhatia thanks for the suggestion! It worked.

@davinci26 davinci26 mentioned this pull request Nov 30, 2020
Closed
@davinci26
Copy link
Member Author

/retest

@repokitteh-read-only
Copy link

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

🐱

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

see: more, trace.

@davinci26
Copy link
Member Author

@wrowe and @antoniovicente PTAL when you get some time

antoniovicente
antoniovicente reviewed Dec 1, 2020
View reviewed changes
Copy link
Contributor

@antoniovicente antoniovicente left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
source/common/event/win32/signal_impl.cc Show resolved Hide resolved
source/exe/win32/platform_impl.cc Outdated Show resolved Hide resolved
source/common/event/win32/signal_impl.h Outdated Show resolved Hide resolved
test/exe/win32_outofproc_main_test.cc Outdated Show resolved Hide resolved
Change from map to array
c378f5f 
Signed-off-by: Sotiris Nanopoulos <[email protected]>
@davinci26 davinci26 mentioned this pull request Dec 2, 2020
Open
Spelling
7550099 
Signed-off-by: Sotiris Nanopoulos <[email protected]>
@davinci26
Copy link
Member Author

/retest

@repokitteh-read-only
Copy link

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

🐱

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

see: more, trace.

antoniovicente
antoniovicente previously approved these changes Dec 3, 2020
View reviewed changes
Copy link
Contributor

@antoniovicente antoniovicente left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@davinci26
Copy link
Member Author

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

@ggreenway ggreenway self-assigned this Dec 4, 2020
ggreenway
ggreenway requested changes Dec 4, 2020
View reviewed changes
source/common/event/win32/signal_impl.cc Outdated Show resolved Hide resolved
source/exe/win32/platform_impl.cc Outdated Show resolved Hide resolved
.bazelrc Show resolved Hide resolved
source/exe/win32/platform_impl.cc Outdated Show resolved Hide resolved
source/exe/win32/platform_impl.cc Outdated Show resolved Hide resolved
source/server/server.cc Outdated Show resolved Hide resolved
fix pr comments by greg
30bbdfa 
Signed-off-by: Sotiris Nanopoulos <[email protected]>
@davinci26
Copy link
Member Author

@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!

Sotiris Nanopoulos added 4 commits December 4, 2020 17:26
fix spelling mistake
a28a4f0 
Signed-off-by: Sotiris Nanopoulos <[email protected]>
Merge remote-tracking branch 'upstream/master' into processManagement
85e19e3 
Signed-off-by: Sotiris Nanopoulos <[email protected]>
fix allocation + add panic
63e7ced 
Signed-off-by: Sotiris Nanopoulos <[email protected]>
remove double signal entry
1f1e211 
Signed-off-by: Sotiris Nanopoulos <[email protected]>
ggreenway
ggreenway approved these changes Dec 7, 2020
View reviewed changes
Copy link
Contributor

@ggreenway ggreenway left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ggreenway ggreenway merged commit 9e2df02 into envoyproxy:master Dec 8, 2020
mpuncel added a commit to mpuncel/envoy that referenced this pull request Dec 8, 2020
@mpuncel
Merge branch 'master' into mpuncel/http2-hc-goaway
2b0d930 
* 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 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@antoniovicente antoniovicente antoniovicente left review comments

@sunjayBhatia sunjayBhatia sunjayBhatia requested changes

@wrowe wrowe wrowe requested changes

@ggreenway ggreenway ggreenway approved these changes

Assignees

@wrowe wrowe

@ggreenway ggreenway

@antoniovicente antoniovicente

Labels
area/windows
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@davinci26 @antoniovicente @sunjayBhatia @wrowe @ggreenway