fix pr comments by greg

Signed-off-by: Sotiris Nanopoulos <[email protected]>
envoyproxy · Dec 5, 2020 · 30bbdfa · 30bbdfa
1 parent 7550099
commit 30bbdfa
Show file tree

Hide file tree

Showing 5 changed files with 18 additions and 17 deletions.
diff --git 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
@@ -11,23 +11,23 @@
 
 namespace Envoy {
 
-static volatile bool shutdown_pending = false;
+static std::atomic<bool> shutdown_pending = false;
 
 BOOL WINAPI CtrlHandler(DWORD fdwCtrlType) {
   if (shutdown_pending) {
     return 0;
   }
   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
@@ -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
@@ -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
@@ -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);