Browse Source

Fix: failing service did not propagate release to dependencies correctly

A service that failed to start wouldn't release its dependencies due to
an incorrect check. As a result dependencies might continue to run when
they should actually stop.
Davin McCall 1 year ago
parent
commit
b989677c4e
3 changed files with 36 additions and 5 deletions
  1. 4 4
      src/service.cc
  2. 2 1
      src/tests/test_service.h
  3. 30 0
      src/tests/tests.cc

+ 4 - 4
src/service.cc

@@ -160,7 +160,7 @@ void service_record::release(bool issue_stop) noexcept
         // a require was pending though:
         prop_release = !prop_require;
         prop_require = false;
-        if (prop_release && service_state != service_state_t::STOPPED) {
+        if (prop_release) {
             services->add_prop_queue(this);
         }
 
@@ -632,14 +632,14 @@ void service_record::do_stop(bool with_restart) noexcept
         if (service_state == service_state_t::STARTING) {
             // If waiting for a dependency, or waiting for the console, we can interrupt start. Otherwise,
             // we need to delegate to can_interrupt_start() (which can be overridden).
-            if (! waiting_for_deps && ! waiting_for_console) {
-                if (! can_interrupt_start()) {
+            if (!waiting_for_deps && !waiting_for_console) {
+                if (!can_interrupt_start()) {
                     // Well this is awkward: we're going to have to continue starting. We can stop once
                     // we've reached the started state.
                     return;
                 }
 
-                if (! interrupt_start()) {
+                if (!interrupt_start()) {
                     // Now wait for service startup to actually end; we don't need to handle it here.
                     notify_listeners(service_event_t::STARTCANCELLED);
                     return;

+ 2 - 1
src/tests/test_service.h

@@ -9,6 +9,7 @@ class test_service : public service_record
 {
     public:
     bool bring_up_reqd = false;
+    bool start_interruptible = false;
 
     test_service(service_set *set, std::string name, service_type_t type_p,
             const std::list<prelim_dep> &deplist_p)
@@ -46,7 +47,7 @@ class test_service : public service_record
     // having to wait for it reach STARTED and then go through STOPPING).
     virtual bool can_interrupt_start() noexcept override
     {
-        return waiting_for_deps;
+        return waiting_for_deps || start_interruptible;
     }
 
     virtual bool interrupt_start() noexcept override

+ 30 - 0
src/tests/tests.cc

@@ -1850,6 +1850,35 @@ void test_restart_stop4()
     s1->stopped();
 }
 
+void test_release_from_failed()
+{
+    service_set sset;
+
+    test_service *s1 = new test_service(&sset, "test-service-1", service_type_t::INTERNAL, {});
+    test_service *s2 = new test_service(&sset, "test-service-2", service_type_t::INTERNAL, {});
+    test_service *s3 = new test_service(&sset, "test-service-3", service_type_t::INTERNAL, {{s1, REG}, {s2, WAITS}});
+    //s1->auto_stop = false;
+    s2->start_interruptible = true;
+    //s3->auto_stop = false;
+    sset.add_service(s1);
+    sset.add_service(s2);
+    sset.add_service(s3);
+    assert(sset.find_service("test-service-1") == s1);
+    assert(sset.find_service("test-service-2") == s2);
+    assert(sset.find_service("test-service-3") == s3);
+
+    // start s3, which also starts s1/s2
+    sset.start_service(s3);
+    assert(s1->bring_up_reqd == true);
+
+    s1->failed_to_start();
+    sset.process_queues();
+    assert(s1->get_state() == service_state_t::STOPPED);
+    assert(s2->get_state() == service_state_t::STOPPED);
+    assert(s3->get_state() == service_state_t::STOPPED);
+}
+
+
 #define RUN_TEST(name, spacing) \
     std::cout << #name "..." spacing << std::flush; \
     name(); \
@@ -1902,4 +1931,5 @@ int main(int argc, char **argv)
     RUN_TEST(test_restart_stop2, "        ");
     RUN_TEST(test_restart_stop3, "        ");
     RUN_TEST(test_restart_stop4, "        ");
+    RUN_TEST(test_release_from_failed, "  ");
 }