commit 70deaf2feb07b89f37307c11e4d7785b75c52230
author Adam Snaider <adam.snaider@bluerivertech.com> Fri Aug 11 13:58:34 2023 -0700
committer James Kuszmaul <james.kuszmaul@bluerivertech.com> Thu Sep 14 14:31:46 2023 -0700
tree 48bb8a95c89ada3c8699d9f9c58607f46ab97acc
parent 7e4176831fc6aa7e25754d1711c03e846b8ae324

Disable timers on aos::Application on drop

The issue occurs when an application doesn't live for the duration
of the event loop. Originally, we were seeing segmentation faults
due to timer handlers being called after the application had been
dropped. We now do the proper cleanup to avoid this.

Change-Id: I42b474e72a536f9332b757a853edae471f29fc6c
Signed-off-by: James Kuszmaul <james.kuszmaul@bluerivertech.com>

aos/starter/subprocess.cc

diff --git a/aos/starter/subprocess.cc b/aos/starter/subprocess.cc
index 36e780a..f5563e6 100644
--- a/aos/starter/subprocess.cc
+++ b/aos/starter/subprocess.cc
@@ -663,4 +663,12 @@
   }
 }
 
+Application::~Application() {
+  start_timer_->Disable();
+  restart_timer_->Disable();
+  stop_timer_->Disable();
+  pipe_timer_->Disable();
+  child_status_handler_->Disable();
+}
+
 }  // namespace aos::starter

aos/starter/subprocess.h

diff --git a/aos/starter/subprocess.h b/aos/starter/subprocess.h
index ce70644..6de5cb8 100644
--- a/aos/starter/subprocess.h
+++ b/aos/starter/subprocess.h
@@ -81,6 +81,8 @@
               aos::EventLoop *event_loop, std::function<void()> on_change,
               QuietLogging quiet_flag = QuietLogging::kNo);
 
+  ~Application();
+
   flatbuffers::Offset<aos::starter::ApplicationStatus> PopulateStatus(
       flatbuffers::FlatBufferBuilder *builder, util::Top *top);
   aos::starter::State status() const { return status_; };
@@ -119,6 +121,7 @@
   bool autostart() const { return autostart_; }
 
   bool autorestart() const { return autorestart_; }
+  void set_autorestart(bool autorestart) { autorestart_ = autorestart; }
 
   const std::string &GetStdout();
   const std::string &GetStderr();

aos/starter/subprocess_test.cc

diff --git a/aos/starter/subprocess_test.cc b/aos/starter/subprocess_test.cc
index 4aa371b..cab22b4 100644
--- a/aos/starter/subprocess_test.cc
+++ b/aos/starter/subprocess_test.cc
@@ -1,5 +1,8 @@
 #include "aos/starter/subprocess.h"
 
+#include <signal.h>
+#include <sys/types.h>
+
 #include "gtest/gtest.h"
 
 #include "aos/events/shm_event_loop.h"
@@ -179,4 +182,47 @@
   EXPECT_EQ(aos::starter::State::STOPPED, error_out.status());
 }
 
+// Tests that Nothing Bad™ happens if the event loop outlives the Application.
+//
+// Note that this is a bit of a hope test, as there is no guarantee that we
+// will trigger a crash even if the resources tied to the event loop in the
+// aos::Application aren't properly released.
+TEST_F(SubprocessTest, ShortLivedApp) {
+  const std::string config_file =
+      ::aos::testing::ArtifactPath("aos/events/pingpong_config.json");
+
+  aos::FlatbufferDetachedBuffer<aos::Configuration> config =
+      aos::configuration::ReadConfig(config_file);
+  aos::ShmEventLoop event_loop(&config.message());
+
+  auto application =
+      std::make_unique<Application>("sleep", "sleep", &event_loop, []() {});
+  application->set_args({"10"});
+  application->Start();
+  pid_t pid = application->get_pid();
+
+  int ticks = 0;
+  aos::TimerHandler *exit_timer = event_loop.AddTimer([&event_loop, &ticks,
+                                                       &application, pid]() {
+    ticks++;
+    if (application && application->status() == aos::starter::State::RUNNING) {
+      // Kill the application, it will autorestart.
+      kill(pid, SIGTERM);
+      application.reset();
+    }
+
+    // event loop lives for longer.
+    if (ticks >= 5) {
+      // Now we exit.
+      event_loop.Exit();
+    }
+  });
+
+  event_loop.OnRun([&event_loop, exit_timer]() {
+    exit_timer->Schedule(event_loop.monotonic_now(),
+                         std::chrono::milliseconds(1000));
+  });
+
+  event_loop.Run();
+}
 }  // namespace aos::starter::testing