core: when a service's ExecStartPre= times out, skip ExecStop=

This makes sure we never run two control processes at the same time, we cannot keep track off.

This introduces a slight change of behaviour but cleans up the definition of ExecStop= and ExecStopPost=. The former is
now invoked only if the service managed to start-up correctly. The latter is called even if start-up failed half-way.
Thus, ExecStopPost= may be used as clean-up step for both successful and failed start-up attempts, but ExecStop='s
purpose is clearly defined as being responsible for shutting down the service and nothing else.

The precise behaviour of this was not documented yet. This commit adds the necessary docs.

Fixes: #1254

2 files changed, 33 insertions, 13 deletions

diff --git a/man/systemd.service.xml b/man/systemd.service.xml
index 14f6cd6adc..4cd36ac70e 100644
--- a/man/systemd.service.xml
+++ b/man/systemd.service.xml
@@ -383,6 +383,11 @@
         used to start long-running processes. All processes forked
         off by processes invoked via <varname>ExecStartPre=</varname> will
         be killed before the next service process is run.</para>
+
+        <para>Note that if any of the commands specified in <varname>ExecStartPre=</varname>,
+        <varname>ExecStart=</varname>, or <varname>ExecStartPost=</varname> fail (and are not prefixed with
+        <literal>-</literal>, see above) or time out before the service is fully up, execution continues with commands
+        specified in <varname>ExecStopPost=</varname>, the commands in <varname>ExecStop=</varname> are skipped.</para>
         </listitem>
       </varlistentry>
 
@@ -438,21 +443,36 @@
         <constant>SIGKILL</constant> immediately after the command
         exited, this would not result in a clean stop. The specified
         command should hence be a synchronous operation, not an
-        asynchronous one.</para></listitem>
+        asynchronous one.</para>
+
+        <para>Note that the commands specified in <varname>ExecStop=</varname> are only executed when the service
+        started successfuly first. They are not invoked if the service was never started at all, or in case its
+        start-up failed, for example because any of the commands specified in <varname>ExecStart=</varname>,
+        <varname>ExecStartPre=</varname> or <varname>ExecStartPost=</varname> failed (and weren't prefixed with
+        <literal>-</literal>, see above) or timed out. Use <varname>ExecStopPost=</varname> to invoke commands when a
+        service failed to start up correctly and is shut down again.</para>
+
+        <para>It is recommended to use this setting for commands that communicate with the service requesting clean
+        termination. When the commands specified with this option are executed it should be assumed that the service is
+        still fully up and is able to react correctly to all commands. For post-mortem clean-up steps use
+        <varname>ExecStopPost=</varname> instead.</para></listitem>
       </varlistentry>
 
       <varlistentry>
         <term><varname>ExecStopPost=</varname></term>
-        <listitem><para>Additional commands that are executed after
-        the service was stopped. This includes cases where the
-        commands configured in <varname>ExecStop=</varname> were used,
-        where the service does not have any
-        <varname>ExecStop=</varname> defined, or where the service
-        exited unexpectedly. This argument takes multiple command
-        lines, following the same scheme as described for
-        <varname>ExecStart=</varname>. Use of these settings is
-        optional. Specifier and environment variable substitution is
-        supported.</para></listitem>
+        <listitem><para>Additional commands that are executed after the service is stopped. This includes cases where
+        the commands configured in <varname>ExecStop=</varname> were used, where the service does not have any
+        <varname>ExecStop=</varname> defined, or where the service exited unexpectedly. This argument takes multiple
+        command lines, following the same scheme as described for <varname>ExecStart=</varname>. Use of these settings
+        is optional. Specifier and environment variable substitution is supported. Note that – unlike
+        <varname>ExecStop=</varname> – commands specified with this setting are invoked when a service failed to start
+        up correctly and is shut down again.</para>
+
+        <para>It is recommended to use this setting for clean-up operations that shall be executed even when the
+        service failed to start up correctly. Commands configured with this setting need to be able to operate even if
+        the service failed starting up half-way and left incompletely initialized data around. As the service's
+        processes have been terminated already when the commands specified with this setting are executed they should
+        not attempt to communicate with them.</para></listitem>
       </varlistentry>
 
       <varlistentry>
diff --git a/src/core/service.c b/src/core/service.c
index a9345e38b9..4942fc425d 100644
--- a/src/core/service.c
+++ b/src/core/service.c
@@ -2788,7 +2788,7 @@ static void service_sigchld_event(Unit *u, pid_t pid, int code, int status) {
 
                         case SERVICE_START_POST:
                                 if (f != SERVICE_SUCCESS) {
-                                        service_enter_stop(s, f);
+                                        service_enter_signal(s, SERVICE_STOP_SIGTERM, f);
                                         break;
                                 }
 
@@ -2878,7 +2878,7 @@ static int service_dispatch_timer(sd_event_source *source, usec_t usec, void *us
 
         case SERVICE_START_POST:
                 log_unit_warning(UNIT(s), "Start-post operation timed out. Stopping.");
-                service_enter_stop(s, SERVICE_FAILURE_TIMEOUT);
+                service_enter_signal(s, SERVICE_STOP_SIGTERM, SERVICE_FAILURE_TIMEOUT);
                 break;
 
         case SERVICE_RUNNING: