WiP test: sim-time race repro and candidate fix

source/common/common/posix/thread_impl.cc

@@ -80,14 +80,19 @@ class ThreadImplPosix : public Thread {
 #endif
   }
 
-  ~ThreadImplPosix() override { ASSERT(joined_); }
+  ~ThreadImplPosix() override {
+    ENVOY_LOG_MISC(error, "joined_={}", joined_);
+    ASSERT(joined_);
+  }
 
   std::string name() const override { return name_; }
 
   // Thread::Thread
   void join() override {
     ASSERT(!joined_);
+    ENVOY_LOG_MISC(error, "Setting joined to true");
     joined_ = true;
+    ENVOY_LOG_MISC(error, "Setting joined_={}", joined_);
     const int rc = pthread_join(thread_handle_, nullptr);
     RELEASE_ASSERT(rc == 0, "");
   }

test/test_common/simulated_time_system.cc

@@ -49,25 +49,52 @@ class UnlockGuard {
 
 // Our simulated alarm inherits from TimerImpl so that the same dispatching
 // mechanism used in RealTimeSystem timers is employed for simulated alarms.
-class SimulatedTimeSystemHelper::Alarm : public Timer {
+class SimulatedTimeSystemHelper::Alarm {
 public:
   Alarm(SimulatedScheduler& simulated_scheduler, SimulatedTimeSystemHelper& time_system,
         CallbackScheduler& cb_scheduler, TimerCb cb)
       : cb_(cb_scheduler.createSchedulableCallback([this, cb] { runAlarm(cb); })),
-        simulated_scheduler_(simulated_scheduler), time_system_(time_system), armed_(false),
-        pending_(false) {}
+        simulated_scheduler_(simulated_scheduler), time_system_(time_system) {}
 
-  ~Alarm() override;
+  ~Alarm();
 
-  // Timer
-  void disableTimer() override;
+  struct ScopedBusy {
+    ScopedBusy(Alarm& alarm) : alarm_(alarm) {
+      ASSERT(!alarm_.busy_);
+      alarm_.busy_ = true;
+    }
+    ~ScopedBusy() {
+      ASSERT(alarm_.busy_);
+      alarm_.busy_ = false;
+      if (alarm_.delete_when_idle_) {
+        delete &alarm_;
+      }
+    }
+    Alarm& alarm_;
+  };
+
+  void release() {
+    absl::MutexLock lock(&time_system_.mutex_);
+    ASSERT(!delete_when_idle_);
+    if (armed_) {
+      cb_->cancel();
+      disableTimerLockHeld();
+    }
+    if (busy_) {
+      delete_when_idle_ = true;
+    } else {
+      delete this;
+    }
+  }
+
+  void disableTimer();
   void enableTimer(const std::chrono::milliseconds& duration,
-                   const ScopeTrackedObject* scope) override {
+                   const ScopeTrackedObject* scope) {
     enableHRTimer(duration, scope);
-  };
+  }
   void enableHRTimer(const std::chrono::microseconds& duration,
-                     const ScopeTrackedObject* scope) override;
-  bool enabled() override {
+                     const ScopeTrackedObject* scope);
+  bool enabled() {
     absl::MutexLock lock(&time_system_.mutex_);
     return armed_ || cb_->enabled();
   }
@@ -79,6 +106,9 @@ class SimulatedTimeSystemHelper::Alarm : public Timer {
    * typically via Dispatcher::run().
    */
   void activateLockHeld() ABSL_EXCLUSIVE_LOCKS_REQUIRED(time_system_.mutex_) {
+    if (delete_when_idle_) {
+      return;
+    }
     ASSERT(armed_);
     armed_ = false;
     if (pending_) {
@@ -92,8 +122,17 @@ class SimulatedTimeSystemHelper::Alarm : public Timer {
     // https://github.com/libevent/libevent/blob/29cc8386a2f7911eaa9336692a2c5544d8b4734f/event.c#L1917
     // See class comment for UnlockGuard for details on saving
     // time_system_.mutex_ prior to running libevent, which may delete this.
-    UnlockGuard unlocker(time_system_.mutex_);
-    cb_->scheduleCallbackCurrentIteration();
+    ASSERT(!busy_);
+    busy_ = true;
+    {
+      UnlockGuard unlocker(time_system_.mutex_);
+      cb_->scheduleCallbackCurrentIteration();
+    }
+    ASSERT(busy_);
+    busy_ = false;
+    if (delete_when_idle_) {
+      delete this;
+    }
   }
 
   SimulatedTimeSystemHelper& timeSystem() { return time_system_; }
@@ -113,8 +152,33 @@ class SimulatedTimeSystemHelper::Alarm : public Timer {
   SchedulableCallbackPtr cb_;
   SimulatedScheduler& simulated_scheduler_;
   SimulatedTimeSystemHelper& time_system_;
-  bool armed_ ABSL_GUARDED_BY(time_system_.mutex_);
-  bool pending_ ABSL_GUARDED_BY(time_system_.mutex_);
+  bool armed_ ABSL_GUARDED_BY(time_system_.mutex_) = false;
+  bool pending_ ABSL_GUARDED_BY(time_system_.mutex_) = false;
+  bool busy_ ABSL_GUARDED_BY(time_system_.mutex_) = false;
+  bool delete_when_idle_ ABSL_GUARDED_BY(time_system_.mutex_) = false;
+};
+
+// Our simulated alarm inherits from TimerImpl so that the same dispatching
+// mechanism used in RealTimeSystem timers is employed for simulated alarms.
+class SimulatedTimeSystemHelper::TimerImpl : public Timer {
+public:
+  explicit TimerImpl(Alarm& alarm) : alarm_(alarm) {}
+  ~TimerImpl() { alarm_.release(); }
+
+  // Timer
+  void disableTimer() override { alarm_.disableTimer(); }
+  void enableTimer(const std::chrono::milliseconds& duration,
+                   const ScopeTrackedObject* scope) override {
+    alarm_.enableTimer(duration, scope);
+  }
+  void enableHRTimer(const std::chrono::microseconds& duration,
+                     const ScopeTrackedObject* scope) override {
+    alarm_.enableHRTimer(duration, scope);
+  }
+  bool enabled() override { return alarm_.enabled(); }
+
+private:
+  Alarm& alarm_;
 };
 
 // Each timer is maintained and ordered by a common TimeSystem, but is
@@ -128,9 +192,9 @@ class SimulatedTimeSystemHelper::SimulatedScheduler : public Scheduler {
         schedule_ready_alarms_cb_(cb_scheduler.createSchedulableCallback(
             [this] { time_system_.scheduleReadyAlarms(); })) {}
   TimerPtr createTimer(const TimerCb& cb, Dispatcher& /*dispatcher*/) override {
-    return std::make_unique<SimulatedTimeSystemHelper::Alarm>(*this, time_system_, cb_scheduler_,
-                                                              cb);
-  };
+    return std::make_unique<SimulatedTimeSystemHelper::TimerImpl>(
+        *new Alarm(*this, time_system_, cb_scheduler_, cb));
+  }
   void scheduleReadyAlarms() { schedule_ready_alarms_cb_->scheduleCallbackNextIteration(); }
 
 private:
@@ -140,9 +204,6 @@ class SimulatedTimeSystemHelper::SimulatedScheduler : public Scheduler {
 };
 
 SimulatedTimeSystemHelper::Alarm::Alarm::~Alarm() {
-  if (armed_) {
-    disableTimer();
-  }
 }
 
 void SimulatedTimeSystemHelper::Alarm::Alarm::disableTimer() {
@@ -165,14 +226,14 @@ void SimulatedTimeSystemHelper::Alarm::Alarm::disableTimerLockHeld() {
 void SimulatedTimeSystemHelper::Alarm::Alarm::enableHRTimer(
     const std::chrono::microseconds& duration, const ScopeTrackedObject* /*scope*/) {
   if (duration.count() != 0) {
-    disableTimer();
+    cb_->cancel();
   }
   absl::MutexLock lock(&time_system_.mutex_);
   if (pending_) {
     // Calling enableTimer on a timer that is already pending is a no-op. Timer will still fire
     // based on the original time it was scheduled.
     return;
-  } else if (armed_) {
+  } else if (armed_ ) {
     disableTimerLockHeld();
   }
 
@@ -375,6 +436,7 @@ void SimulatedTimeSystemHelper::scheduleReadyAlarmsLockHeld() {
     }
 
     Alarm& alarm = alarm_registration.alarm_;
+    //Alarm::ScopedBusy busy(alarm);
     removeAlarmLockHeld(alarm);
     alarmActivateLockHeld(alarm);
   }

test/test_common/simulated_time_system.h

@@ -66,7 +66,10 @@ class SimulatedTimeSystemHelper : public TestTimeSystem {
 private:
   class SimulatedScheduler;
   class Alarm;
+  using AlarmSharedPtr = std::shared_ptr<Alarm>;
   friend class Alarm; // Needed to reference mutex for thread annotations.
+  class TimerImpl;
+  friend class TimerImpl;
   struct AlarmRegistration {
     AlarmRegistration(MonotonicTime time, uint64_t randomness, Alarm& alarm)
         : time_(time), randomness_(randomness), alarm_(alarm) {}
@@ -127,6 +130,7 @@ class SimulatedTimeSystemHelper : public TestTimeSystem {
   }
   void decPendingLockHeld() ABSL_EXCLUSIVE_LOCKS_REQUIRED(mutex_) { --pending_alarms_; }
   void waitForNoPendingLockHeld() const ABSL_EXCLUSIVE_LOCKS_REQUIRED(mutex_);
+  //void removeAlarm(Alarm&);
 
   RealTimeSource real_time_source_; // Used to initialize monotonic_time_ and system_time_;
   MonotonicTime monotonic_time_ ABSL_GUARDED_BY(mutex_);

test/test_common/simulated_time_system_test.cc

@@ -474,6 +474,20 @@ TEST_P(SimulatedTimeSystemTest, Enabled) {
   EXPECT_TRUE(timer->enabled());
 }
 
+TEST_P(SimulatedTimeSystemTest, DeleteTimerFromThread) {
+  //ENVOY_LOG_MISC(error, "1");
+  TimerPtr timer = scheduler_->createTimer([](){}, dispatcher_);
+  //ENVOY_LOG_MISC(error, "2");
+  timer->enableTimer(std::chrono::milliseconds(0));
+  //ENVOY_LOG_MISC(error, "3");
+  auto thread = Thread::threadFactoryForTest().createThread([&timer]() { timer.reset(); });
+  //ENVOY_LOG_MISC(error, "4");
+  advanceMsAndLoop(1);
+  ENVOY_LOG_MISC(error, "joining thread");
+  thread->join();
+  thread.reset();
+}
+
 } // namespace
 } // namespace Test
 } // namespace Event