hot restart: fix race condition when fetching parent stats

include/envoy/thread/thread.h

@@ -12,6 +12,7 @@ class BasicLockable {
   virtual ~BasicLockable() {}
 
   virtual void lock() PURE;
+  virtual bool try_lock() PURE;
   virtual void unlock() PURE;
 };
 

source/common/common/thread.h

@@ -35,6 +35,7 @@ typedef std::unique_ptr<Thread> ThreadPtr;
 class MutexBasicLockable : public BasicLockable {
 public:
   void lock() override { mutex_.lock(); }
+  bool try_lock() override { return mutex_.try_lock(); }
   void unlock() override { mutex_.unlock(); }
 
 private:

source/exe/hot_restart.cc

@@ -14,7 +14,7 @@ namespace Server {
 
 // Increment this whenever there is a shared memory / RPC change that will prevent a hot restart
 // from working. Operations code can then cope with this and do a full restart.
-const uint64_t SharedMemory::VERSION = 5;
+const uint64_t SharedMemory::VERSION = 6;
 
 SharedMemory& SharedMemory::initialize(Options& options) {
   int flags = O_RDWR;
@@ -48,6 +48,7 @@ SharedMemory& SharedMemory::initialize(Options& options) {
     shmem->initializeMutex(shmem->log_lock_);
     shmem->initializeMutex(shmem->access_log_lock_);
     shmem->initializeMutex(shmem->stat_lock_);
+    shmem->initializeMutex(shmem->init_lock_);
   } else {
     RELEASE_ASSERT(shmem->size_ == sizeof(SharedMemory));
     RELEASE_ASSERT(shmem->version_ == VERSION);
@@ -78,7 +79,8 @@ std::string SharedMemory::version() { return fmt::format("{}.{}", VERSION, sizeo
 
 HotRestartImpl::HotRestartImpl(Options& options)
     : options_(options), shmem_(SharedMemory::initialize(options)), log_lock_(shmem_.log_lock_),
-      access_log_lock_(shmem_.access_log_lock_), stat_lock_(shmem_.stat_lock_) {
+      access_log_lock_(shmem_.access_log_lock_), stat_lock_(shmem_.stat_lock_),
+      init_lock_(shmem_.init_lock_) {
 
   my_domain_socket_ = bindDomainSocket(options.restartEpoch());
   child_address_ = createDomainSocketAddress((options.restartEpoch() + 1));
@@ -176,8 +178,27 @@ int HotRestartImpl::duplicateParentListenSocket(uint32_t port) {
 }
 
 void HotRestartImpl::getParentStats(GetParentStatsInfo& info) {
+  // There exists a race condition during hot restart involving fetching parent stats. It looks like
+  // this:
+  // 1) There currently exist 2 Envoy processes (draining has not completed): P0 and P1.
+  // 2) New process (P2) comes up and passes the INITIALIZING check.
+  // 3) P2 proceeds to the parent admin shutdown phase.
+  // 4) This races with P1 fetching parent stats from P0.
+  // 5) Calling receiveTypedRpc() below picks up the wrong message.
+  //
+  // There are not any great solutions to this problem. We could potentially guard this using flags,
+  // but this is a legitimate race condition even under normal restart conditions, so exiting P2
+  // with an error is not great. We could also rework all of this code so that P0<->P1 and P1<->P2
+  // communication occur over different socket pairs. This could work, but is a large change. We
+  // could also potentially use connection oriented sockets and accept connections from our child,
+  // and connect to our parent, but again, this becomes complicated.
+  //
+  // Instead, we guard this condition with a lock. However, to avoid deadlock, we must try_lock()
+  // in this path, since this call runs in the same thread as the event loop that is receiving
+  // messages. If try_lock() fails it is sufficient to not return any parent stats.
+  std::unique_lock<Thread::BasicLockable> lock(init_lock_, std::defer_lock);
   memset(&info, 0, sizeof(info));
-  if (options_.restartEpoch() == 0 || parent_terminated_) {
+  if (options_.restartEpoch() == 0 || parent_terminated_ || !lock.try_lock()) {
     return;
   }
 
@@ -362,6 +383,8 @@ void HotRestartImpl::onSocketEvent() {
 }
 
 void HotRestartImpl::shutdownParentAdmin(ShutdownParentAdminInfo& info) {
+  // See large comment in getParentStats() on why this operation is locked.
+  std::unique_lock<Thread::BasicLockable> lock(init_lock_);
   if (options_.restartEpoch() == 0) {
     return;
   }

source/exe/hot_restart.h

@@ -45,6 +45,7 @@ class SharedMemory {
   pthread_mutex_t log_lock_;
   pthread_mutex_t access_log_lock_;
   pthread_mutex_t stat_lock_;
+  pthread_mutex_t init_lock_;
   std::array<Stats::RawStatData, 16384> stats_slots_;
 
   friend class HotRestartImpl;
@@ -68,6 +69,20 @@ class ProcessSharedMutex : public Thread::BasicLockable {
     }
   }
 
+  bool try_lock() override {
+    int rc = pthread_mutex_trylock(&mutex_);
+    if (rc == EBUSY) {
+      return false;
+    }
+
+    ASSERT(rc == 0 || rc == EOWNERDEAD);
+    if (rc == EOWNERDEAD) {
+      pthread_mutex_consistent(&mutex_);
+    }
+
+    return true;
+  }
+
   void unlock() override {
     int rc = pthread_mutex_unlock(&mutex_);
     ASSERT(rc == 0);
@@ -169,6 +184,7 @@ class HotRestartImpl : public HotRestart,
   ProcessSharedMutex log_lock_;
   ProcessSharedMutex access_log_lock_;
   ProcessSharedMutex stat_lock_;
+  ProcessSharedMutex init_lock_;
   int my_domain_socket_{-1};
   sockaddr_un parent_address_;
   sockaddr_un child_address_;