Fix a bug with multiple interleaved events in SimulatedEventLoop
The new test used to segfault, and now it works correctly.
Change-Id: I2dd35ee637f42d8ff926c508fa3872227fe6cdab
diff --git a/aos/events/event_loop.cc b/aos/events/event_loop.cc
index da89d9b..e69fd0d 100644
--- a/aos/events/event_loop.cc
+++ b/aos/events/event_loop.cc
@@ -442,12 +442,20 @@
namespace {
bool CompareEvents(const EventLoopEvent *first, const EventLoopEvent *second) {
- return first->event_time() > second->event_time();
+ if (first->event_time() > second->event_time()) {
+ return true;
+ }
+ if (first->event_time() < second->event_time()) {
+ return false;
+ }
+ return first->generation() > second->generation();
}
} // namespace
void EventLoop::AddEvent(EventLoopEvent *event) {
DCHECK(std::find(events_.begin(), events_.end(), event) == events_.end());
+ DCHECK(event->generation() == 0);
+ event->set_generation(++event_generation_);
events_.push_back(event);
std::push_heap(events_.begin(), events_.end(), CompareEvents);
}
@@ -455,6 +463,7 @@
void EventLoop::RemoveEvent(EventLoopEvent *event) {
auto e = std::find(events_.begin(), events_.end(), event);
if (e != events_.end()) {
+ DCHECK(event->generation() != 0);
events_.erase(e);
std::make_heap(events_.begin(), events_.end(), CompareEvents);
event->Invalidate();
diff --git a/aos/events/event_loop.h b/aos/events/event_loop.h
index c36b195..81dfe42 100644
--- a/aos/events/event_loop.h
+++ b/aos/events/event_loop.h
@@ -676,6 +676,7 @@
void ReserveEvents();
std::vector<EventLoopEvent *> events_;
+ size_t event_generation_ = 1;
// If true, don't send AOS_LOG to /aos
bool skip_logger_ = false;
diff --git a/aos/events/event_loop_event.h b/aos/events/event_loop_event.h
index 6438da1..808a4fe 100644
--- a/aos/events/event_loop_event.h
+++ b/aos/events/event_loop_event.h
@@ -12,21 +12,31 @@
virtual ~EventLoopEvent() {}
bool valid() const { return event_time_ != monotonic_clock::max_time; }
- void Invalidate() { event_time_ = monotonic_clock::max_time; }
+ void Invalidate() {
+ event_time_ = monotonic_clock::max_time;
+ generation_ = 0;
+ }
monotonic_clock::time_point event_time() const {
DCHECK(valid());
return event_time_;
}
-
- virtual void HandleEvent() = 0;
-
void set_event_time(monotonic_clock::time_point event_time) {
event_time_ = event_time;
}
+ // Internal book-keeping for EventLoop.
+ size_t generation() const {
+ DCHECK(valid());
+ return generation_;
+ }
+ void set_generation(size_t generation) { generation_ = generation; }
+
+ virtual void HandleEvent() = 0;
+
private:
monotonic_clock::time_point event_time_ = monotonic_clock::max_time;
+ size_t generation_ = 0;
};
// Adapter class to implement EventLoopEvent by calling HandleEvent on T.
@@ -34,7 +44,7 @@
class EventHandler final : public EventLoopEvent {
public:
EventHandler(T *t) : t_(t) {}
- ~EventHandler() = default;
+ ~EventHandler() override = default;
void HandleEvent() override { t_->HandleEvent(); }
private:
diff --git a/aos/events/event_loop_param_test.cc b/aos/events/event_loop_param_test.cc
index 71bef37..d0ca3ee 100644
--- a/aos/events/event_loop_param_test.cc
+++ b/aos/events/event_loop_param_test.cc
@@ -830,6 +830,48 @@
EXPECT_EQ(iteration_list.size(), 3);
}
+// Verify that we can disable a timer during execution of another timer
+// scheduled for the same time, with one ordering of creation for the timers.
+//
+// Also schedule some more events to reshuffle the heap in EventLoop used for
+// tracking events to change up the order. This used to segfault
+// SimulatedEventLoop.
+TEST_P(AbstractEventLoopTest, TimerDisableOther) {
+ for (bool creation_order : {true, false}) {
+ for (bool setup_order : {true, false}) {
+ for (int shuffle_events = 0; shuffle_events < 5; ++shuffle_events) {
+ auto loop = MakePrimary();
+ aos::TimerHandler *test_timer, *ender_timer;
+ if (creation_order) {
+ test_timer = loop->AddTimer([]() {});
+ ender_timer =
+ loop->AddTimer([&test_timer]() { test_timer->Disable(); });
+ } else {
+ ender_timer =
+ loop->AddTimer([&test_timer]() { test_timer->Disable(); });
+ test_timer = loop->AddTimer([]() {});
+ }
+
+ const auto start = loop->monotonic_now();
+
+ for (int i = 0; i < shuffle_events; ++i) {
+ loop->AddTimer([]() {})->Setup(start + std::chrono::milliseconds(10));
+ }
+
+ if (setup_order) {
+ test_timer->Setup(start + ::std::chrono::milliseconds(20));
+ ender_timer->Setup(start + ::std::chrono::milliseconds(20));
+ } else {
+ ender_timer->Setup(start + ::std::chrono::milliseconds(20));
+ test_timer->Setup(start + ::std::chrono::milliseconds(20));
+ }
+ EndEventLoop(loop.get(), ::std::chrono::milliseconds(40));
+ Run();
+ }
+ }
+ }
+}
+
// Verifies that the event loop implementations detect when Channel is not a
// pointer into confguration()
TEST_P(AbstractEventLoopDeathTest, InvalidChannel) {
diff --git a/aos/events/event_scheduler.cc b/aos/events/event_scheduler.cc
index a9ae7c0..f985eb1 100644
--- a/aos/events/event_scheduler.cc
+++ b/aos/events/event_scheduler.cc
@@ -14,6 +14,22 @@
}
void EventScheduler::Deschedule(EventScheduler::Token token) {
+ // We basically want to DCHECK some nontrivial logic. Guard it with NDEBUG to ensure the compiler
+ // realizes it's all unnecessary when not doing debug checks.
+#ifndef NDEBUG
+ {
+ bool found = false;
+ auto i = events_list_.begin();
+ while (i != events_list_.end()) {
+ if (i == token) {
+ CHECK(!found) << ": The same iterator is in the multimap twice??";
+ found = true;
+ }
+ ++i;
+ }
+ CHECK(found) << ": Trying to deschedule an event which is not scheduled";
+ }
+#endif
events_list_.erase(token);
}