From 30bbdfa9c6a57e9b792bdc78a1b680246c88dba8 Mon Sep 17 00:00:00 2001
From: Sotiris Nanopoulos <sonanopo@microsoft.com>
Date: Fri, 4 Dec 2020 16:37:58 -0800
Subject: [PATCH] fix pr comments by greg

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>
---
 include/envoy/common/platform.h       |  4 +++-
 source/exe/win32/platform_impl.cc     | 12 ++++++------
 source/server/server.cc               | 16 +++++++---------
 test/exe/BUILD                        |  1 +
 test/exe/win32_outofproc_main_test.cc |  2 +-
 5 files changed, 18 insertions(+), 17 deletions(-)

diff --git a/include/envoy/common/platform.h b/include/envoy/common/platform.h
index 3a1c906e231e..ceb6c8b220e7 100644
--- a/include/envoy/common/platform.h
+++ b/include/envoy/common/platform.h
@@ -153,7 +153,7 @@ struct msghdr {
 #define HANDLE_ERROR_INVALID ERROR_INVALID_HANDLE
 
 #define ENVOY_WIN32_SIGNAL_COUNT 1
-#define ENVOY_WIN32_SIGTERM 0
+#define ENVOY_SIGTERM 0
 
 namespace Platform {
 constexpr absl::string_view null_device_path{"NUL"};
@@ -250,6 +250,8 @@ typedef int signal_t;           // NOLINT(modernize-use-using)
 #define HANDLE_ERROR_PERM EACCES
 #define HANDLE_ERROR_INVALID EBADF
 
+#define ENVOY_SIGTERM SIGTERM
+
 namespace Platform {
 constexpr absl::string_view null_device_path{"/dev/null"};
 }
diff --git a/source/exe/win32/platform_impl.cc b/source/exe/win32/platform_impl.cc
index 5c91f1a1a789..ee729b5a167b 100644
--- a/source/exe/win32/platform_impl.cc
+++ b/source/exe/win32/platform_impl.cc
@@ -11,7 +11,7 @@
 
 namespace Envoy {
 
-static volatile bool shutdown_pending = false;
+static std::atomic<bool> shutdown_pending = false;
 
 BOOL WINAPI CtrlHandler(DWORD fdwCtrlType) {
   if (shutdown_pending) {
@@ -19,15 +19,15 @@ BOOL WINAPI CtrlHandler(DWORD fdwCtrlType) {
   }
   shutdown_pending = true;
 
-  auto handler = Event::eventBridgeHandlersSingleton::get()[ENVOY_WIN32_SIGTERM];
+  auto handler = Event::eventBridgeHandlersSingleton::get()[ENVOY_SIGTERM];
   if (!handler) {
     return 0;
   }
 
-  // This code is executed as part of a thread running under a Windows
-  // context. For that reason we want to avoid allocating memory or
-  // taking locks. This is why we use do not want to just write to a socket
-  // to wake up the signal handler.
+  // This code is executed as part of a thread running under a thread owned and
+  // managed by Windows console host. For that reason we want to avoid allocating
+  // substantial amount of memory or taking locks.
+  // This is why we write to a socket to wake up the signal handler.
   Buffer::OwnedImpl buffer;
   constexpr absl::string_view data{"a"};
   buffer.add(data);
diff --git a/source/server/server.cc b/source/server/server.cc
index c86f4b6d9d3a..ffe3bf0c7c96 100644
--- a/source/server/server.cc
+++ b/source/server/server.cc
@@ -643,18 +643,16 @@ RunHelper::RunHelper(Instance& instance, const Options& options, Event::Dispatch
         }
       }) {
   // Setup signals.
+  // Since signals are not supported on Windows we have an internal definition for `SIGTERM`
+  // On POSIX it resolves as expected to SIGTERM
+  // On Windows we use it internaly for all the console events that indicate that we should
+  // terminate the process.
   if (options.signalHandlingEnabled()) {
-#ifdef WIN32
-    sigterm_ = dispatcher.listenForSignal(ENVOY_WIN32_SIGTERM, [&instance]() {
-      ENVOY_LOG(warn, "caught ENVOY_WIN32_SIGTERM");
+    sigterm_ = dispatcher.listenForSignal(ENVOY_SIGTERM, [&instance]() {
+      ENVOY_LOG(warn, "caught ENVOY_SIGTERM");
       instance.shutdown();
     });
-#else
-    sigterm_ = dispatcher.listenForSignal(SIGTERM, [&instance]() {
-      ENVOY_LOG(warn, "caught SIGTERM");
-      instance.shutdown();
-    });
-
+#ifndef WIN32
     sigint_ = dispatcher.listenForSignal(SIGINT, [&instance]() {
       ENVOY_LOG(warn, "caught SIGINT");
       instance.shutdown();
diff --git a/test/exe/BUILD b/test/exe/BUILD
index 1656b7f2c1e5..e81ade64cb75 100644
--- a/test/exe/BUILD
+++ b/test/exe/BUILD
@@ -93,6 +93,7 @@ envoy_cc_test(
         "//source/exe:envoy-static",
         "//test/config/integration:google_com_proxy_port_0",
     ],
+    # TODO(envoyproxy/windows-dev): Disable the manual tag.
     tags = ["manual"],
     deps = [
         "//source/common/api:api_lib",
diff --git a/test/exe/win32_outofproc_main_test.cc b/test/exe/win32_outofproc_main_test.cc
index c3dc7c8db9fa..529d77fa758b 100644
--- a/test/exe/win32_outofproc_main_test.cc
+++ b/test/exe/win32_outofproc_main_test.cc
@@ -80,7 +80,7 @@ TEST_P(MainCommonTest, EnvoyHandlesCtrlBreakEvent) {
   EXPECT_TRUE(total_sleep <= resonable_sleep_time);
   auto output = TestEnvironment::readFileToStringForTest(log_path_);
   size_t count = 0;
-  for (size_t pos = 0; (pos = output.find("ENVOY_WIN32_SIGTERM", pos)) != std::string::npos;
+  for (size_t pos = 0; (pos = output.find("ENVOY_SIGTERM", pos)) != std::string::npos;
        ++pos, ++count) {
   }
   EXPECT_EQ(1, count);