bug#46022: 27.1; kill-emacs should call sd_notify only in daemon mode

bug#46022: 27.1; kill-emacs should call sd_notify only in daemon mode

[Tim Ruffing]

When run with --fg-daemon mode (as done by the provided systemd unit
file "emacs.service"), emacs will call sd_notify() on startup to notify
systemd about its status. On exit, emacs will call sd_notify()
unconditionally, i.e., even when not run in daemon mode.

This means that when the daemon is running, other emacs processes
derived from the main process (e.g., in batch mode started by the async
package) may send exit notifications when they're exiting, even though
the main daemon process is not exiting. systemd will (correctly) ignore
these notifications because they come from the wrong PID; this is the
lucky default value of "NotifyAccess=main" in the systemd unit, i.e.,
only the main PID may send notifications. However, systemd will also
emit a warning in this case. With my config, this results in a lot of
warning messages in the systemd user journal:
Jan 20 14:23:06 systemd[1309]: emacs.service: Got notification message from PID 271936, but reception only permitted for main PID 1365
Jan 20 14:24:06 systemd[1309]: emacs.service: Got notification message from PID 271956, but reception only permitted for main PID 1365
Jan 20 14:25:07 systemd[1309]: emacs.service: Got notification message from PID 271983, but reception only permitted for main PID 1365
Jan 20 14:26:06 systemd[1309]: emacs.service: Got notification message from PID 272071, but reception only permitted for main PID 1365
Jan 20 14:27:06 systemd[1309]: emacs.service: Got notification message from PID 272092, but reception only permitted for main PID 1365
Jan 20 14:28:06 systemd[1309]: emacs.service: Got notification message from PID 272127, but reception only permitted for main PID 1365
Jan 20 14:29:06 systemd[1309]: emacs.service: Got notification message from PID 272304, but reception only permitted for main PID 1365
Jan 20 14:30:06 systemd[1309]: emacs.service: Got notification message from PID 272442, but reception only permitted for main PID 1365 

I admit that this config may be exotic but the core of the issue is not
the number of messages, it's simply that there shouldn't be any
notification (nor warning).

So I think the right thing to do is to call sd_notify in kill-emacs
only when we run in foreground daemon mode, i.e., if daemon_type == 1.
This is done when calling sd_notify in daemon-initialized. I assume the
check was omitted in kill-emacs because daemon-initialized sets
daemon_type = -1, so this information is no longer accessible when
kill-emacs runs (and it was believed sd-notify is a no-op anyway in
those cases -- but that's only partly true due to warning message).
 
In GNU Emacs 27.1 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.24.22,
cairo version 1.17.3)
 of 2020-08-28 built on juergen
Windowing system distributor 'The X.Org Foundation', version
11.0.12010000
System Description: Arch Linux

Configured using:
 'configure --prefix=/usr --sysconfdir=/etc --libexecdir=/usr/lib
 --localstatedir=/var --with-x-toolkit=gtk3 --with-xft --with-wide-int
 --with-modules --with-cairo --with-harfbuzz 'CFLAGS=-march=x86-64
 -mtune=generic -O2 -pipe -fno-plt' CPPFLAGS=-D_FORTIFY_SOURCE=2
 LDFLAGS=-Wl,-O1,--sort-common,--as-needed,-z,relro,-z,now'

Configured features:
XPM JPEG TIFF GIF PNG RSVG CAIRO SOUND GPM DBUS GSETTINGS GLIB NOTIFY
INOTIFY ACL GNUTLS LIBXML2 FREETYPE HARFBUZZ M17N_FLT LIBOTF ZLIB
TOOLKIT_SCROLL_BARS GTK3 X11 XDBE XIM MODULES THREADS LIBSYSTEMD JSON
PDUMPER LCMS2 GMP

bug#46022: 27.1; kill-emacs should call sd_notify only in daemon mode

[Eli Zaretskii]

> From: Tim Ruffing <public@timruffing.de>
> Date: Thu, 21 Jan 2021 17:26:31 +0100
> 
> When run with --fg-daemon mode (as done by the provided systemd unit
> file "emacs.service"), emacs will call sd_notify() on startup to notify
> systemd about its status. On exit, emacs will call sd_notify()
> unconditionally, i.e., even when not run in daemon mode.
> 
> This means that when the daemon is running, other emacs processes
> derived from the main process (e.g., in batch mode started by the async
> package) may send exit notifications when they're exiting, even though
> the main daemon process is not exiting. systemd will (correctly) ignore
> these notifications because they come from the wrong PID; this is the
> lucky default value of "NotifyAccess=main" in the systemd unit, i.e.,
> only the main PID may send notifications. However, systemd will also
> emit a warning in this case. With my config, this results in a lot of
> warning messages in the systemd user journal:

I'm not an expert on systemd, not even close, but please review the
reasoning for what we do in bug#31498, and let's take it from there.
Quote:

  Both of these calls are successful no-ops if emacs was not started
  by systemd.

CC'ing Lucas, who submitted that changeset, in case hoe would like to
comment.

bug#46022: 27.1; kill-emacs should call sd_notify only in daemon mode

[Lucas Werkmeister]

Hm, I wasn’t aware systemd logged those warnings. Probably a good idea
to guard the sd_notify(0, "STOPPING=1") with a guard on (a stashed copy
of?) daemon_type, then, yes.

bug#46022: 27.1; kill-emacs should call sd_notify only in daemon mode

[Eli Zaretskii]

> Cc: 46022@debbugs.gnu.org
> From: Lucas Werkmeister <mail@lucaswerkmeister.de>
> Date: Thu, 21 Jan 2021 21:12:03 +0100
> 
> Hm, I wasn’t aware systemd logged those warnings. Probably a good idea
> to guard the sd_notify(0, "STOPPING=1") with a guard on (a stashed copy
> of?) daemon_type, then, yes.

OK, thanks.  Does the patch below seems reasonable and give good
results?  If yes, I'd like it to go into the upcoming Emacs 27.2.

diff --git a/src/emacs.c b/src/emacs.c
index f2e858f..67220eb 100644
--- a/src/emacs.c
+++ b/src/emacs.c
@@ -187,7 +187,8 @@ #define MAIN_PROGRAM
 /* Name for the server started by the daemon.*/
 static char *daemon_name;
 
-/* 0 not a daemon, 1 new-style (foreground), 2 old-style (background).  */
+/* 0 not a daemon, 1 new-style (foreground), 2 old-style (background).
+   A negative value means the daemon initialization was already done.  */
 int daemon_type;
 
 #ifndef WINDOWSNT
@@ -2371,7 +2372,10 @@ DEFUN ("kill-emacs", Fkill_emacs, Skill_emacs, 0, 1, "P",
   int exit_code;
 
 #ifdef HAVE_LIBSYSTEMD
-  sd_notify(0, "STOPPING=1");
+  /* Notify systemd we are shutting down, but only if we have notified
+     it about startup.  */
+  if (daemon_type == -1)
+    sd_notify(0, "STOPPING=1");
 #endif /* HAVE_LIBSYSTEMD */
 
   /* Fsignal calls emacs_abort () if it sees that waiting_for_input is
@@ -2876,7 +2880,7 @@ DEFUN ("daemon-initialized", Fdaemon_initialized, Sdaemon_initialized, 0, 0, 0,
     }
 
   /* Set it to an invalid value so we know we've already run this function.  */
-  daemon_type = -1;
+  daemon_type = -daemon_type;
 
 #else  /* WINDOWSNT */
   /* Signal the waiting emacsclient process.  */

bug#46022: 27.1; kill-emacs should call sd_notify only in daemon mode

[Lucas Werkmeister]

On 22.01.21 09:19, Eli Zaretskii wrote:
>> Cc: 46022@debbugs.gnu.org
>> From: Lucas Werkmeister <mail@lucaswerkmeister.de>
>> Date: Thu, 21 Jan 2021 21:12:03 +0100
>>
>> Hm, I wasn’t aware systemd logged those warnings. Probably a good idea
>> to guard the sd_notify(0, "STOPPING=1") with a guard on (a stashed copy
>> of?) daemon_type, then, yes.
> 
> OK, thanks.  Does the patch below seems reasonable and give good
> results?  If yes, I'd like it to go into the upcoming Emacs 27.2.
> 
> diff --git a/src/emacs.c b/src/emacs.c
> index f2e858f..67220eb 100644
> --- a/src/emacs.c
> +++ b/src/emacs.c
> @@ -187,7 +187,8 @@ #define MAIN_PROGRAM
>  /* Name for the server started by the daemon.*/
>  static char *daemon_name;
>  
> -/* 0 not a daemon, 1 new-style (foreground), 2 old-style (background).  */
> +/* 0 not a daemon, 1 new-style (foreground), 2 old-style (background).
> +   A negative value means the daemon initialization was already done.  */
>  int daemon_type;
>  
>  #ifndef WINDOWSNT
> @@ -2371,7 +2372,10 @@ DEFUN ("kill-emacs", Fkill_emacs, Skill_emacs, 0, 1, "P",
>    int exit_code;
>  
>  #ifdef HAVE_LIBSYSTEMD
> -  sd_notify(0, "STOPPING=1");
> +  /* Notify systemd we are shutting down, but only if we have notified
> +     it about startup.  */
> +  if (daemon_type == -1)
> +    sd_notify(0, "STOPPING=1");
>  #endif /* HAVE_LIBSYSTEMD */
>  
>    /* Fsignal calls emacs_abort () if it sees that waiting_for_input is
> @@ -2876,7 +2880,7 @@ DEFUN ("daemon-initialized", Fdaemon_initialized, Sdaemon_initialized, 0, 0, 0,
>      }
>  
>    /* Set it to an invalid value so we know we've already run this function.  */
> -  daemon_type = -1;
> +  daemon_type = -daemon_type;
>  
>  #else  /* WINDOWSNT */
>    /* Signal the waiting emacsclient process.  */
> 

That looks good to me. If daemon_type is 0, it will now stay at 0
instead of being marked as invalid, but that should be okay, since
daemon-initialized has no special code for daemon_type == 0.

bug#46022: 27.1; kill-emacs should call sd_notify only in daemon mode

[Eli Zaretskii]

> Cc: public@timruffing.de, 46022@debbugs.gnu.org
> From: Lucas Werkmeister <mail@lucaswerkmeister.de>
> Date: Fri, 22 Jan 2021 10:47:49 +0100
> 
> > -  daemon_type = -1;
> > +  daemon_type = -daemon_type;
> >  
> >  #else  /* WINDOWSNT */
> >    /* Signal the waiting emacsclient process.  */
> > 
> 
> That looks good to me. If daemon_type is 0, it will now stay at 0
> instead of being marked as invalid, but that should be okay, since
> daemon-initialized has no special code for daemon_type == 0.

Right.

Tim, does this work for you?  Can you perhaps test this on your
system?

bug#46022: 27.1; kill-emacs should call sd_notify only in daemon mode

[Tim Ruffing]

I tested the patch and I can conform that it works and makes the
warnings disappear. 

Thanks for the quick response!

Tim


On Fri, 2021-01-22 at 13:45 +0200, Eli Zaretskii wrote:
> > Cc: public@timruffing.de, 46022@debbugs.gnu.org
> > From: Lucas Werkmeister <mail@lucaswerkmeister.de>
> > Date: Fri, 22 Jan 2021 10:47:49 +0100
> > 
> > > -  daemon_type = -1;
> > > +  daemon_type = -daemon_type;
> > >  
> > >  #else  /* WINDOWSNT */
> > >    /* Signal the waiting emacsclient process.  */
> > > 
> > 
> > That looks good to me. If daemon_type is 0, it will now stay at 0
> > instead of being marked as invalid, but that should be okay, since
> > daemon-initialized has no special code for daemon_type == 0.
> 
> Right.
> 
> Tim, does this work for you?  Can you perhaps test this on your
> system?

bug#46022: 27.1; kill-emacs should call sd_notify only in daemon mode

[Eli Zaretskii]

> From: Tim Ruffing <public@timruffing.de>
> Cc: 46022@debbugs.gnu.org
> Date: Fri, 22 Jan 2021 13:07:35 +0100
> 
> I tested the patch and I can conform that it works and makes the
> warnings disappear. 

Thanks, I've now installed the change on the emacs-27 branch, and I'm
closing this bug report.