diff --git a/source/common/common/posix/thread_impl.cc b/source/common/common/posix/thread_impl.cc index 71cbf2b02eb6b..448c765eceb50 100644 --- a/source/common/common/posix/thread_impl.cc +++ b/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, ""); } diff --git a/test/test_common/simulated_time_system.cc b/test/test_common/simulated_time_system.cc index d361beddf4711..4c3366e3cd293 100644 --- a/test/test_common/simulated_time_system.cc +++ b/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(*this, time_system_, cb_scheduler_, - cb); - }; + return std::make_unique( + *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); } diff --git a/test/test_common/simulated_time_system.h b/test/test_common/simulated_time_system.h index e8a369e4f9cc9..16bb728a6565c 100644 --- a/test/test_common/simulated_time_system.h +++ b/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; 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_); diff --git a/test/test_common/simulated_time_system_test.cc b/test/test_common/simulated_time_system_test.cc index 13a435148aff3..fdeca7277ca06 100644 --- a/test/test_common/simulated_time_system_test.cc +++ b/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