Merge branch 'fix-signal-handling'

contrib/nghttpx-logrotate

@@ -6,6 +6,6 @@
   delaycompress
   notifempty
   postrotate
-    killall -USR1 nghttpx 2> /dev/null || true
+    [ -s /var/run/nghttpx.pid ] && kill -USR1 `cat /var/run/nghttpx.pid` 2> /dev/null || true
   endscript
 }

doc/nghttpx.h2r

@@ -49,6 +49,10 @@ SIGUSR2
   the latter.  The former is called master process, and the latter is
   called worker process.  The above signal must be sent to the master
   process.  If the worker process receives one of them, it is ignored.
+  This behaviour of worker process may change in the future release.
+  In other words, in the future release, worker process may terminate
+  upon the reception of these signals.  Therefore these signals should
+  not be sent to the worker process.
 
 SERVER PUSH
 -----------

integration-tests/server_tester.go

@@ -21,6 +21,7 @@ import (
 	"sort"
 	"strconv"
 	"strings"
+	"syscall"
 	"testing"
 	"time"
 )
@@ -203,10 +204,20 @@ func (st *serverTester) Close() {
 		st.conn.Close()
 	}
 	if st.cmd != nil {
-		st.cmd.Process.Kill()
+		done := make(chan struct{})
-		st.cmd.Wait()
+		go func() {
-		// workaround to unreliable Process.Signal()
+			st.cmd.Wait()
-		time.Sleep(150 * time.Millisecond)
+			done <- struct{}{}
+		}()
+
+		st.cmd.Process.Signal(syscall.SIGQUIT)
+
+		select {
+		case <-done:
+		case <-time.After(10 * time.Second):
+			st.cmd.Process.Kill()
+			<-done
+		}
 	}
 	if st.ts != nil {
 		st.ts.Close()

src/Makefile.am

@@ -127,6 +127,7 @@ NGHTTPX_SRCS = \
 	shrpx_memcached_result.h \
 	shrpx_worker_process.cc shrpx_worker_process.h \
 	shrpx_process.h \
+	shrpx_signal.cc shrpx_signal.h \
 	buffer.h memchunk.h template.h
 
 if HAVE_SPDYLAY

src/shrpx.cc

@@ -81,6 +81,7 @@
 #include "shrpx_http2_session.h"
 #include "shrpx_worker_process.h"
 #include "shrpx_process.h"
+#include "shrpx_signal.h"
 #include "util.h"
 #include "app_helper.h"
 #include "ssl.h"
@@ -210,26 +211,55 @@ void save_pid() {
 
 namespace {
 void exec_binary(SignalServer *ssv) {
+  int rv;
+  sigset_t oldset;
+
   LOG(NOTICE) << "Executing new binary";
 
-  auto pid = fork();
+  rv = shrpx_signal_block_all(&oldset);
-
+  if (rv != 0) {
-  if (pid == -1) {
     auto error = errno;
-    LOG(ERROR) << "fork() failed errno=" << error;
+    LOG(ERROR) << "Blocking all signals failed: " << strerror(error);
+
     return;
   }
 
+  auto pid = fork();
+
   if (pid != 0) {
+    if (pid == -1) {
+      auto error = errno;
+      LOG(ERROR) << "fork() failed errno=" << error;
+    }
+
+    rv = shrpx_signal_set(&oldset);
+
+    if (rv != 0) {
+      auto error = errno;
+      LOG(FATAL) << "Restoring signal mask failed: " << strerror(error);
+
+      exit(EXIT_FAILURE);
+    }
+
     return;
   }
 
+  shrpx_signal_unset_master_proc_ign_handler();
+
+  rv = shrpx_signal_unblock_all();
+  if (rv != 0) {
+    auto error = errno;
+    LOG(ERROR) << "Unblocking all signals failed: " << strerror(error);
+
+    exit(EXIT_FAILURE);
+  }
+
   auto exec_path = util::get_exec_path(get_config()->argc, get_config()->argv,
                                        get_config()->cwd);
 
   if (!exec_path) {
     LOG(ERROR) << "Could not resolve the executable path";
-    return;
+    exit(EXIT_FAILURE);
   }
 
   auto argv = make_unique<char *[]>(get_config()->argc + 1);
@@ -306,7 +336,7 @@ void exec_binary(SignalServer *ssv) {
   if (execve(argv[0], argv.get(), envp.get()) == -1) {
     auto error = errno;
     LOG(ERROR) << "execve failed: errno=" << error;
-    _Exit(EXIT_FAILURE);
+    exit(EXIT_FAILURE);
   }
 }
 } // namespace
@@ -647,20 +677,59 @@ void close_env_fd(std::initializer_list<const char *> envnames) {
 namespace {
 pid_t fork_worker_process(SignalServer *ssv) {
   int rv;
-  auto pid = fork();
+  sigset_t oldset;
+
+  rv = shrpx_signal_block_all(&oldset);
+  if (rv != 0) {
+    auto error = errno;
+    LOG(ERROR) << "Blocking all signals failed: " << strerror(error);
 
-  if (pid == -1) {
     return -1;
   }
 
+  auto pid = fork();
+
   if (pid == 0) {
+    ev_loop_fork(EV_DEFAULT);
+
+    shrpx_signal_set_worker_proc_ign_handler();
+
+    rv = shrpx_signal_unblock_all();
+    if (rv != 0) {
+      auto error = errno;
+      LOG(FATAL) << "Unblocking all signals failed: " << strerror(error);
+
+      exit(EXIT_FAILURE);
+    }
+
     close(ssv->ipc_fd[1]);
     WorkerProcessConfig wpconf{ssv->ipc_fd[0], ssv->server_fd, ssv->server_fd6};
     rv = worker_process_event_loop(&wpconf);
     if (rv != 0) {
-      LOG(ERROR) << "Worker process returned error";
+      LOG(FATAL) << "Worker process returned error";
+
+      exit(EXIT_FAILURE);
     }
-    return 0;
+
+    exit(EXIT_SUCCESS);
+  }
+
+  // parent process
+  if (pid == -1) {
+    auto error = errno;
+    LOG(ERROR) << "Could not spawn worker process: " << strerror(error);
+  }
+
+  rv = shrpx_signal_set(&oldset);
+  if (rv != 0) {
+    auto error = errno;
+    LOG(FATAL) << "Restoring signal mask failed: " << strerror(error);
+
+    exit(EXIT_FAILURE);
+  }
+
+  if (pid == -1) {
+    return -1;
   }
 
   close(ssv->ipc_fd[0]);
@@ -675,6 +744,8 @@ namespace {
 int event_loop() {
   int rv;
 
+  shrpx_signal_set_master_proc_ign_handler();
+
   if (get_config()->daemon) {
     if (call_daemon() == -1) {
       auto error = errno;
@@ -710,8 +781,6 @@ int event_loop() {
     util::make_socket_closeonexec(fd);
   }
 
-  auto loop = EV_DEFAULT;
-
   if (get_config()->host_unix) {
     close_env_fd({ENV_LISTENER4_FD, ENV_LISTENER6_FD});
     auto fd = create_unix_domain_server_socket();
@@ -746,19 +815,17 @@ int event_loop() {
     ssv.server_fd6 = fd6;
   }
 
+  auto loop = EV_DEFAULT;
+
   auto pid = fork_worker_process(&ssv);
 
-  switch (pid) {
+  if (pid == -1) {
-  case -1:
     return -1;
-  case 0:
-    // worker process (child)
-    return 0;
   }
 
   ssv.worker_process_pid = pid;
 
-  auto signals =
+  constexpr auto signals =
       std::array<int, 5>{{REOPEN_LOG_SIGNAL, EXEC_BINARY_SIGNAL,
                           GRACEFUL_SHUTDOWN_SIGNAL, SIGINT, SIGTERM}};
   auto sigevs = std::array<ev_signal, signals.size()>();
@@ -2444,10 +2511,6 @@ int main(int argc, char **argv) {
     reset_timer();
   }
 
-  struct sigaction act {};
-  act.sa_handler = SIG_IGN;
-  sigaction(SIGPIPE, &act, nullptr);
-
   if (event_loop() != 0) {
     return -1;
   }

src/shrpx_connection_handler.cc

@@ -43,6 +43,7 @@
 #include "shrpx_downstream_connection.h"
 #include "shrpx_accept_handler.h"
 #include "shrpx_memcached_dispatcher.h"
+#include "shrpx_signal.h"
 #include "util.h"
 #include "template.h"
 
@@ -432,30 +433,62 @@ int ConnectionHandler::start_ocsp_update(const char *cert_file) {
     }
   });
 
-  auto pid = fork();
+  sigset_t oldset;
-  if (pid == -1) {
+
+  rv = shrpx_signal_block_all(&oldset);
+  if (rv != 0) {
     auto error = errno;
-    LOG(WARN) << "Could not execute ocsp query command for " << cert_file
+    LOG(ERROR) << "Blocking all signals failed: " << strerror(error);
-              << ": " << argv[0] << ", fork() failed, errno=" << error;
+
     return -1;
   }
 
+  auto pid = fork();
+
   if (pid == 0) {
     // child process
+    shrpx_signal_unset_worker_proc_ign_handler();
+
+    rv = shrpx_signal_unblock_all();
+    if (rv != 0) {
+      auto error = errno;
+      LOG(FATAL) << "Unblocking all signals failed: " << strerror(error);
+
+      exit(EXIT_FAILURE);
+    }
+
     dup2(pfd[1], 1);
     close(pfd[0]);
 
     rv = execve(argv[0], argv, envp);
     if (rv == -1) {
       auto error = errno;
-      LOG(WARN) << "Could not execute ocsp query command: " << argv[0]
+      LOG(ERROR) << "Could not execute ocsp query command: " << argv[0]
-                << ", execve() faild, errno=" << error;
+                 << ", execve() faild, errno=" << error;
-      _Exit(EXIT_FAILURE);
+      exit(EXIT_FAILURE);
     }
     // unreachable
   }
 
   // parent process
+  if (pid == -1) {
+    auto error = errno;
+    LOG(ERROR) << "Could not execute ocsp query command for " << cert_file
+               << ": " << argv[0] << ", fork() failed, errno=" << error;
+  }
+
+  rv = shrpx_signal_set(&oldset);
+  if (rv != 0) {
+    auto error = errno;
+    LOG(FATAL) << "Restoring all signals failed: " << strerror(error);
+
+    exit(EXIT_FAILURE);
+  }
+
+  if (pid == -1) {
+    return -1;
+  }
+
   close(pfd[1]);
   pfd[1] = -1;
 

src/shrpx_process.h

@@ -27,17 +27,11 @@
 
 #include "shrpx.h"
 
-#include <signal.h>
-
 namespace shrpx {
 
 constexpr uint8_t SHRPX_IPC_REOPEN_LOG = 1;
 constexpr uint8_t SHRPX_IPC_GRACEFUL_SHUTDOWN = 2;
 
-constexpr int REOPEN_LOG_SIGNAL = SIGUSR1;
-constexpr int EXEC_BINARY_SIGNAL = SIGUSR2;
-constexpr int GRACEFUL_SHUTDOWN_SIGNAL = SIGQUIT;
-
 } // namespace shrpx
 
 #endif // SHRPX_PROCESS_H

src/shrpx_signal.cc

@@ -0,0 +1,129 @@
+/*
+ * nghttp2 - HTTP/2 C Library
+ *
+ * Copyright (c) 2015 Tatsuhiro Tsujikawa
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining
+ * a copy of this software and associated documentation files (the
+ * "Software"), to deal in the Software without restriction, including
+ * without limitation the rights to use, copy, modify, merge, publish,
+ * distribute, sublicense, and/or sell copies of the Software, and to
+ * permit persons to whom the Software is furnished to do so, subject to
+ * the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be
+ * included in all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
+ * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+ * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+ * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE
+ * LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION
+ * OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION
+ * WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
+ */
+#include "shrpx_signal.h"
+
+#include <cerrno>
+
+#include "template.h"
+
+using namespace nghttp2;
+
+namespace shrpx {
+
+int shrpx_signal_block_all(sigset_t *oldset) {
+  sigset_t newset;
+  int rv;
+
+  sigfillset(&newset);
+
+#ifndef NOTHREADS
+  rv = pthread_sigmask(SIG_SETMASK, &newset, oldset);
+
+  if (rv != 0) {
+    errno = rv;
+    return -1;
+  }
+
+  return 0;
+#else  // NOTHREADS
+  return sigprocmask(SIG_SETMASK, &newset, &oldset);
+#endif // NOTHREADS
+}
+
+int shrpx_signal_unblock_all() {
+  sigset_t newset;
+  int rv;
+
+  sigemptyset(&newset);
+
+#ifndef NOTHREADS
+  rv = pthread_sigmask(SIG_SETMASK, &newset, nullptr);
+
+  if (rv != 0) {
+    errno = rv;
+    return -1;
+  }
+
+  return 0;
+#else  // NOTHREADS
+  return sigprocmask(SIG_SETMASK, &newset, nullptr);
+#endif // NOTHREADS
+}
+
+int shrpx_signal_set(sigset_t *set) {
+  int rv;
+
+#ifndef NOTHREADS
+  rv = pthread_sigmask(SIG_SETMASK, set, nullptr);
+
+  if (rv != 0) {
+    errno = rv;
+    return -1;
+  }
+
+  return 0;
+#else  // NOTHREADS
+  return sigprocmask(SIG_SETMASK, set, nullptr);
+#endif // NOTHREADS
+}
+
+namespace {
+template <typename Signals>
+void signal_set_handler(void (*handler)(int), Signals &&sigs) {
+  struct sigaction act {};
+  act.sa_handler = handler;
+  sigemptyset(&act.sa_mask);
+  for (auto sig : sigs) {
+    sigaction(sig, &act, nullptr);
+  }
+}
+} // namespace
+
+namespace {
+constexpr auto master_proc_ign_signals = std::array<int, 1>{{SIGPIPE}};
+} // namespace
+
+namespace {
+constexpr auto worker_proc_ign_signals = std::array<int, 4>{
+    {REOPEN_LOG_SIGNAL, EXEC_BINARY_SIGNAL, GRACEFUL_SHUTDOWN_SIGNAL, SIGPIPE}};
+} // namespace
+
+void shrpx_signal_set_master_proc_ign_handler() {
+  signal_set_handler(SIG_IGN, master_proc_ign_signals);
+}
+
+void shrpx_signal_unset_master_proc_ign_handler() {
+  signal_set_handler(SIG_DFL, master_proc_ign_signals);
+}
+
+void shrpx_signal_set_worker_proc_ign_handler() {
+  signal_set_handler(SIG_IGN, worker_proc_ign_signals);
+}
+
+void shrpx_signal_unset_worker_proc_ign_handler() {
+  signal_set_handler(SIG_DFL, worker_proc_ign_signals);
+}
+
+} // namespace shrpx

src/shrpx_signal.h

@@ -0,0 +1,59 @@
+/*
+ * nghttp2 - HTTP/2 C Library
+ *
+ * Copyright (c) 2015 Tatsuhiro Tsujikawa
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining
+ * a copy of this software and associated documentation files (the
+ * "Software"), to deal in the Software without restriction, including
+ * without limitation the rights to use, copy, modify, merge, publish,
+ * distribute, sublicense, and/or sell copies of the Software, and to
+ * permit persons to whom the Software is furnished to do so, subject to
+ * the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be
+ * included in all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
+ * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+ * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+ * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE
+ * LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION
+ * OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION
+ * WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
+ */
+#ifndef SHRPX_SIGNAL_H
+#define SHRPX_SIGNAL_H
+
+#include "shrpx.h"
+
+#include <signal.h>
+
+namespace shrpx {
+
+constexpr int REOPEN_LOG_SIGNAL = SIGUSR1;
+constexpr int EXEC_BINARY_SIGNAL = SIGUSR2;
+constexpr int GRACEFUL_SHUTDOWN_SIGNAL = SIGQUIT;
+
+// Blocks all signals.  The previous signal mask is stored into
+// |oldset| if it is not nullptr.  This function returns 0 if it
+// succeeds, or -1.  The errno will indicate the error.
+int shrpx_signal_block_all(sigset_t *oldset);
+
+// Unblocks all signals.  This function returns 0 if it succeeds, or
+// -1.  The errno will indicate the error.
+int shrpx_signal_unblock_all();
+
+// Sets signal mask |set|.  This function returns 0 if it succeeds, or
+// -1.  The errno will indicate the error.
+int shrpx_signal_set(sigset_t *set);
+
+void shrpx_signal_set_master_proc_ign_handler();
+void shrpx_signal_unset_master_proc_ign_handler();
+
+void shrpx_signal_set_worker_proc_ign_handler();
+void shrpx_signal_unset_worker_proc_ign_handler();
+
+} // namespace shrpx
+
+#endif // SHRPX_SIGNAL_H

src/shrpx_worker_process.cc

@@ -422,23 +422,36 @@ int worker_process_event_loop(WorkerProcessConfig *wpconf) {
 
   int rv;
 
-  // It is good to ignore these control signals since user may use
-  // "killall .. nghttpx".  We don't want catch this signal in worker
-  // process.
-  struct sigaction act {};
-  act.sa_handler = SIG_IGN;
-  sigaction(REOPEN_LOG_SIGNAL, &act, nullptr);
-  sigaction(EXEC_BINARY_SIGNAL, &act, nullptr);
-  sigaction(GRACEFUL_SHUTDOWN_SIGNAL, &act, nullptr);
-
   if (get_config()->num_worker == 1) {
     rv = conn_handler.create_single_worker();
+    if (rv != 0) {
+      return -1;
+    }
   } else {
-    rv = conn_handler.create_worker_thread(get_config()->num_worker);
+#ifndef NOTHREADS
-  }
+    sigset_t set;
+    sigemptyset(&set);
+    sigaddset(&set, SIGCHLD);
 
-  if (rv != 0) {
+    rv = pthread_sigmask(SIG_BLOCK, &set, nullptr);
-    return -1;
+    if (rv != 0) {
+      LOG(ERROR) << "Blocking SIGCHLD failed: " << strerror(rv);
+      return -1;
+    }
+#endif // !NOTHREADS
+
+    rv = conn_handler.create_worker_thread(get_config()->num_worker);
+    if (rv != 0) {
+      return -1;
+    }
+
+#ifndef NOTHREADS
+    rv = pthread_sigmask(SIG_UNBLOCK, &set, nullptr);
+    if (rv != 0) {
+      LOG(ERROR) << "Unblocking SIGCHLD failed: " << strerror(rv);
+      return -1;
+    }
+#endif // !NOTHREADS
   }
 
   drop_privileges();