bug#68799: 30.0.50; emacs --fg-daemon fails silently if server-start fails

bug#68799: 30.0.50; emacs --fg-daemon fails silently if server-start fails

[Spencer Baugh]

1. emacs -Q --fg-daemon=/nonexistent/dir/sock
2. Emacs prints "Starting Emacs daemon." and sits in foreground.
3. emacsclient -c -s /nonexistent/dir/sock
4. emacsclient prints and exits:
emacsclient: can't find socket; have you started the server?
emacsclient: To start the server in Emacs, type "M-x server-start".
emacsclient: error accessing socket "/nonexistent/dir/sock"

This is because in step 1, the server actually failed to start, but
Emacs did not log that at all.  In fact, it's impossible to access the
Emacs started in 1 now, since it's not actually running a server and it
has no frames.

The same thing happens with --bg-daemon, although it's slightly more
obvious that something is wrong in that case, because --bg-daemon is not
supposed to sit in foreground.

Emacs with --fg-daemon should instead print an error and exit if it
fails to start the server.


In GNU Emacs 30.0.50 (build 22, x86_64-pc-linux-gnu, X toolkit, cairo
 version 1.15.12, Xaw scroll bars) of 2024-01-29 built on
 igm-qws-u22796a
Repository revision: cbc8bdc1386a4bc9c420d8ab06b844c6f6928c35
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12011000
System Description: Rocky Linux 8.9 (Green Obsidian)

Configured using:
 'configure -C 'CFLAGS=-O0 -g3' --with-gif=ifavailable
 --with-x-toolkit=lucid'

Configured features:
CAIRO DBUS FREETYPE GLIB GMP GNUTLS GSETTINGS HARFBUZZ JPEG JSON
LIBSELINUX LIBSYSTEMD LIBXML2 MODULES NOTIFY INOTIFY PDUMPER PNG RSVG
SECCOMP SOUND SQLITE3 THREADS TIFF TOOLKIT_SCROLL_BARS X11 XDBE XIM
XINPUT2 XPM LUCID ZLIB

Important settings:
  value of $LANG: en_US.UTF-8
  locale-coding-system: utf-8-unix

Major mode: Lisp Interaction

Minor modes in effect:
  tooltip-mode: t
  global-eldoc-mode: t
  eldoc-mode: t
  show-paren-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  tool-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  minibuffer-regexp-mode: t
  line-number-mode: t
  indent-tabs-mode: t
  transient-mark-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t

Load-path shadows:
None found.

Features:
(shadow sort mail-extr emacsbug message mailcap yank-media puny dired
dired-loaddefs rfc822 mml mml-sec password-cache epa derived epg rfc6068
epg-config gnus-util text-property-search time-date subr-x mm-decode
mm-bodies mm-encode mail-parse rfc2231 mailabbrev gmm-utils mailheader
cl-loaddefs cl-lib sendmail rfc2047 rfc2045 ietf-drums mm-util
mail-prsvr mail-utils rmc iso-transl tooltip cconv eldoc paren electric
uniquify ediff-hook vc-hooks lisp-float-type elisp-mode mwheel
term/x-win x-win term/common-win x-dnd touch-screen tool-bar dnd fontset
image regexp-opt fringe tabulated-list replace newcomment text-mode
lisp-mode prog-mode register page tab-bar menu-bar rfn-eshadow isearch
easymenu timer select scroll-bar mouse jit-lock font-lock syntax
font-core term/tty-colors frame minibuffer nadvice seq simple cl-generic
indonesian philippine cham georgian utf-8-lang misc-lang vietnamese
tibetan thai tai-viet lao korean japanese eucjp-ms cp51932 hebrew greek
romanian slovak czech european ethiopic indian cyrillic chinese
composite emoji-zwj charscript charprop case-table epa-hook
jka-cmpr-hook help abbrev obarray oclosure cl-preloaded button loaddefs
theme-loaddefs faces cus-face macroexp files window text-properties
overlay sha1 md5 base64 format env code-pages mule custom widget keymap
hashtable-print-readable backquote threads dbusbind inotify
dynamic-setting system-font-setting font-render-setting cairo x-toolkit
xinput2 x multi-tty move-toolbar make-network-process emacs)

Memory information:
((conses 16 64886 10659) (symbols 48 9545 0) (strings 32 22761 1636)
 (string-bytes 1 676280) (vectors 16 9262)
 (vector-slots 8 111310 9095) (floats 8 40 17) (intervals 56 262 0)
 (buffers 976 10))

bug#68799: 30.0.50; emacs --fg-daemon fails silently if server-start fails

[Eli Zaretskii]

> From: Spencer Baugh <sbaugh@janestreet.com>
> Date: Mon, 29 Jan 2024 11:54:21 -0500
> 
> 
> 1. emacs -Q --fg-daemon=/nonexistent/dir/sock
> 2. Emacs prints "Starting Emacs daemon." and sits in foreground.
> 3. emacsclient -c -s /nonexistent/dir/sock
> 4. emacsclient prints and exits:
> emacsclient: can't find socket; have you started the server?
> emacsclient: To start the server in Emacs, type "M-x server-start".
> emacsclient: error accessing socket "/nonexistent/dir/sock"
> 
> This is because in step 1, the server actually failed to start, but
> Emacs did not log that at all.  In fact, it's impossible to access the
> Emacs started in 1 now, since it's not actually running a server and it
> has no frames.
> 
> The same thing happens with --bg-daemon, although it's slightly more
> obvious that something is wrong in that case, because --bg-daemon is not
> supposed to sit in foreground.
> 
> Emacs with --fg-daemon should instead print an error and exit if it
> fails to start the server.

Have some faith in Emacs: we already do that.  From startup.el:

  (let ((dn (daemonp)))
    (when dn
      (when (stringp dn) (setq server-name dn))
      (server-start)
      (if server-process
	  (daemon-initialized)
	(if (stringp dn)
	    (message
	     "Unable to start daemon: Emacs server named %S already running"
	     server-name)
	  (message "Unable to start the daemon.\nAnother instance of Emacs is running the server, either as daemon or interactively.\nYou can use emacsclient to connect to that Emacs process."))
	(kill-emacs 1))))

So the problem seems to be that somehow server-start succeeds to leave
a non-nil server-process variable behind, although testing that is the
documented way of telling whether server is running.

So what is the value of server-process in your scenario?

bug#68799: 30.0.50; emacs --fg-daemon fails silently if server-start fails

[Eli Zaretskii]

> Cc: 68799@debbugs.gnu.org
> Date: Mon, 29 Jan 2024 19:11:36 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> 
> Have some faith in Emacs: we already do that.  From startup.el:
> 
>   (let ((dn (daemonp)))
>     (when dn
>       (when (stringp dn) (setq server-name dn))
>       (server-start)
>       (if server-process
> 	  (daemon-initialized)
> 	(if (stringp dn)
> 	    (message
> 	     "Unable to start daemon: Emacs server named %S already running"
> 	     server-name)
> 	  (message "Unable to start the daemon.\nAnother instance of Emacs is running the server, either as daemon or interactively.\nYou can use emacsclient to connect to that Emacs process."))
> 	(kill-emacs 1))))
> 
> So the problem seems to be that somehow server-start succeeds to leave
> a non-nil server-process variable behind, although testing that is the
> documented way of telling whether server is running.

Or maybe server-start signals an error, and then the code which shows
an error message and shuts down Emacs doesn't get run?

bug#68799: 30.0.50; emacs --fg-daemon fails silently if server-start fails

[Spencer Baugh]

Eli Zaretskii <eliz@gnu.org> writes:
>> Cc: 68799@debbugs.gnu.org
>> Date: Mon, 29 Jan 2024 19:11:36 +0200
>> From: Eli Zaretskii <eliz@gnu.org>
>> 
>> Have some faith in Emacs: we already do that.  From startup.el:
>> 
>>   (let ((dn (daemonp)))
>>     (when dn
>>       (when (stringp dn) (setq server-name dn))
>>       (server-start)
>>       (if server-process
>> 	  (daemon-initialized)
>> 	(if (stringp dn)
>> 	    (message
>> 	     "Unable to start daemon: Emacs server named %S already running"
>> 	     server-name)
>> 	  (message "Unable to start the daemon.\nAnother instance of Emacs is running the server, either as daemon or interactively.\nYou can use emacsclient to connect to that Emacs process."))
>> 	(kill-emacs 1))))
>> 
>> So the problem seems to be that somehow server-start succeeds to leave
>> a non-nil server-process variable behind, although testing that is the
>> documented way of telling whether server is running.
>
> Or maybe server-start signals an error, and then the code which shows
> an error message and shuts down Emacs doesn't get run?

Yes, that's exactly what happens.

So should we wrap a condition-case around server-start, I suppose?

bug#68799: 30.0.50; emacs --fg-daemon fails silently if server-start fails

[Eli Zaretskii]

> From: Spencer Baugh <sbaugh@janestreet.com>
> Cc: 68799@debbugs.gnu.org
> Date: Mon, 29 Jan 2024 12:32:00 -0500
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> So the problem seems to be that somehow server-start succeeds to leave
> >> a non-nil server-process variable behind, although testing that is the
> >> documented way of telling whether server is running.
> >
> > Or maybe server-start signals an error, and then the code which shows
> > an error message and shuts down Emacs doesn't get run?
> 
> Yes, that's exactly what happens.
> 
> So should we wrap a condition-case around server-start, I suppose?

Probably.  But perhaps we should also modify server-start so that,
when it is called from a daemon, it signals in this case a distinct
error, which startup.el could then detect and display a friendly error
message.

bug#68799: 30.0.50; emacs --fg-daemon fails silently if server-start fails

[Spencer Baugh]

[-- Attachment #1: Type: text/plain, Size: 1219 bytes --]

Eli Zaretskii <eliz@gnu.org> writes:
>> From: Spencer Baugh <sbaugh@janestreet.com>
>> Cc: 68799@debbugs.gnu.org
>> Date: Mon, 29 Jan 2024 12:32:00 -0500
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> >> So the problem seems to be that somehow server-start succeeds to leave
>> >> a non-nil server-process variable behind, although testing that is the
>> >> documented way of telling whether server is running.
>> >
>> > Or maybe server-start signals an error, and then the code which shows
>> > an error message and shuts down Emacs doesn't get run?
>> 
>> Yes, that's exactly what happens.
>> 
>> So should we wrap a condition-case around server-start, I suppose?
>
> Probably.  But perhaps we should also modify server-start so that,
> when it is called from a daemon, it signals in this case a distinct
> error, which startup.el could then detect and display a friendly error
> message.

Actually, it seems that main() already does a
fputs ("Error: server did not start correctly\n", stderr);
if the server process exits non-zero.  So we already get a nice:

Starting Emacs daemon.
Creating directory: Permission denied, /nonexistent
Error: server did not start correctly

So I think the below patch suffices.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Message-when-server-start-errors-in-emacs-daemon.patch --]
[-- Type: text/x-patch, Size: 1406 bytes --]

From 2a6f039663a678d0fcae47318011fbb5d8bb5f1c Mon Sep 17 00:00:00 2001
From: Spencer Baugh <sbaugh@janestreet.com>
Date: Mon, 29 Jan 2024 13:11:47 -0500
Subject: [PATCH] Message when server-start errors in emacs --daemon

Previously, if a user invoked emacs --daemon and then server-start
failed, there would be no indication of this and emacs would simply
hang.

Now, something like emacs --daemon=/nonexistent/sock results in:

$ emacs --fg-daemon=/nonexistent/sock
Starting Emacs daemon.
Creating directory: Permission denied, /nonexistent
$ emacs --daemon=/nonexistent/sock
Starting Emacs daemon.
Creating directory: Permission denied, /nonexistent
Error: server did not start correctly

and Emacs exits non-zero.

* lisp/startup.el (command-line): Catch errors from server-start, and
message and exit.  (bug#68799)
---
 lisp/startup.el | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/lisp/startup.el b/lisp/startup.el
index 23937055f30..ae465b5bb07 100644
--- a/lisp/startup.el
+++ b/lisp/startup.el
@@ -1627,7 +1627,11 @@ command-line
   (let ((dn (daemonp)))
     (when dn
       (when (stringp dn) (setq server-name dn))
-      (server-start)
+      (condition-case err
+          (server-start)
+        (error
+         (message "%s" (error-message-string err))
+         (kill-emacs 1)))
       (if server-process
 	  (daemon-initialized)
 	(if (stringp dn)
-- 
2.39.3

bug#68799: 30.0.50; emacs --fg-daemon fails silently if server-start fails

[Eli Zaretskii]

> From: Spencer Baugh <sbaugh@janestreet.com>
> Cc: 68799@debbugs.gnu.org
> Date: Mon, 29 Jan 2024 13:13:42 -0500
> 
> --- a/lisp/startup.el
> +++ b/lisp/startup.el
> @@ -1627,7 +1627,11 @@ command-line
>    (let ((dn (daemonp)))
>      (when dn
>        (when (stringp dn) (setq server-name dn))
> -      (server-start)
> +      (condition-case err
> +          (server-start)
> +        (error
> +         (message "%s" (error-message-string err))
> +         (kill-emacs 1)))

Thanks, this needs a comment explaining why we need condition-case and
where does error-message-string come from.

bug#68799: 30.0.50; emacs --fg-daemon fails silently if server-start fails

[Spencer Baugh]

Eli Zaretskii <eliz@gnu.org> writes:
>> From: Spencer Baugh <sbaugh@janestreet.com>
>> Cc: 68799@debbugs.gnu.org
>> Date: Mon, 29 Jan 2024 13:13:42 -0500
>> 
>> --- a/lisp/startup.el
>> +++ b/lisp/startup.el
>> @@ -1627,7 +1627,11 @@ command-line
>>    (let ((dn (daemonp)))
>>      (when dn
>>        (when (stringp dn) (setq server-name dn))
>> -      (server-start)
>> +      (condition-case err
>> +          (server-start)
>> +        (error
>> +         (message "%s" (error-message-string err))
>> +         (kill-emacs 1)))
>
> Thanks, this needs a comment explaining why we need condition-case and
> where does error-message-string come from.

Actually, on second thought, we could fail anywhere in startup.el, not
just in server-start.  So should we actually have a wrapper around all
of normal-top-level which detects an error at startup in a daemon?

For example, this will also fail to start the server and hang with no
error message:

emacs --eval '(error)' --daemon

Likewise any errors raised in init.el will also just hang without
starting the server, although that at least causes some logs to be
printed.

Maybe we should handle this in the C top_level_1, conditional on
IS_DAEMON?

bug#68799: 30.0.50; emacs --fg-daemon fails silently if server-start fails

[Eli Zaretskii]

> From: Spencer Baugh <sbaugh@janestreet.com>
> Cc: 68799@debbugs.gnu.org
> Date: Mon, 29 Jan 2024 15:28:47 -0500
> 
> >> --- a/lisp/startup.el
> >> +++ b/lisp/startup.el
> >> @@ -1627,7 +1627,11 @@ command-line
> >>    (let ((dn (daemonp)))
> >>      (when dn
> >>        (when (stringp dn) (setq server-name dn))
> >> -      (server-start)
> >> +      (condition-case err
> >> +          (server-start)
> >> +        (error
> >> +         (message "%s" (error-message-string err))
> >> +         (kill-emacs 1)))
> >
> > Thanks, this needs a comment explaining why we need condition-case and
> > where does error-message-string come from.
> 
> Actually, on second thought, we could fail anywhere in startup.el, not
> just in server-start.  So should we actually have a wrapper around all
> of normal-top-level which detects an error at startup in a daemon?

I'd prefer to handle each specific problem specially, to make sure the
error message is self-explanatory.  Also, if the error happens after
the server has been started, there's no reason to forcibly exit.

So I think we should for now solve this particular issue, and not try
generalizing too much.

bug#68799: 30.0.50; emacs --fg-daemon fails silently if server-start fails

[sbaugh]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Spencer Baugh <sbaugh@janestreet.com>
>> Cc: 68799@debbugs.gnu.org
>> Date: Mon, 29 Jan 2024 15:28:47 -0500
>> 
>> >> --- a/lisp/startup.el
>> >> +++ b/lisp/startup.el
>> >> @@ -1627,7 +1627,11 @@ command-line
>> >>    (let ((dn (daemonp)))
>> >>      (when dn
>> >>        (when (stringp dn) (setq server-name dn))
>> >> -      (server-start)
>> >> +      (condition-case err
>> >> +          (server-start)
>> >> +        (error
>> >> +         (message "%s" (error-message-string err))
>> >> +         (kill-emacs 1)))
>> >
>> > Thanks, this needs a comment explaining why we need condition-case and
>> > where does error-message-string come from.
>> 
>> Actually, on second thought, we could fail anywhere in startup.el, not
>> just in server-start.  So should we actually have a wrapper around all
>> of normal-top-level which detects an error at startup in a daemon?
>
> I'd prefer to handle each specific problem specially, to make sure the
> error message is self-explanatory.  Also, if the error happens after
> the server has been started, there's no reason to forcibly exit.
>
> So I think we should for now solve this particular issue, and not try
> generalizing too much.

To be clear, right now any error anywhere in command-line causes "emacs
--fg-daemon" and "emacs --bg-daemon" to hang indefinitely, without
printing an error, with no way to ever interact with the Emacs process.
This error can come from any code, so if we have *any* bugs anywhere in
code called from command-line, it will cause Emacs to enter this state.

We can add good error messages for individual classes of error, but we
should also have a catch-all check to make sure that Emacs doesn't enter
this broken state if we (or the user) write code which contains a bug.

I have concrete reasons to want this: I think there's a bug in
command-line in trunk which some of my users using emacs --daemon have
run into.  But I have zero information about what caused the bug,
because Emacs just hangs without printing any error message in this
case.

To allow users to report bugs that are at all useful, we should at least
print the error that occurred, even if we don't kill Emacs.

bug#68799: 30.0.50; emacs --fg-daemon fails silently if server-start fails

[Eli Zaretskii]

> From: sbaugh@catern.com
> Date: Sat, 10 Feb 2024 19:50:20 +0000 (UTC)
> Cc: Spencer Baugh <sbaugh@janestreet.com>, 68799@debbugs.gnu.org
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> > Thanks, this needs a comment explaining why we need condition-case and
> >> > where does error-message-string come from.
> >> 
> >> Actually, on second thought, we could fail anywhere in startup.el, not
> >> just in server-start.  So should we actually have a wrapper around all
> >> of normal-top-level which detects an error at startup in a daemon?
> >
> > I'd prefer to handle each specific problem specially, to make sure the
> > error message is self-explanatory.  Also, if the error happens after
> > the server has been started, there's no reason to forcibly exit.
> >
> > So I think we should for now solve this particular issue, and not try
> > generalizing too much.
> 
> To be clear, right now any error anywhere in command-line causes "emacs
> --fg-daemon" and "emacs --bg-daemon" to hang indefinitely, without
> printing an error, with no way to ever interact with the Emacs process.

That's not what you said before: you said "anywhere in startup.el",
which is much more general.  Now you are saying something different.

What exactly is meant by "anywhere in command-line"? the function
command-line in startup.el? or something else?

> This error can come from any code, so if we have *any* bugs anywhere in
> code called from command-line, it will cause Emacs to enter this state.

Why would we assume that *any code* there will signal an error?
That's like saying that Emacs is a useless program that always signals
errors in random places.  That's a non-starter here.

> We can add good error messages for individual classes of error, but we
> should also have a catch-all check to make sure that Emacs doesn't enter
> this broken state if we (or the user) write code which contains a bug.

There's no reason to have a catch-all check where no error is
expected.  Do you always wrap all of your code in condition-case and
the likes?  If not, why not?

> I have concrete reasons to want this: I think there's a bug in
> command-line in trunk which some of my users using emacs --daemon have
> run into.  But I have zero information about what caused the bug,
> because Emacs just hangs without printing any error message in this
> case.

Then please debug that, and let's talk when you do have concrete data.

> To allow users to report bugs that are at all useful, we should at least
> print the error that occurred, even if we don't kill Emacs.

Users aren't supposed to debug Emacs, it's the job of developers.  And
developers know how to run Emacs under a debugger and do any number of
other debugging tricks.  Injecting debugging code into Emacs just
because some of your users did something wrong is not a good idea.  Or
maybe it is -- but we won't know until we understand what exactly goes
wrong in those cases.

bug#68799: 30.0.50; emacs --fg-daemon fails silently if server-start fails

[sbaugh]

Eli Zaretskii <eliz@gnu.org> writes:
>> From: sbaugh@catern.com
>> Date: Sat, 10 Feb 2024 19:50:20 +0000 (UTC)
>> Cc: Spencer Baugh <sbaugh@janestreet.com>, 68799@debbugs.gnu.org
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> >> > Thanks, this needs a comment explaining why we need condition-case and
>> >> > where does error-message-string come from.
>> >> 
>> >> Actually, on second thought, we could fail anywhere in startup.el, not
>> >> just in server-start.  So should we actually have a wrapper around all
>> >> of normal-top-level which detects an error at startup in a daemon?
>> >
>> > I'd prefer to handle each specific problem specially, to make sure the
>> > error message is self-explanatory.  Also, if the error happens after
>> > the server has been started, there's no reason to forcibly exit.
>> >
>> > So I think we should for now solve this particular issue, and not try
>> > generalizing too much.
>> 
>> To be clear, right now any error anywhere in command-line causes "emacs
>> --fg-daemon" and "emacs --bg-daemon" to hang indefinitely, without
>> printing an error, with no way to ever interact with the Emacs process.
>
> That's not what you said before: you said "anywhere in startup.el",
> which is much more general.  Now you are saying something different.
>
> What exactly is meant by "anywhere in command-line"? the function
> command-line in startup.el? or something else?

The function command-line in startup.el.

Or, more specifically: anywhere during the evaluation of the form in the
variable top-level, which by default is (normal-top-level).  (which
calls (command-line))

>> This error can come from any code, so if we have *any* bugs anywhere in
>> code called from command-line, it will cause Emacs to enter this state.
>
> Why would we assume that *any code* there will signal an error?
> That's like saying that Emacs is a useless program that always signals
> errors in random places.  That's a non-starter here.

We aren't assuming this code in particular will signal an error.  We are
just make this code behave equivalently to other Emacs code, which, when
it throws an error due to a bug, logs that error.

>> We can add good error messages for individual classes of error, but we
>> should also have a catch-all check to make sure that Emacs doesn't enter
>> this broken state if we (or the user) write code which contains a bug.
>
> There's no reason to have a catch-all check where no error is
> expected.  Do you always wrap all of your code in condition-case and
> the likes?  If not, why not?

The reason to have a catch-all is what I just said: if there's an error
in this code, either caused by the user or from a bug in the code, it
causes Emacs to silently hang without logging an error, providing
absolutely no way for the user to know what's going wrong.

You might as well ask why we have a condition-case wrapped around
command_loop_1 which calls cmd_error, instead of just discarding the
error and continuing.

>> I have concrete reasons to want this: I think there's a bug in
>> command-line in trunk which some of my users using emacs --daemon have
>> run into.  But I have zero information about what caused the bug,
>> because Emacs just hangs without printing any error message in this
>> case.
>
> Then please debug that, and let's talk when you do have concrete data.

I can't debug that because I can't reproduce it and the failure case
leaves no information behind.  That's part of the point of why we should
log on error.

Your argument here justifies silencing all errors in Emacs and never
writing them to *Messages*.  That's obviously absurd...

bug#68799: 30.0.50; emacs --fg-daemon fails silently if server-start fails

[Eli Zaretskii]

> From: sbaugh@catern.com
> Date: Sat, 10 Feb 2024 23:23:19 +0000 (UTC)
> Cc: 68799@debbugs.gnu.org, sbaugh@janestreet.com
> 
> > What exactly is meant by "anywhere in command-line"? the function
> > command-line in startup.el? or something else?
> 
> The function command-line in startup.el.
> 
> Or, more specifically: anywhere during the evaluation of the form in the
> variable top-level, which by default is (normal-top-level).  (which
> calls (command-line))

Some of that code already detects errors and takes appropriate
actions.  I'm okay with augmenting the existing error-detecting code
in startup.el with daemon-specific actions, if what we do now is not
TRT for the daemon invocations.  I'm also okay with identifying more
places where error detection and recovery is needed, perhaps only in
the daemon case.  But that must be on a case by case basis, based on
clear understanding what could be the reasons and the effects of those
reasons.  I'm not interested in "catch-all" dumb processing of startup
errors without understanding them, because the recovery code will then
not necessarily be correct and on target.

> The reason to have a catch-all is what I just said: if there's an error
> in this code, either caused by the user or from a bug in the code, it
> causes Emacs to silently hang without logging an error, providing
> absolutely no way for the user to know what's going wrong.

Not true, at least not in all cases.  Once again, the only way to make
progress here that I agree to is one case at a time, based on
understanding the possible reasons and on what we want Emacs (daemon
or not) do in each case.

> You might as well ask why we have a condition-case wrapped around
> command_loop_1 which calls cmd_error, instead of just discarding the
> error and continuing.

The reason for doing it with command_loop_1 is well known, and is not
really relevant to this discussion.

> >> I have concrete reasons to want this: I think there's a bug in
> >> command-line in trunk which some of my users using emacs --daemon have
> >> run into.  But I have zero information about what caused the bug,
> >> because Emacs just hangs without printing any error message in this
> >> case.
> >
> > Then please debug that, and let's talk when you do have concrete data.
> 
> I can't debug that because I can't reproduce it and the failure case
> leaves no information behind.  That's part of the point of why we should
> log on error.

It's okay for you to add code locally to the relevant parts that would
log the necessary information, so you can understand what happens.
Please get back after you understand what went wrong, and we can then
discuss whether such situations could happen with others, and whether
and how to handle them.

> Your argument here justifies silencing all errors in Emacs and never
> writing them to *Messages*.  That's obviously absurd...

Yes, so let's not go "ad absurdum".

bug#68799: 30.0.50; emacs --fg-daemon fails silently if server-start fails

[Spencer Baugh]

Spencer Baugh <sbaugh@janestreet.com> writes:
> 1. emacs -Q --fg-daemon=/nonexistent/dir/sock
> 2. Emacs prints "Starting Emacs daemon." and sits in foreground.
> 3. emacsclient -c -s /nonexistent/dir/sock
> 4. emacsclient prints and exits:
> emacsclient: can't find socket; have you started the server?
> emacsclient: To start the server in Emacs, type "M-x server-start".
> emacsclient: error accessing socket "/nonexistent/dir/sock"
>
> This is because in step 1, the server actually failed to start, but
> Emacs did not log that at all.  In fact, it's impossible to access the
> Emacs started in 1 now, since it's not actually running a server and it
> has no frames.

Okay, I found what one might call the root cause, the following code and
comment.  Adding Stefan and Jason to CC as the original authors.

/* The initial frame is a special non-displaying frame. It
   will be current in daemon mode when there are no frames
   to display, and in non-daemon mode before the real frame
   has finished initializing.  If an error is thrown in the
   latter case while creating the frame, then the frame
   will never be displayed, so the safest thing to do is
   write to stderr and quit.  In daemon mode, there are
   many other potential errors that do not prevent frames
   from being created, so continuing as normal is better in
   that case.  */
|| (!IS_DAEMON && FRAME_INITIAL_P (sf))

The comment is mostly sensible: we should exit while initializing, and
shouldn't exit while in the steady state of daemon mode.

However, it doesn't handle the case when Emacs is initializing *and* in
daemon mode.  In that case, an error will prevent frames from being
created, just like in non-daemon mode.

So this check really wants to be something more like:
|| ( IS_DAEMON && [something to check if Emacs is starting up])
|| (!IS_DAEMON && FRAME_INITIAL_P (sf))

Not sure what [something to check if Emacs is starting up] should be
though.  Maybe it should check that server-process is non-nil and alive?
That would be pretty nice, because then an error which stops the server
would cause the Emacs daemon to log the error to stderr - and otherwise
there's no way to get the error message from such an error.

bug#68799: 30.0.50; emacs --fg-daemon fails silently if server-start fails

[Eli Zaretskii]

> Cc: Stefan Monnier <monnier@iro.umontreal.ca>, Jason Rumney <jasonr@gnu.org>
> From: Spencer Baugh <sbaugh@janestreet.com>
> Date: Mon, 12 Feb 2024 17:10:34 -0500
> 
> Spencer Baugh <sbaugh@janestreet.com> writes:
> > 1. emacs -Q --fg-daemon=/nonexistent/dir/sock
> > 2. Emacs prints "Starting Emacs daemon." and sits in foreground.
> > 3. emacsclient -c -s /nonexistent/dir/sock
> > 4. emacsclient prints and exits:
> > emacsclient: can't find socket; have you started the server?
> > emacsclient: To start the server in Emacs, type "M-x server-start".
> > emacsclient: error accessing socket "/nonexistent/dir/sock"
> >
> > This is because in step 1, the server actually failed to start, but
> > Emacs did not log that at all.  In fact, it's impossible to access the
> > Emacs started in 1 now, since it's not actually running a server and it
> > has no frames.
> 
> Okay, I found what one might call the root cause, the following code and
> comment.  Adding Stefan and Jason to CC as the original authors.
> 
> /* The initial frame is a special non-displaying frame. It
>    will be current in daemon mode when there are no frames
>    to display, and in non-daemon mode before the real frame
>    has finished initializing.  If an error is thrown in the
>    latter case while creating the frame, then the frame
>    will never be displayed, so the safest thing to do is
>    write to stderr and quit.  In daemon mode, there are
>    many other potential errors that do not prevent frames
>    from being created, so continuing as normal is better in
>    that case.  */
> || (!IS_DAEMON && FRAME_INITIAL_P (sf))
> 
> The comment is mostly sensible: we should exit while initializing, and
> shouldn't exit while in the steady state of daemon mode.

Bug#1310 and bug#1836 are also relevant here.

> However, it doesn't handle the case when Emacs is initializing *and* in
> daemon mode.  In that case, an error will prevent frames from being
> created, just like in non-daemon mode.
> 
> So this check really wants to be something more like:
> || ( IS_DAEMON && [something to check if Emacs is starting up])
> || (!IS_DAEMON && FRAME_INITIAL_P (sf))
> 
> Not sure what [something to check if Emacs is starting up] should be
> though.

after-init-time, I guess.  But note that this still leaves a window
between where that is set non-nil and starting the server.

Also, the patch for server-start which prevents it from erroring out
is still needed, because we do want to show the error message about
being unable to start the daemon.

And this solution will still print error messages that are not
specific enough to be helpful.  E.g., what do you do if you try
starting the daemon and see

   wrong-type-argument, stringp, nil

and that's all?  And that is even before we consider the use case
where stderr of the daemon is redirected to the great void, which
happens in some cases.

So my vote is still for diagnosing the important places where a fatal
error could happen, and adding a clear diagnostic there.  Yes, it is
more work.  But the gain will be much higher.

bug#68799: 30.0.50; emacs --fg-daemon fails silently if server-start fails

[Spencer Baugh]

[-- Attachment #1: Type: text/plain, Size: 3462 bytes --]

Eli Zaretskii <eliz@gnu.org> writes:

>> Cc: Stefan Monnier <monnier@iro.umontreal.ca>, Jason Rumney <jasonr@gnu.org>
>> From: Spencer Baugh <sbaugh@janestreet.com>
>> Date: Mon, 12 Feb 2024 17:10:34 -0500
>> 
>> Spencer Baugh <sbaugh@janestreet.com> writes:
>> > 1. emacs -Q --fg-daemon=/nonexistent/dir/sock
>> > 2. Emacs prints "Starting Emacs daemon." and sits in foreground.
>> > 3. emacsclient -c -s /nonexistent/dir/sock
>> > 4. emacsclient prints and exits:
>> > emacsclient: can't find socket; have you started the server?
>> > emacsclient: To start the server in Emacs, type "M-x server-start".
>> > emacsclient: error accessing socket "/nonexistent/dir/sock"
>> >
>> > This is because in step 1, the server actually failed to start, but
>> > Emacs did not log that at all.  In fact, it's impossible to access the
>> > Emacs started in 1 now, since it's not actually running a server and it
>> > has no frames.
>> 
>> Okay, I found what one might call the root cause, the following code and
>> comment.  Adding Stefan and Jason to CC as the original authors.
>> 
>> /* The initial frame is a special non-displaying frame. It
>>    will be current in daemon mode when there are no frames
>>    to display, and in non-daemon mode before the real frame
>>    has finished initializing.  If an error is thrown in the
>>    latter case while creating the frame, then the frame
>>    will never be displayed, so the safest thing to do is
>>    write to stderr and quit.  In daemon mode, there are
>>    many other potential errors that do not prevent frames
>>    from being created, so continuing as normal is better in
>>    that case.  */
>> || (!IS_DAEMON && FRAME_INITIAL_P (sf))
>> 
>> The comment is mostly sensible: we should exit while initializing, and
>> shouldn't exit while in the steady state of daemon mode.
>
> Bug#1310 and bug#1836 are also relevant here.

Indeed.

>> However, it doesn't handle the case when Emacs is initializing *and* in
>> daemon mode.  In that case, an error will prevent frames from being
>> created, just like in non-daemon mode.
>> 
>> So this check really wants to be something more like:
>> || ( IS_DAEMON && [something to check if Emacs is starting up])
>> || (!IS_DAEMON && FRAME_INITIAL_P (sf))
>> 
>> Not sure what [something to check if Emacs is starting up] should be
>> though.
>
> after-init-time, I guess.  But note that this still leaves a window
> between where that is set non-nil and starting the server.

Oh, actually there is a DAEMON_RUNNING define which is exactly what we
want here.  DAEMON_RUNNING just didn't exist at the time the check on
IS_DAEMON was added.  Patch using DAEMON_RUNNING attached.

> Also, the patch for server-start which prevents it from erroring out
> is still needed, because we do want to show the error message about
> being unable to start the daemon.
>
> And this solution will still print error messages that are not
> specific enough to be helpful.  E.g., what do you do if you try
> starting the daemon and see
>
>    wrong-type-argument, stringp, nil
>
> and that's all?  And that is even before we consider the use case
> where stderr of the daemon is redirected to the great void, which
> happens in some cases.
>
> So my vote is still for diagnosing the important places where a fatal
> error could happen, and adding a clear diagnostic there.  Yes, it is
> more work.  But the gain will be much higher.

Agreed, I will also work on that as a separate change.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Check-daemon-is-initialized-before-supressing-errors.patch --]
[-- Type: text/x-patch, Size: 2394 bytes --]

From a24a2b1ceb12f11c9d345190fbf554f27c4ec186 Mon Sep 17 00:00:00 2001
From: Spencer Baugh <sbaugh@janestreet.com>
Date: Tue, 13 Feb 2024 12:20:39 -0500
Subject: [PATCH] Check daemon is initialized before supressing errors

Previously, the default error handler would correctly suppress
unhandled errors raised when IS_DAEMON and the initial frame was
current, since this is the normal state of operation for a daemon-mode
Emacs.  However, this also incorrectly suppressed errors raised while
a daemon-mode Emacs was starting up.

Now, errors raised while a daemon-mode Emacs is starting up will be
handled just like errors when a non-daemon Emacs is starting up.

This was previously the case before changes for bug#1310 and bug#1836,
which added the suppression of errors when IS_DAEMON.  DAEMON_RUNNING
didn't exist at the time of those changes, but now it does, so we can
do better.

* src/keyboard.c (Fcommand_error_default_function): Check
!DAEMON_RUNNING in addition to IS_DAEMON. (bug#68799)
* src/lisp.h (DAEMON_RUNNING): Add a clarifying comment about what
this define means.
---
 src/keyboard.c | 5 +++--
 src/lisp.h     | 1 +
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/keyboard.c b/src/keyboard.c
index ff89b1b3510..5c00996ea6d 100644
--- a/src/keyboard.c
+++ b/src/keyboard.c
@@ -1076,8 +1076,9 @@ DEFUN ("command-error-default-function", Fcommand_error_default_function,
 	     write to stderr and quit.  In daemon mode, there are
 	     many other potential errors that do not prevent frames
 	     from being created, so continuing as normal is better in
-	     that case.  */
-	  || (!IS_DAEMON && FRAME_INITIAL_P (sf))
+	     that case, as long as the daemon has actually finished
+	     initialization. */
+	  || (!(IS_DAEMON && !DAEMON_RUNNING) && FRAME_INITIAL_P (sf))
 	  || noninteractive))
     {
       print_error_message (data, Qexternal_debugging_output,
diff --git a/src/lisp.h b/src/lisp.h
index 5326824bf38..176ca24b318 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -5038,6 +5038,7 @@ fast_c_string_match_ignore_case (Lisp_Object regexp,
 /* 0 not a daemon, 1 foreground daemon, 2 background daemon.  */
 extern int daemon_type;
 #define IS_DAEMON (daemon_type != 0)
+/* True means daemon-initialized has not yet been called. */
 #define DAEMON_RUNNING (daemon_type >= 0)
 #else  /* WINDOWSNT */
 extern void *w32_daemon_event;
-- 
2.39.3

bug#68799: 30.0.50; emacs --fg-daemon fails silently if server-start fails

[Eli Zaretskii]

> From: Spencer Baugh <sbaugh@janestreet.com>
> Cc: 68799@debbugs.gnu.org,  monnier@iro.umontreal.ca,  jasonr@gnu.org
> Date: Tue, 13 Feb 2024 12:37:30 -0500
> 
> >> So this check really wants to be something more like:
> >> || ( IS_DAEMON && [something to check if Emacs is starting up])
> >> || (!IS_DAEMON && FRAME_INITIAL_P (sf))
> >> 
> >> Not sure what [something to check if Emacs is starting up] should be
> >> though.
> >
> > after-init-time, I guess.  But note that this still leaves a window
> > between where that is set non-nil and starting the server.
> 
> Oh, actually there is a DAEMON_RUNNING define which is exactly what we
> want here.  DAEMON_RUNNING just didn't exist at the time the check on
> IS_DAEMON was added.  Patch using DAEMON_RUNNING attached.

Using DAEMON_RUNNING would mean any errors while starting the server
will fail to show the explicit error message about that.  That's a
regression in my book.  So I hope we can find a better solution.

bug#68799: 30.0.50; emacs --fg-daemon fails silently if server-start fails

[Spencer Baugh]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Spencer Baugh <sbaugh@janestreet.com>
>> Cc: 68799@debbugs.gnu.org,  monnier@iro.umontreal.ca,  jasonr@gnu.org
>> Date: Tue, 13 Feb 2024 12:37:30 -0500
>> 
>> >> So this check really wants to be something more like:
>> >> || ( IS_DAEMON && [something to check if Emacs is starting up])
>> >> || (!IS_DAEMON && FRAME_INITIAL_P (sf))
>> >> 
>> >> Not sure what [something to check if Emacs is starting up] should be
>> >> though.
>> >
>> > after-init-time, I guess.  But note that this still leaves a window
>> > between where that is set non-nil and starting the server.
>> 
>> Oh, actually there is a DAEMON_RUNNING define which is exactly what we
>> want here.  DAEMON_RUNNING just didn't exist at the time the check on
>> IS_DAEMON was added.  Patch using DAEMON_RUNNING attached.
>
> Using DAEMON_RUNNING would mean any errors while starting the server
> will fail to show the explicit error message about that.  That's a
> regression in my book.  So I hope we can find a better solution.

Could you be more specific about what would regress in this situation?
Preferably with a step by step of something that behaves worse.

Currently the error message doesn't get shown at all, so I'm not clear
what could be regressing.

bug#68799: 30.0.50; emacs --fg-daemon fails silently if server-start fails

[Eli Zaretskii]

> From: Spencer Baugh <sbaugh@janestreet.com>
> Cc: 68799@debbugs.gnu.org,  monnier@iro.umontreal.ca,  jasonr@gnu.org
> Date: Tue, 13 Feb 2024 13:04:24 -0500
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Using DAEMON_RUNNING would mean any errors while starting the server
> > will fail to show the explicit error message about that.  That's a
> > regression in my book.  So I hope we can find a better solution.
> 
> Could you be more specific about what would regress in this situation?
> Preferably with a step by step of something that behaves worse.

I mean this error message:

  (let ((dn (daemonp)))
    (when dn
      (when (stringp dn) (setq server-name dn))
      (server-start)
      (if server-process
	  (daemon-initialized)
	(if (stringp dn)
	    (message
	     "Unable to start daemon: Emacs server named %S already running"
	     server-name)
	  (message "Unable to start the daemon.\nAnother instance of Emacs is running the server, either as daemon or interactively.\nYou can use emacsclient to connect to that Emacs process."))
	(kill-emacs 1))))

> Currently the error message doesn't get shown at all, so I'm not clear
> what could be regressing.

We already established why the message doesn't show: because errors in
server-start are not caught.  I thought we also agreed that catching
the errors there and letting startup.el show the above error messages
is something that should be done.  I think your patch will prevent us
from doing that because the code which you suggest to patch with
DAEMON_RUNNING will kill Emacs before startup.el gets the chance to
show the error message, because daemon-initialized has not yet been
run by that time.

bug#68799: 30.0.50; emacs --fg-daemon fails silently if server-start fails

[Spencer Baugh]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Spencer Baugh <sbaugh@janestreet.com>
>> Cc: 68799@debbugs.gnu.org,  monnier@iro.umontreal.ca,  jasonr@gnu.org
>> Date: Tue, 13 Feb 2024 13:04:24 -0500
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> > Using DAEMON_RUNNING would mean any errors while starting the server
>> > will fail to show the explicit error message about that.  That's a
>> > regression in my book.  So I hope we can find a better solution.
>> 
>> Could you be more specific about what would regress in this situation?
>> Preferably with a step by step of something that behaves worse.
>
> I mean this error message:
>
>   (let ((dn (daemonp)))
>     (when dn
>       (when (stringp dn) (setq server-name dn))
>       (server-start)
>       (if server-process
> 	  (daemon-initialized)
> 	(if (stringp dn)
> 	    (message
> 	     "Unable to start daemon: Emacs server named %S already running"
> 	     server-name)
> 	  (message "Unable to start the daemon.\nAnother instance of Emacs is running the server, either as daemon or interactively.\nYou can use emacsclient to connect to that Emacs process."))
> 	(kill-emacs 1))))

Yes.  With my patch, those messages will still show in exactly the same
cases.  No regression.

>> Currently the error message doesn't get shown at all, so I'm not clear
>> what could be regressing.
>
> We already established why the message doesn't show: because errors in
> server-start are not caught.  I thought we also agreed that catching
> the errors there and letting startup.el show the above error messages
> is something that should be done.

Yes.

> I think your patch will prevent us from doing that because the code
> which you suggest to patch with DAEMON_RUNNING will kill Emacs before
> startup.el gets the chance to show the error message, because
> daemon-initialized has not yet been run by that time.

Nope, it won't prevent us from doing that.  My patch affects only
*uncaught* errors, and improves behavior when errors are uncaught.  The
separate change we will make to the code you posted is to *catch more
errors*.  A change to the behavior for uncaught errors doesn't interfere
with us deciding to catch more errors.  Defining an explicit error
handler, of course, completely overrides the default handle for uncaught
errors.  So they're totally separate patches.

bug#68799: 30.0.50; emacs --fg-daemon fails silently if server-start fails

[Eli Zaretskii]

> From: Spencer Baugh <sbaugh@janestreet.com>
> Cc: 68799@debbugs.gnu.org,  monnier@iro.umontreal.ca,  jasonr@gnu.org
> Date: Tue, 13 Feb 2024 15:02:00 -0500
> 
> > I think your patch will prevent us from doing that because the code
> > which you suggest to patch with DAEMON_RUNNING will kill Emacs before
> > startup.el gets the chance to show the error message, because
> > daemon-initialized has not yet been run by that time.
> 
> Nope, it won't prevent us from doing that.  My patch affects only
> *uncaught* errors, and improves behavior when errors are uncaught.  The
> separate change we will make to the code you posted is to *catch more
> errors*.  A change to the behavior for uncaught errors doesn't interfere
> with us deciding to catch more errors.  Defining an explicit error
> handler, of course, completely overrides the default handle for uncaught
> errors.  So they're totally separate patches.

Then let's see them together, so we understand what happens in each
and every case.  This bug started from a silent failure to start the
server, so its solution should solve at least that.

bug#68799: 30.0.50; emacs --fg-daemon fails silently if server-start fails

[Spencer Baugh]

Eli Zaretskii <eliz@gnu.org> writes:
>> From: Spencer Baugh <sbaugh@janestreet.com>
>> Cc: 68799@debbugs.gnu.org,  monnier@iro.umontreal.ca,  jasonr@gnu.org
>> Date: Tue, 13 Feb 2024 15:02:00 -0500
>> 
>> > I think your patch will prevent us from doing that because the code
>> > which you suggest to patch with DAEMON_RUNNING will kill Emacs before
>> > startup.el gets the chance to show the error message, because
>> > daemon-initialized has not yet been run by that time.
>> 
>> Nope, it won't prevent us from doing that.  My patch affects only
>> *uncaught* errors, and improves behavior when errors are uncaught.  The
>> separate change we will make to the code you posted is to *catch more
>> errors*.  A change to the behavior for uncaught errors doesn't interfere
>> with us deciding to catch more errors.  Defining an explicit error
>> handler, of course, completely overrides the default handle for uncaught
>> errors.  So they're totally separate patches.
>
> Then let's see them together, so we understand what happens in each
> and every case.  This bug started from a silent failure to start the
> server, so its solution should solve at least that.

To be clear, the patch I posted which checks DAEMON_RUNNING *does* solve
that.

$ ./src/emacs -Q --fg-daemon=/tmp/foo
Starting Emacs daemon.
‘/tmp’ is not a safe directory because it is not owned by you (owner = root (0))
$ 

(which previously would have hung)

The issue now is solely that the printed error message is maybe not
informative enough.

So how about the following diff, which changes the output to:

$ ./src/emacs -Q --fg-daemon=/tmp/foo
Starting Emacs daemon.
Unable to start daemon: ‘/tmp’ is not a safe directory because it is not owned by you (owner = root (0))
$ 

diff --git a/lisp/startup.el b/lisp/startup.el
index 773765a4b97..d1420a3f9c5 100644
--- a/lisp/startup.el
+++ b/lisp/startup.el
@@ -1627,7 +1627,9 @@ command-line
   (let ((dn (daemonp)))
     (when dn
       (when (stringp dn) (setq server-name dn))
-      (server-start)
+      (condition-case err
+          (server-start)
+        (error (error "Unable to start daemon: %s" (error-message-string err))))
       (if server-process
 	  (daemon-initialized)
 	(if (stringp dn)

bug#68799: 30.0.50; emacs --fg-daemon fails silently if server-start fails

[Eli Zaretskii]

> From: Spencer Baugh <sbaugh@janestreet.com>
> Cc: 68799@debbugs.gnu.org,  monnier@iro.umontreal.ca,  jasonr@gnu.org
> Date: Tue, 13 Feb 2024 15:20:21 -0500
> 
> To be clear, the patch I posted which checks DAEMON_RUNNING *does* solve
> that.
> 
> $ ./src/emacs -Q --fg-daemon=/tmp/foo
> Starting Emacs daemon.
> ‘/tmp’ is not a safe directory because it is not owned by you (owner = root (0))
> $ 
> 
> (which previously would have hung)
> 
> The issue now is solely that the printed error message is maybe not
> informative enough.
> 
> So how about the following diff, which changes the output to:
> 
> $ ./src/emacs -Q --fg-daemon=/tmp/foo
> Starting Emacs daemon.
> Unable to start daemon: ‘/tmp’ is not a safe directory because it is not owned by you (owner = root (0))

That's an improvement, but maybe it will be even better if you append
"; exiting" at the end of the message?

bug#68799: 30.0.50; emacs --fg-daemon fails silently if server-start fails

[Spencer Baugh]

[-- Attachment #1: Type: text/plain, Size: 1102 bytes --]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Spencer Baugh <sbaugh@janestreet.com>
>> Cc: 68799@debbugs.gnu.org,  monnier@iro.umontreal.ca,  jasonr@gnu.org
>> Date: Tue, 13 Feb 2024 15:20:21 -0500
>> 
>> To be clear, the patch I posted which checks DAEMON_RUNNING *does* solve
>> that.
>> 
>> $ ./src/emacs -Q --fg-daemon=/tmp/foo
>> Starting Emacs daemon.
>> ‘/tmp’ is not a safe directory because it is not owned by you (owner = root (0))
>> $ 
>> 
>> (which previously would have hung)
>> 
>> The issue now is solely that the printed error message is maybe not
>> informative enough.
>> 
>> So how about the following diff, which changes the output to:
>> 
>> $ ./src/emacs -Q --fg-daemon=/tmp/foo
>> Starting Emacs daemon.
>> Unable to start daemon: ‘/tmp’ is not a safe directory because it is not owned by you (owner = root (0))
>
> That's an improvement, but maybe it will be even better if you append
> "; exiting" at the end of the message?

Can do.  Here's the patch, which should be applied in addition to the
DAEMON_RUNNING patch I posted earlier.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Add-context-to-errors-thrown-by-server-start-during-.patch --]
[-- Type: text/x-patch, Size: 1137 bytes --]

From d65f34b76c4d4496bdb2c53bede45db1869af232 Mon Sep 17 00:00:00 2001
From: Spencer Baugh <sbaugh@janestreet.com>
Date: Wed, 14 Feb 2024 11:09:33 -0500
Subject: [PATCH] Add context to errors thrown by server-start during startup

When server-start errors during startup, the error is printed to
the terminal without context.  To help the user understand
better what went wrong, that printed error now mentions that the
error came from starting up the daemon.

* lisp/startup.el (command-line): Catch and annotate errors
thrown by server-start.  (bug#68799)
---
 lisp/startup.el | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/lisp/startup.el b/lisp/startup.el
index 773765a4b97..4871cf3d21f 100644
--- a/lisp/startup.el
+++ b/lisp/startup.el
@@ -1627,7 +1627,9 @@ command-line
   (let ((dn (daemonp)))
     (when dn
       (when (stringp dn) (setq server-name dn))
-      (server-start)
+      (condition-case err
+          (server-start)
+        (error (error "Unable to start daemon: %s; exiting" (error-message-string err))))
       (if server-process
 	  (daemon-initialized)
 	(if (stringp dn)
-- 
2.39.3

bug#68799: 30.0.50; emacs --fg-daemon fails silently if server-start fails

[Eli Zaretskii]

> From: Spencer Baugh <sbaugh@janestreet.com>
> Cc: 68799@debbugs.gnu.org,  monnier@iro.umontreal.ca,  jasonr@gnu.org
> Date: Wed, 14 Feb 2024 11:11:10 -0500
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> From: Spencer Baugh <sbaugh@janestreet.com>
> >> Cc: 68799@debbugs.gnu.org,  monnier@iro.umontreal.ca,  jasonr@gnu.org
> >> Date: Tue, 13 Feb 2024 15:20:21 -0500
> >> 
> >> To be clear, the patch I posted which checks DAEMON_RUNNING *does* solve
> >> that.
> >> 
> >> $ ./src/emacs -Q --fg-daemon=/tmp/foo
> >> Starting Emacs daemon.
> >> ‘/tmp’ is not a safe directory because it is not owned by you (owner = root (0))
> >> $ 
> >> 
> >> (which previously would have hung)
> >> 
> >> The issue now is solely that the printed error message is maybe not
> >> informative enough.
> >> 
> >> So how about the following diff, which changes the output to:
> >> 
> >> $ ./src/emacs -Q --fg-daemon=/tmp/foo
> >> Starting Emacs daemon.
> >> Unable to start daemon: ‘/tmp’ is not a safe directory because it is not owned by you (owner = root (0))
> >
> > That's an improvement, but maybe it will be even better if you append
> > "; exiting" at the end of the message?
> 
> Can do.  Here's the patch, which should be applied in addition to the
> DAEMON_RUNNING patch I posted earlier.

Thanks, installed.

bug#68799: 30.0.50; emacs --fg-daemon fails silently if server-start fails

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>> 1. emacs -Q --fg-daemon=/nonexistent/dir/sock
>> 2. Emacs prints "Starting Emacs daemon." and sits in foreground.
[...]
>> This is because in step 1, the server actually failed to start, but
>> Emacs did not log that at all.  In fact, it's impossible to access the
>> Emacs started in 1 now, since it's not actually running a server and it
>> has no frames.

The problem you point out later is real, but maybe more importantly we
should make sure that any error gets printed at step 2.


        Stefan

bug#68799: 30.0.50; emacs --fg-daemon fails silently if server-start fails

[Björn Bidar via Bug reports for GNU Emacs, the Swiss army knife of text editors]

A side effect of this bug and another is that when emacs --fg-daemon is
used e.g. in combination with a systemd service. If the service
takes to long to be ready when a user calls emacsclient that it using
the standard desktop file it creates another daemon which will then
make the Emacs started with --fg-daemon fail resulting in the emacs in
the systemd service fail in circles.

bug#68799: 30.0.50; emacs --fg-daemon fails silently if server-start fails

[Eli Zaretskii]

> Cc: 68799@debbugs.gnu.org
> Date: Tue, 13 Feb 2024 23:30:41 +0200
> From:  Björn Bidar via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> A side effect of this bug and another is that when emacs --fg-daemon is
> used e.g. in combination with a systemd service. If the service
> takes to long to be ready when a user calls emacsclient that it using
> the standard desktop file it creates another daemon which will then
> make the Emacs started with --fg-daemon fail resulting in the emacs in
> the systemd service fail in circles.

Please tell more details about the scenario you have in mind, because
I don't think I follow.  In particular, why would starting the daemon
take too long? due to what problems?  And how is user invoking
emacsclient relevant in this case?  AFAIU, systemd starts Emacs at
system start or user login time, whereas emacsclient is invoked when
the user wants to run some command or clicks a desktop shortcut, and
those events are not adjacent in time.

I'm probably missing something here.

Thanks.

bug#68799: 30.0.50; emacs --fg-daemon fails silently if server-start fails

[Björn Bidar via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:

>> Cc: 68799@debbugs.gnu.org
>> Date: Tue, 13 Feb 2024 23:30:41 +0200
>> From:  Björn Bidar via "Bug reports for GNU Emacs,
>>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
>> 
>> A side effect of this bug and another is that when emacs --fg-daemon is
>> used e.g. in combination with a systemd service. If the service
>> takes to long to be ready when a user calls emacsclient that it using
>> the standard desktop file it creates another daemon which will then
>> make the Emacs started with --fg-daemon fail resulting in the emacs in
>> the systemd service fail in circles.
>
> Please tell more details about the scenario you have in mind, because
> I don't think I follow.  In particular, why would starting the daemon
> take too long? due to what problems?  And how is user invoking
> emacsclient relevant in this case?  AFAIU, systemd starts Emacs at
> system start or user login time, whereas emacsclient is invoked when
> the user wants to run some command or clicks a desktop shortcut, and
> those events are not adjacent in time.
>
0. User logs in
1. The session reaches graphical-session.target
2. Emacs executed in emacs.service 
3. Emacs launches with all it's modes loaded, desktop-mode session
restored etc.
4. Emacs is ready to receive commands

When the user starts emacsclient before step 4 is reached then it can
timeout because it can take some time till Emacs is ready.

Step 1. Starts all parts of the programs which require a graphical
session including the desktop environment that the user is likely to
call upon emacsclient.

bug#68799: 30.0.50; emacs --fg-daemon fails silently if server-start fails

[Eli Zaretskii]

> From: Björn Bidar <bjorn.bidar@thaodan.de>
> Cc: sbaugh@janestreet.com,  68799@debbugs.gnu.org
> Date: Wed, 14 Feb 2024 17:10:39 +0200
> 
> 0. User logs in
> 1. The session reaches graphical-session.target
> 2. Emacs executed in emacs.service 
> 3. Emacs launches with all it's modes loaded, desktop-mode session
> restored etc.
> 4. Emacs is ready to receive commands
> 
> When the user starts emacsclient before step 4 is reached then it can
> timeout because it can take some time till Emacs is ready.

Wouldn't this be a cockpit mistake?  The user presumably knows that
loading desktop takes some time, and should give Emacs that time
before invoking emacsclient.  No?

bug#68799: 30.0.50; emacs --fg-daemon fails silently if server-start fails

[Björn Bidar via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Björn Bidar <bjorn.bidar@thaodan.de>
>> Cc: sbaugh@janestreet.com,  68799@debbugs.gnu.org
>> Date: Wed, 14 Feb 2024 17:10:39 +0200
>> 
>> 0. User logs in
>> 1. The session reaches graphical-session.target
>> 2. Emacs executed in emacs.service 
>> 3. Emacs launches with all it's modes loaded, desktop-mode session
>> restored etc.
>> 4. Emacs is ready to receive commands
>> 
>> When the user starts emacsclient before step 4 is reached then it can
>> timeout because it can take some time till Emacs is ready.
>
> Wouldn't this be a cockpit mistake?  The user presumably knows that
> loading desktop takes some time, and should give Emacs that time
> before invoking emacsclient.  No?

Of course but it the user doesn't know how exactly Emacs takes to start
and to avoid race conditions it would make sense to wait.