From ef9652ba98d536ac22bc64e010a3b1abc0922f5d Mon Sep 17 00:00:00 2001 From: Davin McCall Date: Wed, 29 Mar 2017 16:59:52 +0100 Subject: [PATCH] Rework acquire/release handling. Now a service that is released releases its own dependencies immediately, instead of after it stops. This prevents those dependencies from restarting unnecessarily if they die/are killed in the meantime. --- TODO | 16 ------------ src/service.cc | 68 ++++++++++++++++++++++++-------------------------- src/service.h | 19 +++++++++----- 3 files changed, 45 insertions(+), 58 deletions(-) diff --git a/TODO b/TODO index 05ecf03..a1b0d8b 100644 --- a/TODO +++ b/TODO @@ -1,19 +1,3 @@ -* service "active" state is currently a combination of explicit marking and - state of dependents. I.e. a service is "active" if it has a running - dependent. This means it may restart automatically if it dies for some - reason (via normal restart or smooth recovery). - However, this is not always ideal. Sometimes the dependent is only - running because it is waiting for its own dependents to stop; that is, - there is no dependent further down the hierarchy that is explicitly - marked active. In that case, we don't really want to restart the service - that happened to die (unless and until a dependent becomes active). - The key here is to set the desired state to STOPPED earlier. When a - service is stopping all its dependencies which aren't otherwise required - should also go into STOPPING state (but should not actually start stopping - until the dependent is stopped). The means counting the two types of - require - require from running service vs require from stopping service - - separately. - * shutdown command presently hangs if terminal output blocked (scroll lock via ^S). Should use a buffer as dinit does, and pipe output from subcommands via the buffer too. diff --git a/src/service.cc b/src/service.cc index 6db03ef..d23c3fc 100644 --- a/src/service.cc +++ b/src/service.cc @@ -64,7 +64,7 @@ void ServiceSet::stopService(const std::string & name) noexcept } } -// Called when a service has actually stopped. +// Called when a service has actually stopped; dependents have stopped already. void ServiceRecord::stopped() noexcept { if (service_type != ServiceType::SCRIPTED && service_type != ServiceType::BGPROCESS && onstart_flags.runs_on_console) { @@ -73,20 +73,10 @@ void ServiceRecord::stopped() noexcept releaseConsole(); } - service_state = ServiceState::STOPPED; force_stop = false; - - logServiceStopped(service_name); - notifyListeners(ServiceEvent::STOPPED); - - bool will_restart = (desired_state == ServiceState::STARTED) && service_set->get_auto_restart(); - for (auto dependency : depends_on) { - if (! will_restart || ! dependency->can_interrupt_stop()) { - dependency->dependentStopped(); - } - } - + // If we are a soft dependency of another target, break the acquisition from that target now: + bool will_restart = (desired_state == ServiceState::STARTED) && service_set->get_auto_restart(); if (! will_restart) { for (auto dependency : soft_dpts) { if (dependency->holding_acq) { @@ -95,14 +85,20 @@ void ServiceRecord::stopped() noexcept } } } - + + will_restart &= (desired_state == ServiceState::STARTED); + for (auto dependency : depends_on) { + if (! will_restart || ! dependency->can_interrupt_stop()) { + dependency->dependentStopped(); + } + } + if (will_restart) { // Desired state is "started". + service_state = ServiceState::STOPPED; service_set->addToStartQueue(this); } else { - desired_state = ServiceState::STOPPED; - if (socket_fd != -1) { close(socket_fd); socket_fd = -1; @@ -110,14 +106,19 @@ void ServiceRecord::stopped() noexcept if (start_explicit) { start_explicit = false; - required_by--; + release(); } + service_state = ServiceState::STOPPED; if (required_by == 0) { - // Service is now completely inactive. - release_dependencies(); + // Since state wasn't STOPPED until now, any release performed above won't have marked + // the service inactive. We check for that now: + service_set->service_inactive(this); } } + + logServiceStopped(service_name); + notifyListeners(ServiceEvent::STOPPED); } dasynq::rearm ServiceChildWatcher::child_status(EventLoop_t &loop, pid_t child, int status) noexcept @@ -327,14 +328,15 @@ void ServiceRecord::release() noexcept { if (--required_by == 0) { desired_state = ServiceState::STOPPED; - // Can stop, and release dependencies once we're stopped. - if (service_state == ServiceState::STOPPED) { - prop_release = true; - prop_require = false; - service_set->addToPropQueue(this); + // Can stop, and can release dependencies now: + prop_release = true; + prop_require = false; + service_set->addToPropQueue(this); + if (service_state != ServiceState::STOPPED) { + service_set->addToStopQueue(this); } else { - service_set->addToStopQueue(this); + service_set->service_inactive(this); } } } @@ -352,8 +354,6 @@ void ServiceRecord::release_dependencies() noexcept i->holding_acq = false; } } - - service_set->service_inactive(this); } void ServiceRecord::start(bool activate) noexcept @@ -987,10 +987,10 @@ void ServiceRecord::do_stop() noexcept return; } } - + service_state = ServiceState::STOPPING; waiting_for_deps = true; - + // If we get here, we are in STARTED state; stop all dependents. if (stopDependents()) { allDepsStopped(); @@ -1021,14 +1021,10 @@ bool ServiceRecord::stopDependents() noexcept // check is run anyway. all_deps_stopped = false; } - if (force_stop) { - (*i)->forceStop(); - } - else { - service_set->addToStopQueue(*i); - } + + (*i)->forceStop(); } - + return all_deps_stopped; } diff --git a/src/service.h b/src/service.h index 91c0bb8..a07e0d1 100644 --- a/src/service.h +++ b/src/service.h @@ -68,6 +68,17 @@ * - desired state (which is manipulated by require/release operations) * * So a forced stop cannot occur until the service is not pinned started, for instance. + * + * Two-phase transition + * -------------------- + * Transition between states occurs in two phases: propagation and execution. In the + * propagation phase, acquisition/release messages are processed, and desired state may be + * altered accordingly. Desired state of dependencies/dependents should not be examined in + * this phase, since it may change during the phase (i.e. its current value at any point + * may not reflect the true final value). + * + * In the execution phase, actions are taken to achieve the desired state. Actual state may + * transition according to the current and desired states. */ struct OnstartFlags { @@ -338,10 +349,10 @@ class ServiceRecord // Open the activation socket, return false on failure bool open_socket() noexcept; - + // Check whether dependencies have started, and optionally ask them to start bool startCheckDependencies(bool do_start) noexcept; - + // Whether a STARTING service can immediately transition to STOPPED (as opposed to // having to wait for it reach STARTED and then go through STOPPING). bool can_interrupt_start() noexcept @@ -355,10 +366,6 @@ class ServiceRecord return waiting_for_deps && ! force_stop; } - // Notify dependencies that we no longer need them, - // (if this is actually the case). - void notify_dependencies_stopped() noexcept; - // A dependent has reached STOPPED state void dependentStopped() noexcept; -- 2.25.1