Re: master 2c79a8f 2/2: Use posix_spawn if possible.

Re: master 2c79a8f 2/2: Use posix_spawn if possible.

[Eli Zaretskii]

I'm sorry, but I reverted this commit and the previous one.  They
broke the MinGW build in fundamental ways, and I cannot afford having
the master branch broken so frequently, definitely not on weekends,
which are the only time I can work seriously on Emacs.

If you cannot make sure Emacs still builds on all supported platforms,
please either post your patches first, or push them to a scratch
branch, and ask people who have access to those platforms to test it
before the changes land on master.

Btw, regarding use of posix_spawn, I'd expect a discussion before we
make such a change.  AFAIU it is not a trivial decision, as
posix_spawn has its down sides, and therefore is not necessarily the
best API for running sub-processes on every supported platform, even
if you consider only the Posix ones.  We should consider the
advantages and disadvantages before we make the decision.

Thanks.

Re: master 2c79a8f 2/2: Use posix_spawn if possible.

[Philipp Stephani]

Am Fr., 25. Dez. 2020 um 14:17 Uhr schrieb Eli Zaretskii <eliz@gnu.org>:
>
> I'm sorry, but I reverted this commit and the previous one.  They
> broke the MinGW build in fundamental ways, and I cannot afford having
> the master branch broken so frequently, definitely not on weekends,
> which are the only time I can work seriously on Emacs.

OK.

>
> If you cannot make sure Emacs still builds on all supported platforms,

Hmm, that's indeed a rather challenging request. Don't we support like
dozens of platforms, including several nonfree operating systems? It
would be rather difficult to contribute changes if the authors needed
to test them on all supported platforms, especially in areas of the
code with lots of conditional compilation.
How do others deal with this? Do people run Windows VMs and try to
build Emacs on them before pushing?

> please either post your patches first, or push them to a scratch
> branch, and ask people who have access to those platforms to test it
> before the changes land on master.

OK, I've now done that (branch scratch/posix-spawn).

>
> Btw, regarding use of posix_spawn, I'd expect a discussion before we
> make such a change.  AFAIU it is not a trivial decision, as
> posix_spawn has its down sides, and therefore is not necessarily the
> best API for running sub-processes on every supported platform, even
> if you consider only the Posix ones.  We should consider the
> advantages and disadvantages before we make the decision.

Sure, I'm happy to have that discussion. I briefly reviewed the
posix_spawn implementation of GNU libc and Gnulib, and found that it
uses vfork/clone + execve like our hand-rolled code, so I wouldn't
expect any significant change. The primary advantage is to offload
complexity into a library that can properly deal with system-specific
issues and can improve over time. For example, on Linux, posix_spawn
can use clone instead of vfork. On Windows, posix_spawn could directly
call CreateProcess (though Gnulib doesn't implement that yet).

Re: master 2c79a8f 2/2: Use posix_spawn if possible.

[Eli Zaretskii]

> From: Philipp Stephani <p.stephani2@gmail.com>
> Date: Sat, 26 Dec 2020 12:26:24 +0100
> Cc: Emacs developers <emacs-devel@gnu.org>
> 
> > If you cannot make sure Emacs still builds on all supported platforms,
> 
> Hmm, that's indeed a rather challenging request. Don't we support like
> dozens of platforms, including several nonfree operating systems?

No, I mean only the main ones: GNU/Linux, some *BSD, macOS,
MS-Windows.

> How do others deal with this? Do people run Windows VMs and try to
> build Emacs on them before pushing?

Sometimes, yes.  Other times, users of those systems are requested to
text the patch before landing it.

But many of the breaking changes can be found by simple inspection.
For example, in the case in point, if we are not going to use
posix_spawn on MS-Windows, we need to augment nt/mingw-cfg.site and/or
nt/gnulib-cfg.mk to avoid compiling the respective Gnulib modules,
since doing that just risks causing trouble (as happened in this
case).  Also, since you deduced (correctly) that the Gnulib
posix_spawn module cannot be used on MS-Windows, it isn't enough to
have a run-time condition for bypassing that, you need to #ifdef away
the relevant code, because otherwise Emacs might fail to link for no
good reason.

> > please either post your patches first, or push them to a scratch
> > branch, and ask people who have access to those platforms to test it
> > before the changes land on master.
> 
> OK, I've now done that (branch scratch/posix-spawn).

Thanks.  Would people please try that on various platforms and report
any problems?

> > Btw, regarding use of posix_spawn, I'd expect a discussion before we
> > make such a change.  AFAIU it is not a trivial decision, as
> > posix_spawn has its down sides, and therefore is not necessarily the
> > best API for running sub-processes on every supported platform, even
> > if you consider only the Posix ones.  We should consider the
> > advantages and disadvantages before we make the decision.
> 
> Sure, I'm happy to have that discussion. I briefly reviewed the
> posix_spawn implementation of GNU libc and Gnulib, and found that it
> uses vfork/clone + execve like our hand-rolled code, so I wouldn't
> expect any significant change. The primary advantage is to offload
> complexity into a library that can properly deal with system-specific
> issues and can improve over time. For example, on Linux, posix_spawn
> can use clone instead of vfork.

See Savannah bug #59093 for one subtle issue:

  https://savannah.gnu.org/bugs/?59093

Since Emacs also sets its stack limit in some cases, this could be
directly relevant to us.  (But I didn't look into it close enough to
tell whether it actually is relevant.)

> On Windows, posix_spawn could directly call CreateProcess (though
> Gnulib doesn't implement that yet).

FYI: there's a general problem with using Gnulib code on MS-Windows
for anything in Emacs that involves file names.  Emacs on MS-Windows
uses UTF-8 encoded file names, which allows us to support 'char *'
strings as file names outside of the domain of the current system
codepage.  This works by basically wrapping any API that accepts file
names with our own code that converts file names to UTF-16, and then
invokes Windows APIs which accept 'wchar_t *' "wide" strings.  Gnulib
doesn't have this feature, it just calls Windows APIs assuming the
'char *' strings as file names will be correctly interpreted -- which
doesn't work with UTF-8 encoded file names.

Running subprocesses definitely manipulates file names, so the above
issue is very relevant here.

Re: master 2c79a8f 2/2: Use posix_spawn if possible.

[Eli Zaretskii]

> Date: Sat, 26 Dec 2020 14:08:11 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: emacs-devel@gnu.org
> 
> See Savannah bug #59093 for one subtle issue:
> 
>   https://savannah.gnu.org/bugs/?59093
> 
> Since Emacs also sets its stack limit in some cases, this could be
> directly relevant to us.  (But I didn't look into it close enough to
> tell whether it actually is relevant.)

Btw, given this condition for using posix_spawn:

  +  /* `posix_spawn' doesn't yet support setting up pseudoterminals, so
  +     we fall back to `vfork' if we're supposed to use a
  +     pseudoterminal.  */
  +
  +  bool use_posix_spawn = can_use_posix_spawn && pty == NULL;

I understand that the absolute majority of subprocesses on GNU and
Posix platforms will not use posix_spawn, because we usually do use
PTYs?  If so, one wonders why it is a good idea to complicate our code
and make it more dependent on Gnulib, for such small gains?  Should we
perhaps wait until posix_spawn does support PTYs?

Re: master 2c79a8f 2/2: Use posix_spawn if possible.

[Philipp Stephani]

(I'm still trying to set up some emulated Windows installation, but
haven't really been successful so far.)

Am Sa., 26. Dez. 2020 um 13:08 Uhr schrieb Eli Zaretskii <eliz@gnu.org>:
>
> But many of the breaking changes can be found by simple inspection.
> For example, in the case in point, if we are not going to use
> posix_spawn on MS-Windows, we need to augment nt/mingw-cfg.site and/or
> nt/gnulib-cfg.mk to avoid compiling the respective Gnulib modules,
> since doing that just risks causing trouble (as happened in this
> case).  Also, since you deduced (correctly) that the Gnulib
> posix_spawn module cannot be used on MS-Windows, it isn't enough to
> have a run-time condition for bypassing that, you need to #ifdef away
> the relevant code, because otherwise Emacs might fail to link for no
> good reason.

Gnulib provides a stub definition for posix_spawn on Windows as well
(in fact, as of a few days ago, it provides a non-stub
implementation), so linking shouldn't fail. Also compiling the stub
implementation on Windows shouldn't hurt; if it's never called the
linker will likely remove the stub.
What error messages do you see on Windows when trying to build the branch?

> > > Btw, regarding use of posix_spawn, I'd expect a discussion before we
> > > make such a change.  AFAIU it is not a trivial decision, as
> > > posix_spawn has its down sides, and therefore is not necessarily the
> > > best API for running sub-processes on every supported platform, even
> > > if you consider only the Posix ones.  We should consider the
> > > advantages and disadvantages before we make the decision.
> >
> > Sure, I'm happy to have that discussion. I briefly reviewed the
> > posix_spawn implementation of GNU libc and Gnulib, and found that it
> > uses vfork/clone + execve like our hand-rolled code, so I wouldn't
> > expect any significant change. The primary advantage is to offload
> > complexity into a library that can properly deal with system-specific
> > issues and can improve over time. For example, on Linux, posix_spawn
> > can use clone instead of vfork.
>
> See Savannah bug #59093 for one subtle issue:
>
>   https://savannah.gnu.org/bugs/?59093
>
> Since Emacs also sets its stack limit in some cases, this could be
> directly relevant to us.  (But I didn't look into it close enough to
> tell whether it actually is relevant.)

I think that specific problem isn't relevant (we don't change the
stack size between fork and exec), but the fix to
https://debbugs.gnu.org/cgi/bugreport.cgi?bug=24869 (ironically
reported by me) is.
As indicated in https://debbugs.gnu.org/cgi/bugreport.cgi?bug=24869#8,
it might be better to solve the underlying problem in a different way
that doesn't involve changing resource limits (changing a
process-global setting is somewhat fishy anyway, especially with
modules). Essentially we just need to make sure to not add file
descriptors larger than FD_SETSIZE to an fd_set.

>
> > On Windows, posix_spawn could directly call CreateProcess (though
> > Gnulib doesn't implement that yet).
>
> FYI: there's a general problem with using Gnulib code on MS-Windows
> for anything in Emacs that involves file names.  Emacs on MS-Windows
> uses UTF-8 encoded file names, which allows us to support 'char *'
> strings as file names outside of the domain of the current system
> codepage.  This works by basically wrapping any API that accepts file
> names with our own code that converts file names to UTF-16, and then
> invokes Windows APIs which accept 'wchar_t *' "wide" strings.  Gnulib
> doesn't have this feature, it just calls Windows APIs assuming the
> 'char *' strings as file names will be correctly interpreted -- which
> doesn't work with UTF-8 encoded file names.
>
> Running subprocesses definitely manipulates file names, so the above
> issue is very relevant here.

Agreed. It would be really nice if Gnulib adopted the Emacs approach
(probably hidden behind some flag).

Re: master 2c79a8f 2/2: Use posix_spawn if possible.

[Philipp Stephani]

Am Sa., 26. Dez. 2020 um 13:16 Uhr schrieb Eli Zaretskii <eliz@gnu.org>:
>
> > Date: Sat, 26 Dec 2020 14:08:11 +0200
> > From: Eli Zaretskii <eliz@gnu.org>
> > Cc: emacs-devel@gnu.org
> >
> > See Savannah bug #59093 for one subtle issue:
> >
> >   https://savannah.gnu.org/bugs/?59093
> >
> > Since Emacs also sets its stack limit in some cases, this could be
> > directly relevant to us.  (But I didn't look into it close enough to
> > tell whether it actually is relevant.)
>
> Btw, given this condition for using posix_spawn:
>
>   +  /* `posix_spawn' doesn't yet support setting up pseudoterminals, so
>   +     we fall back to `vfork' if we're supposed to use a
>   +     pseudoterminal.  */
>   +
>   +  bool use_posix_spawn = can_use_posix_spawn && pty == NULL;
>
> I understand that the absolute majority of subprocesses on GNU and
> Posix platforms will not use posix_spawn, because we usually do use
> PTYs?  If so, one wonders why it is a good idea to complicate our code
> and make it more dependent on Gnulib, for such small gains?  Should we
> perhaps wait until posix_spawn does support PTYs?

I think posix_spawn supports TTYs well enough by now. From what I see
in the code we perform 5 TTY-related actions between fork and exec:
1. setsid. This is now supported by posix_spawn at least on GNU/Linux
and macOS, and IIUC will be in the next POSIX standard.
2. Making the TTY the controlling terminal of the process. IIUC we
first try using TIOCSCTTY, and then opening the TTY file without
O_NOCTTY. On GNU/Linux, the former one probably fails because stdin is
-1. So we could only do the latter one, which is supported by
posix_spawn (since it's just opening a file).
3. Some terminal setup (setting attributes, loading a line discipline,
etc.). We should be able to do that in the parent process as well,
assuming we can define USG_SUBTTY_WORKS (see below).
4. setpgid. That shouldn't be necessary and probably actually fails
due to the setsid before.
5. tcsetpgrp. That also shouldn't be necessary, because after the
setsid, we have a single process in a single process group in the
session.
I experimented (on GNU/Linux) with posix_spawn for TTYs as well, and
it seems to work fine (I played around with M-x term a bit).
One thing that I'm wondering about is the comment in process.c:
      /* On most USG systems it does not work to open the pty's tty
here, then close it and reopen it in the child.  */
That's an old comment, and I'm not sure it's true any more. When
patching configure.ac so that it defines USG_SUBTTY_WORKS on GNU/Linux
as well, then everything still seems to work fine.

Re: master 2c79a8f 2/2: Use posix_spawn if possible.

[Eli Zaretskii]

> From: Philipp Stephani <p.stephani2@gmail.com>
> Date: Tue, 29 Dec 2020 17:29:59 +0100
> Cc: Emacs developers <emacs-devel@gnu.org>
> 
> (I'm still trying to set up some emulated Windows installation, but
> haven't really been successful so far.)

Thank you for your efforts.

> Gnulib provides a stub definition for posix_spawn on Windows as well
> (in fact, as of a few days ago, it provides a non-stub
> implementation), so linking shouldn't fail. Also compiling the stub
> implementation on Windows shouldn't hurt; if it's never called the
> linker will likely remove the stub.

Based on bitter experience, I doubt that, but we shall see.

> What error messages do you see on Windows when trying to build the branch?

The below is only for compiling Gnulib; the build process stopped
after that, so what will happen in lib-src/ and src/ is still TBD:

    CC       getdtablesize.o
  getdtablesize.c:58:1: warning: no previous prototype for 'getdtablesize' [-Wmissing-prototypes]
     58 | getdtablesize (void)
	| ^~~~~~~~~~~~~
    CC       open.o
  open.c: In function 'sys_open':
  open.c:131:48: error: 'O_CLOEXEC' undeclared (first use in this function)
    131 |                   flags & ~(have_cloexec < 0 ? O_CLOEXEC : 0), mode);
	|                                                ^~~~~~~~~
  open.c:131:48: note: each undeclared identifier is reported only once for each function it appears in
  Makefile:95: recipe for target `open.o' failed
  make[1]: *** [open.o] Error 1
    CC       spawn_faction_adddup2.o
  spawn_faction_adddup2.c: In function 'rpl_posix_spawn_file_actions_adddup2':
  spawn_faction_adddup2.c:26:30: warning: implicit declaration of function 'getdtablesize'; did you mean 'getpagesize'? [-Wimplicit-function-declaration]
     26 | # define __sysconf(open_max) getdtablesize ()
	|                              ^~~~~~~~~~~~~
  spawn_faction_adddup2.c:40:15: note: in expansion of macro '__sysconf'
     40 |   int maxfd = __sysconf (_SC_OPEN_MAX);
	|               ^~~~~~~~~
  spawn_faction_adddup2.c:26:30: warning: nested extern declaration of 'getdtablesize' [-Wnested-externs]
     26 | # define __sysconf(open_max) getdtablesize ()
	|                              ^~~~~~~~~~~~~
  spawn_faction_adddup2.c:40:15: note: in expansion of macro '__sysconf'
     40 |   int maxfd = __sysconf (_SC_OPEN_MAX);
	|               ^~~~~~~~~
    CC       cloexec.o
  cloexec.c: In function 'dup_cloexec':
  cloexec.c:82:10: warning: implicit declaration of function 'fcntl' [-Wimplicit-function-declaration]
     82 |   return fcntl (fd, F_DUPFD_CLOEXEC, 0);
	|          ^~~~~
  cloexec.c:82:10: warning: nested extern declaration of 'fcntl' [-Wnested-externs]
  cloexec.c:82:21: error: 'F_DUPFD_CLOEXEC' undeclared (first use in this function)
     82 |   return fcntl (fd, F_DUPFD_CLOEXEC, 0);
	|                     ^~~~~~~~~~~~~~~
  cloexec.c:82:21: note: each undeclared identifier is reported only once for each function it appears in
  cloexec.c:83:1: warning: control reaches end of non-void function [-Wreturn-type]
     83 | }
	| ^
  Makefile:95: recipe for target `cloexec.o' failed
  make[1]: *** [cloexec.o] Error 1

Re: master 2c79a8f 2/2: Use posix_spawn if possible.

[Philipp Stephani]

Am Di., 29. Dez. 2020 um 19:15 Uhr schrieb Eli Zaretskii <eliz@gnu.org>:

> > What error messages do you see on Windows when trying to build the branch?
>
> The below is only for compiling Gnulib; the build process stopped
> after that, so what will happen in lib-src/ and src/ is still TBD:
>
>     CC       getdtablesize.o
>   getdtablesize.c:58:1: warning: no previous prototype for 'getdtablesize' [-Wmissing-prototypes]
>      58 | getdtablesize (void)
>         | ^~~~~~~~~~~~~

Ah, I guess this is because the MinGW build excludes Gnulib's unistd.h
etc. So for now it looks like we do have to exclude the spawn modules
from the Windows build (unless we can include the POSIX headers from
Gnulib).

Re: master 2c79a8f 2/2: Use posix_spawn if possible.

[Eli Zaretskii]

> From: Philipp Stephani <p.stephani2@gmail.com>
> Date: Tue, 29 Dec 2020 22:36:40 +0100
> Cc: Emacs developers <emacs-devel@gnu.org>
> 
> >     CC       getdtablesize.o
> >   getdtablesize.c:58:1: warning: no previous prototype for 'getdtablesize' [-Wmissing-prototypes]
> >      58 | getdtablesize (void)
> >         | ^~~~~~~~~~~~~
> 
> Ah, I guess this is because the MinGW build excludes Gnulib's unistd.h
> etc. So for now it looks like we do have to exclude the spawn modules
> from the Windows build (unless we can include the POSIX headers from
> Gnulib).

The Gnulib unistd.h caused trouble in the MinGW build, so we disabled
it long ago.  On the trunk, we don't compile getdtablesize.c, either,
and I don't see why we would need to.

(It's not that we always exclude headers from Gnulib on Windows, we
only do that with headers that cause trouble.)

Re: master 2c79a8f 2/2: Use posix_spawn if possible.

[Philipp Stephani]

Am Mi., 30. Dez. 2020 um 04:33 Uhr schrieb Eli Zaretskii <eliz@gnu.org>:
>
> > From: Philipp Stephani <p.stephani2@gmail.com>
> > Date: Tue, 29 Dec 2020 22:36:40 +0100
> > Cc: Emacs developers <emacs-devel@gnu.org>
> >
> > >     CC       getdtablesize.o
> > >   getdtablesize.c:58:1: warning: no previous prototype for 'getdtablesize' [-Wmissing-prototypes]
> > >      58 | getdtablesize (void)
> > >         | ^~~~~~~~~~~~~
> >
> > Ah, I guess this is because the MinGW build excludes Gnulib's unistd.h
> > etc. So for now it looks like we do have to exclude the spawn modules
> > from the Windows build (unless we can include the POSIX headers from
> > Gnulib).
>
> The Gnulib unistd.h caused trouble in the MinGW build, so we disabled
> it long ago.  On the trunk, we don't compile getdtablesize.c, either,
> and I don't see why we would need to.
>
> (It's not that we always exclude headers from Gnulib on Windows, we
> only do that with headers that cause trouble.)

I've now created the branch scratch/posix-spawn-no-gnulib that calls
the libc posix_spawn function/syscall directly. I've realized that for
now that's the better approach, as the Gnulib replacement doesn't let
us call setsid() yet (that's a new functionality that is available on
GNU/Linux and macOS, but not yet in Gnulib). This should also lower
the impact on the Windows build.

Re: master 2c79a8f 2/2: Use posix_spawn if possible.

[Philipp Stephani]

Am Di., 29. Dez. 2020 um 17:43 Uhr schrieb Philipp Stephani
<p.stephani2@gmail.com>:
>
> Am Sa., 26. Dez. 2020 um 13:16 Uhr schrieb Eli Zaretskii <eliz@gnu.org>:
> >
> > > Date: Sat, 26 Dec 2020 14:08:11 +0200
> > > From: Eli Zaretskii <eliz@gnu.org>
> > > Cc: emacs-devel@gnu.org
> > >
> > > See Savannah bug #59093 for one subtle issue:
> > >
> > >   https://savannah.gnu.org/bugs/?59093
> > >
> > > Since Emacs also sets its stack limit in some cases, this could be
> > > directly relevant to us.  (But I didn't look into it close enough to
> > > tell whether it actually is relevant.)
> >
> > Btw, given this condition for using posix_spawn:
> >
> >   +  /* `posix_spawn' doesn't yet support setting up pseudoterminals, so
> >   +     we fall back to `vfork' if we're supposed to use a
> >   +     pseudoterminal.  */
> >   +
> >   +  bool use_posix_spawn = can_use_posix_spawn && pty == NULL;
> >
> > I understand that the absolute majority of subprocesses on GNU and
> > Posix platforms will not use posix_spawn, because we usually do use
> > PTYs?  If so, one wonders why it is a good idea to complicate our code
> > and make it more dependent on Gnulib, for such small gains?  Should we
> > perhaps wait until posix_spawn does support PTYs?
>
> I think posix_spawn supports TTYs well enough by now. From what I see
> in the code we perform 5 TTY-related actions between fork and exec:
> [...]
> 2. Making the TTY the controlling terminal of the process. IIUC we
> first try using TIOCSCTTY, and then opening the TTY file without
> O_NOCTTY. On GNU/Linux, the former one probably fails because stdin is
> -1. So we could only do the latter one, which is supported by
> posix_spawn (since it's just opening a file).

The annoying thing about this, though, is that it doesn't work on the
BSDs (including macOS). For pty processes on those systems we'll have
to continue using fork/vfork for the time being.
I think that using posix_spawn on GNU/Linux and for non-pty processes
is still a significant win, though.

Re: master 2c79a8f 2/2: Use posix_spawn if possible.

[Eli Zaretskii]

> From: Philipp Stephani <p.stephani2@gmail.com>
> Date: Thu, 31 Dec 2020 17:24:41 +0100
> Cc: Emacs developers <emacs-devel@gnu.org>
> 
> I think that using posix_spawn on GNU/Linux and for non-pty processes
> is still a significant win, though.

Maybe so, but are there enough use cases that need non-PTY
subprocesses to make this worth our while?

Re: master 2c79a8f 2/2: Use posix_spawn if possible.

[Philipp Stephani]

Am Do., 31. Dez. 2020 um 17:40 Uhr schrieb Eli Zaretskii <eliz@gnu.org>:
>
> > From: Philipp Stephani <p.stephani2@gmail.com>
> > Date: Thu, 31 Dec 2020 17:24:41 +0100
> > Cc: Emacs developers <emacs-devel@gnu.org>
> >
> > I think that using posix_spawn on GNU/Linux and for non-pty processes
> > is still a significant win, though.
>
> Maybe so, but are there enough use cases that need non-PTY
> subprocesses to make this worth our while?

Most processes don't need a TTY. Only those that interact with the
user (shell, term, interactive compile, ...) do, but that's probably a
minority. In particular, everything running in the background
(Flymake, VC...) doesn't, as well as synchronous processes
(call-process etc.). TTY-based communication happens to be the default
for asynchronous processes, but that's more of a historical choice
than a real need.

Re: master 2c79a8f 2/2: Use posix_spawn if possible.

[Eli Zaretskii]

> From: Philipp Stephani <p.stephani2@gmail.com>
> Date: Thu, 31 Dec 2020 18:36:47 +0100
> Cc: Emacs developers <emacs-devel@gnu.org>
> 
> > Maybe so, but are there enough use cases that need non-PTY
> > subprocesses to make this worth our while?
> 
> Most processes don't need a TTY. Only those that interact with the
> user (shell, term, interactive compile, ...) do, but that's probably a
> minority.

I'm not sure it's a minority.  How about if you come up with a list of
specific commands that launch subprocesses which don't need a PTY?

> In particular, everything running in the background
> (Flymake, VC...) doesn't

These two maybe not, but I'm not so sure about what hides behind the
ellipsis.

But assuming you are right, you intend to audit all the places where
we start a process and switch their :connection-type to nil if needed?
Or change the default value of process-connection-type?

Re: master 2c79a8f 2/2: Use posix_spawn if possible.

[Philipp Stephani]

Am Di., 29. Dez. 2020 um 17:29 Uhr schrieb Philipp Stephani
<p.stephani2@gmail.com>:
>
> > > > Btw, regarding use of posix_spawn, I'd expect a discussion before we
> > > > make such a change.  AFAIU it is not a trivial decision, as
> > > > posix_spawn has its down sides, and therefore is not necessarily the
> > > > best API for running sub-processes on every supported platform, even
> > > > if you consider only the Posix ones.  We should consider the
> > > > advantages and disadvantages before we make the decision.
> > >
> > > Sure, I'm happy to have that discussion. I briefly reviewed the
> > > posix_spawn implementation of GNU libc and Gnulib, and found that it
> > > uses vfork/clone + execve like our hand-rolled code, so I wouldn't
> > > expect any significant change. The primary advantage is to offload
> > > complexity into a library that can properly deal with system-specific
> > > issues and can improve over time. For example, on Linux, posix_spawn
> > > can use clone instead of vfork.
> >
> > See Savannah bug #59093 for one subtle issue:
> >
> >   https://savannah.gnu.org/bugs/?59093
> >
> > Since Emacs also sets its stack limit in some cases, this could be
> > directly relevant to us.  (But I didn't look into it close enough to
> > tell whether it actually is relevant.)
>
> I think that specific problem isn't relevant (we don't change the
> stack size between fork and exec), but the fix to
> https://debbugs.gnu.org/cgi/bugreport.cgi?bug=24869 (ironically
> reported by me) is.
> As indicated in https://debbugs.gnu.org/cgi/bugreport.cgi?bug=24869#8,
> it might be better to solve the underlying problem in a different way
> that doesn't involve changing resource limits (changing a
> process-global setting is somewhat fishy anyway, especially with
> modules). Essentially we just need to make sure to not add file
> descriptors larger than FD_SETSIZE to an fd_set.

I've now done this with commit
8bc85d46cc9214a531f2d2ecb3f5fb48af8105a6. While the setrlimit approach
is nominally cleaner, it can be subverted because soft rlimits can be
changed arbitrarily, so I'd propose we revert the commits that
introduced the setrlimit calls
(b6d9613df83813609ef80da45975e70954d1fb6d,
a5509099484e0762842bc2c9e914779397b91469).

Re: master 2c79a8f 2/2: Use posix_spawn if possible.

[Eli Zaretskii]

> From: Philipp Stephani <p.stephani2@gmail.com>
> Date: Thu, 31 Dec 2020 17:10:28 +0100
> Cc: Emacs developers <emacs-devel@gnu.org>
> 
> I've now created the branch scratch/posix-spawn-no-gnulib that calls
> the libc posix_spawn function/syscall directly. I've realized that for
> now that's the better approach, as the Gnulib replacement doesn't let
> us call setsid() yet (that's a new functionality that is available on
> GNU/Linux and macOS, but not yet in Gnulib). This should also lower
> the impact on the Windows build.

Thanks, that builds on MS-Windows and passes my smoke test (after
fixing a minor glitch that caused a compilation warning).

Re: master 2c79a8f 2/2: Use posix_spawn if possible.

[Eli Zaretskii]

> From: Philipp Stephani <p.stephani2@gmail.com>
> Date: Thu, 31 Dec 2020 18:50:48 +0100
> Cc: Emacs developers <emacs-devel@gnu.org>
> 
> > I think that specific problem isn't relevant (we don't change the
> > stack size between fork and exec), but the fix to
> > https://debbugs.gnu.org/cgi/bugreport.cgi?bug=24869 (ironically
> > reported by me) is.
> > As indicated in https://debbugs.gnu.org/cgi/bugreport.cgi?bug=24869#8,
> > it might be better to solve the underlying problem in a different way
> > that doesn't involve changing resource limits (changing a
> > process-global setting is somewhat fishy anyway, especially with
> > modules). Essentially we just need to make sure to not add file
> > descriptors larger than FD_SETSIZE to an fd_set.
> 
> I've now done this with commit
> 8bc85d46cc9214a531f2d2ecb3f5fb48af8105a6.

On which branch?

Re: master 2c79a8f 2/2: Use posix_spawn if possible.

[Philipp Stephani]

Am Do., 31. Dez. 2020 um 19:35 Uhr schrieb Eli Zaretskii <eliz@gnu.org>:
>
> > From: Philipp Stephani <p.stephani2@gmail.com>
> > Date: Thu, 31 Dec 2020 18:50:48 +0100
> > Cc: Emacs developers <emacs-devel@gnu.org>
> >
> > > I think that specific problem isn't relevant (we don't change the
> > > stack size between fork and exec), but the fix to
> > > https://debbugs.gnu.org/cgi/bugreport.cgi?bug=24869 (ironically
> > > reported by me) is.
> > > As indicated in https://debbugs.gnu.org/cgi/bugreport.cgi?bug=24869#8,
> > > it might be better to solve the underlying problem in a different way
> > > that doesn't involve changing resource limits (changing a
> > > process-global setting is somewhat fishy anyway, especially with
> > > modules). Essentially we just need to make sure to not add file
> > > descriptors larger than FD_SETSIZE to an fd_set.
> >
> > I've now done this with commit
> > 8bc85d46cc9214a531f2d2ecb3f5fb48af8105a6.
>
> On which branch?

master (this is only tangentially related to the posix-spawn discussion).

Re: master 2c79a8f 2/2: Use posix_spawn if possible.

[Philipp Stephani]

Am Do., 31. Dez. 2020 um 18:47 Uhr schrieb Eli Zaretskii <eliz@gnu.org>:
>
> > From: Philipp Stephani <p.stephani2@gmail.com>
> > Date: Thu, 31 Dec 2020 18:36:47 +0100
> > Cc: Emacs developers <emacs-devel@gnu.org>
> >
> > > Maybe so, but are there enough use cases that need non-PTY
> > > subprocesses to make this worth our while?
> >
> > Most processes don't need a TTY. Only those that interact with the
> > user (shell, term, interactive compile, ...) do, but that's probably a
> > minority.
>
> I'm not sure it's a minority.  How about if you come up with a list of
> specific commands that launch subprocesses which don't need a PTY?

Such a list would be difficult to compile because the connection type
often depends on ambient state (the value of process-connection-type).
What I've done instead is
(1) counting the connection types in my current interactive session
doing "typical" work
(2) counting them in the Emacs unit test suite.
I don't know how representative that is, but in both cases pipe
processes significantly outnumbered pty processes (296 vs. 10 for (1)
and 831 vs. 122 for (2)). While this isn't very scientific, I think
it's at least some evidence that pipe processes are more common in
practice.

>
> > In particular, everything running in the background
> > (Flymake, VC...) doesn't
>
> These two maybe not, but I'm not so sure about what hides behind the
> ellipsis.
>
> But assuming you are right, you intend to audit all the places where
> we start a process and switch their :connection-type to nil if needed?

It's not needed (the default works fine in typical cases), the TTY
setup is just superfluous in many cases.

> Or change the default value of process-connection-type?

That's not very realistic. Well-written libraries ought to bind
process-connection-type to the value they prefer, but not all do.

Re: master 2c79a8f 2/2: Use posix_spawn if possible.

[Eli Zaretskii]

> From: Philipp Stephani <p.stephani2@gmail.com>
> Date: Thu, 31 Dec 2020 21:24:50 +0100
> Cc: Emacs developers <emacs-devel@gnu.org>
> 
> > > Most processes don't need a TTY. Only those that interact with the
> > > user (shell, term, interactive compile, ...) do, but that's probably a
> > > minority.
> >
> > I'm not sure it's a minority.  How about if you come up with a list of
> > specific commands that launch subprocesses which don't need a PTY?
> 
> Such a list would be difficult to compile because the connection type
> often depends on ambient state (the value of process-connection-type).
> What I've done instead is
> (1) counting the connection types in my current interactive session
> doing "typical" work
> (2) counting them in the Emacs unit test suite.
> I don't know how representative that is, but in both cases pipe
> processes significantly outnumbered pty processes (296 vs. 10 for (1)
> and 831 vs. 122 for (2)).

That's strange, because every important feature using subprocesses
that I frequently invoke seems to want PTY: gdb, ispell,
flyspell-mode, compile, grep, man, shell...

> > But assuming you are right, you intend to audit all the places where
> > we start a process and switch their :connection-type to nil if needed?
> 
> It's not needed (the default works fine in typical cases), the TTY
> setup is just superfluous in many cases.

But then we won't be able to benefit from posix_spawn, right?

Re: master 2c79a8f 2/2: Use posix_spawn if possible.

[martin rudalics]

 > That's strange, because every important feature using subprocesses
 > that I frequently invoke seems to want PTY: gdb, ispell,
 > flyspell-mode, compile, grep, man, shell...

Here I see

(defcustom ispell-use-ptys-p nil

Using PTYs for spell-checking strikes me as overkill.

martin

Re: master 2c79a8f 2/2: Use posix_spawn if possible.

[Eli Zaretskii]

> From: martin rudalics <rudalics@gmx.at>
> Date: Fri, 1 Jan 2021 08:59:37 +0100
> Cc: emacs-devel@gnu.org
> 
>  > That's strange, because every important feature using subprocesses
>  > that I frequently invoke seems to want PTY: gdb, ispell,
>  > flyspell-mode, compile, grep, man, shell...
> 
> Here I see
> 
> (defcustom ispell-use-ptys-p nil

Oops.

Re: master 2c79a8f 2/2: Use posix_spawn if possible.

[Andy Moreton]

On Thu 31 Dec 2020, Philipp Stephani wrote:
> The annoying thing about this, though, is that it doesn't work on the
> BSDs (including macOS). For pty processes on those systems we'll have
> to continue using fork/vfork for the time being.
> I think that using posix_spawn on GNU/Linux and for non-pty processes
> is still a significant win, though.

Can I ask why ? You have not yet explained what benefit this change
brings, and the length of the discussion shows that there are
difficulties with it. What is improved by using posix_spawn ?

    AndyM

Re: master 2c79a8f 2/2: Use posix_spawn if possible.

[Alan Third]

On Fri, Jan 01, 2021 at 11:38:14PM +0000, Andy Moreton wrote:
> On Thu 31 Dec 2020, Philipp Stephani wrote:
> > The annoying thing about this, though, is that it doesn't work on the
> > BSDs (including macOS). For pty processes on those systems we'll have
> > to continue using fork/vfork for the time being.
> > I think that using posix_spawn on GNU/Linux and for non-pty processes
> > is still a significant win, though.
> 
> Can I ask why ? You have not yet explained what benefit this change
> brings, and the length of the discussion shows that there are
> difficulties with it. What is improved by using posix_spawn ?

posix_spawn is supposed to be significantly faster than fork/exec on
macOS and Cygwin.
-- 
Alan Third

Re: master 2c79a8f 2/2: Use posix_spawn if possible.

[Andy Moreton]

On Fri 01 Jan 2021, Alan Third wrote:

> On Fri, Jan 01, 2021 at 11:38:14PM +0000, Andy Moreton wrote:
>> On Thu 31 Dec 2020, Philipp Stephani wrote:
>> > The annoying thing about this, though, is that it doesn't work on the
>> > BSDs (including macOS). For pty processes on those systems we'll have
>> > to continue using fork/vfork for the time being.
>> > I think that using posix_spawn on GNU/Linux and for non-pty processes
>> > is still a significant win, though.
>> 
>> Can I ask why ? You have not yet explained what benefit this change
>> brings, and the length of the discussion shows that there are
>> difficulties with it. What is improved by using posix_spawn ?
>
> posix_spawn is supposed to be significantly faster than fork/exec on
> macOS and Cygwin.

Thanks Alan, useful info. However Philipp started with Linux, so there
ought to also be something useful there that motivated these patches. I
would be interested to learn what helps on Linux.

    AndyM

Re: master 2c79a8f 2/2: Use posix_spawn if possible.

[Eli Zaretskii]

> From: Andy Moreton <andrewjmoreton@gmail.com>
> Date: Sat, 02 Jan 2021 01:12:36 +0000
> 
> > posix_spawn is supposed to be significantly faster than fork/exec on
> > macOS and Cygwin.
> 
> Thanks Alan, useful info. However Philipp started with Linux, so there
> ought to also be something useful there that motivated these patches. I
> would be interested to learn what helps on Linux.

AFAIK, the speedup also happens on GNU/Linux.

Re: master 2c79a8f 2/2: Use posix_spawn if possible.

[Andreas Schwab]

On Jan 02 2021, Eli Zaretskii wrote:

>> From: Andy Moreton <andrewjmoreton@gmail.com>
>> Date: Sat, 02 Jan 2021 01:12:36 +0000
>> 
>> > posix_spawn is supposed to be significantly faster than fork/exec on
>> > macOS and Cygwin.
>> 
>> Thanks Alan, useful info. However Philipp started with Linux, so there
>> ought to also be something useful there that motivated these patches. I
>> would be interested to learn what helps on Linux.
>
> AFAIK, the speedup also happens on GNU/Linux.

Emacs already uses vfork, what else does posix_spawn give?  Nothing,
AFAICS.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: master 2c79a8f 2/2: Use posix_spawn if possible.

[YAMAMOTO Mitsuharu]

On Sat, 02 Jan 2021 17:56:58 +0900,
Andreas Schwab wrote:
> 
> On Jan 02 2021, Eli Zaretskii wrote:
> 
> >> From: Andy Moreton <andrewjmoreton@gmail.com>
> >> Date: Sat, 02 Jan 2021 01:12:36 +0000
> >> 
> >> > posix_spawn is supposed to be significantly faster than fork/exec on
> >> > macOS and Cygwin.
> >> 
> >> Thanks Alan, useful info. However Philipp started with Linux, so there
> >> ought to also be something useful there that motivated these patches. I
> >> would be interested to learn what helps on Linux.
> >
> > AFAIK, the speedup also happens on GNU/Linux.
> 
> Emacs already uses vfork, what else does posix_spawn give?  Nothing,
> AFAICS.

According to the vfork man page below on macOS 12.0 released this
week, it is deprecating vfork.

				     YAMAMOTO Mitsuharu
				mituharu@math.s.chiba-u.ac.jp

NAME
     vfork – deprecated system call to create a new process

SYNOPSIS
     #include <unistd.h>

     pid_t
     vfork(void);

DESCRIPTION
     The vfork system call can be used to create new processes. As of macOS
     12.0, this system call behaves identically to the fork(2) system call,
     except without calling any handlers registered with pthread_atfork(2).

     This system call is deprecated. In a future release, it may begin to return
     errors in all cases, or may be removed entirely.  It is extremely strongly
     recommended to replace all uses with fork(2) or, ideally, posix_spawn(3).

Re: master 2c79a8f 2/2: Use posix_spawn if possible.

[Alan Third]

On Fri, Oct 29, 2021 at 06:46:47PM +0900, YAMAMOTO Mitsuharu wrote:
> According to the vfork man page below on macOS 12.0 released this
> week, it is deprecating vfork.
> 
> 				     YAMAMOTO Mitsuharu
> 				mituharu@math.s.chiba-u.ac.jp
> 
> NAME
>      vfork – deprecated system call to create a new process
> 
> SYNOPSIS
>      #include <unistd.h>
> 
>      pid_t
>      vfork(void);
> 
> DESCRIPTION
>      The vfork system call can be used to create new processes. As of macOS
>      12.0, this system call behaves identically to the fork(2) system call,
>      except without calling any handlers registered with pthread_atfork(2).
> 
>      This system call is deprecated. In a future release, it may begin to return
>      errors in all cases, or may be removed entirely.  It is extremely strongly
>      recommended to replace all uses with fork(2) or, ideally, posix_spawn(3).

Ugh, so this completely undoes the work we did to improve process
spawning performance in Emacs 26.
-- 
Alan Third

Re: master 2c79a8f 2/2: Use posix_spawn if possible.

[Alan Third]

On Sat, Oct 30, 2021 at 07:30:49PM +0100, Alan Third wrote:
> On Fri, Oct 29, 2021 at 06:46:47PM +0900, YAMAMOTO Mitsuharu wrote:
> > According to the vfork man page below on macOS 12.0 released this
> > week, it is deprecating vfork.
> > 
> > 				     YAMAMOTO Mitsuharu
> > 				mituharu@math.s.chiba-u.ac.jp
> > 
> > NAME
> >      vfork – deprecated system call to create a new process
> > 
> > SYNOPSIS
> >      #include <unistd.h>
> > 
> >      pid_t
> >      vfork(void);
> > 
> > DESCRIPTION
> >      The vfork system call can be used to create new processes. As of macOS
> >      12.0, this system call behaves identically to the fork(2) system call,
> >      except without calling any handlers registered with pthread_atfork(2).
> > 
> >      This system call is deprecated. In a future release, it may begin to return
> >      errors in all cases, or may be removed entirely.  It is extremely strongly
> >      recommended to replace all uses with fork(2) or, ideally, posix_spawn(3).
> 
> Ugh, so this completely undoes the work we did to improve process
> spawning performance in Emacs 26.

OK, so this is already causing complaints on reddit.

When we put in the fix to use vfork on macOS back in Emacs 26 the
measured difference between fork and vfork was something like 15x. Now
Apple have turned vfork into fork I guess we're back to the same
situation. Packages that make heavy use of external processes, such as
Magit, are badly hit by this slow down.

Is there anything we need to do to get scratch/posix-spawn-no-gnulib
merged into master (or even emacs-28)?

-- 
Alan Third

Re: master 2c79a8f 2/2: Use posix_spawn if possible.

[Eli Zaretskii]

> Date: Tue, 2 Nov 2021 19:58:10 +0000
> From: Alan Third <alan@idiocy.org>
> Cc: Philipp Stephani <p.stephani2@gmail.com>
> 
> When we put in the fix to use vfork on macOS back in Emacs 26 the
> measured difference between fork and vfork was something like 15x. Now
> Apple have turned vfork into fork I guess we're back to the same
> situation. Packages that make heavy use of external processes, such as
> Magit, are badly hit by this slow down.
> 
> Is there anything we need to do to get scratch/posix-spawn-no-gnulib
> merged into master (or even emacs-28)?

Does it solve the problem?  And which builds does it affect?

Re: master 2c79a8f 2/2: Use posix_spawn if possible.

[Alan Third]

On Tue, Nov 02, 2021 at 10:15:29PM +0200, Eli Zaretskii wrote:
> > Date: Tue, 2 Nov 2021 19:58:10 +0000
> > From: Alan Third <alan@idiocy.org>
> > Cc: Philipp Stephani <p.stephani2@gmail.com>
> > 
> > When we put in the fix to use vfork on macOS back in Emacs 26 the
> > measured difference between fork and vfork was something like 15x. Now
> > Apple have turned vfork into fork I guess we're back to the same
> > situation. Packages that make heavy use of external processes, such as
> > Magit, are badly hit by this slow down.
> > 
> > Is there anything we need to do to get scratch/posix-spawn-no-gnulib
> > merged into master (or even emacs-28)?
> 
> Does it solve the problem?  And which builds does it affect?

As I understand it Yamamoto Mitsuharu has already merged it into the
Mac port and I've seen it reported that the problems are solved with
it.

I don't have a Mac with the latest macOS on to test it myself.

This is really only a problem on macOS, so we could limit the use of
posix_spawn to Darwin builds.

I am concerned that there could be hidden issues with the branch since
it doesn't seem to have had much testing, so it's probably best going
into master, but I expect we'll see an increasing number of complaints
about performance as more people switch to macOS 12. Perhaps we should
keep it in mind as a possible update for 28.2, whenever that may be.

-- 
Alan Third

Re: master 2c79a8f 2/2: Use posix_spawn if possible.

[Eli Zaretskii]

> Date: Tue, 2 Nov 2021 20:36:04 +0000
> From: Alan Third <alan@idiocy.org>
> Cc: p.stephani2@gmail.com, mituharu@math.s.chiba-u.ac.jp,
> 	emacs-devel@gnu.org
> 
> > > Is there anything we need to do to get scratch/posix-spawn-no-gnulib
> > > merged into master (or even emacs-28)?
> > 
> > Does it solve the problem?  And which builds does it affect?
> 
> As I understand it Yamamoto Mitsuharu has already merged it into the
> Mac port and I've seen it reported that the problems are solved with
> it.
> 
> I don't have a Mac with the latest macOS on to test it myself.
> 
> This is really only a problem on macOS, so we could limit the use of
> posix_spawn to Darwin builds.
> 
> I am concerned that there could be hidden issues with the branch since
> it doesn't seem to have had much testing, so it's probably best going
> into master, but I expect we'll see an increasing number of complaints
> about performance as more people switch to macOS 12. Perhaps we should
> keep it in mind as a possible update for 28.2, whenever that may be.

It is fine with me to merge this to master, as a macOS-only feature,
unless Philipp has reasons not to do that yet.  We can reconsider
backporting to emacs-28 at a later date, if and when we get enough
complaints.

Thanks.

Re: master 2c79a8f 2/2: Use posix_spawn if possible.

[Aaron Jensen]

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

Hi all,

Attached is the posix spawn patch adapted to be only effective on
Darwin. I chose to leave the patch as-is aside from adding the
additional conditional, but let me know if I should do something
different. It wasn't clear where things stood in terms of whether or
not we would ever want to use posix_spawn on non-darwin OSes.

Thanks,

Aaron

[-- Attachment #2: 0001-Use-posix_spawn-if-possible-on-Darwin.patch --]
[-- Type: application/octet-stream, Size: 8764 bytes --]

From 8055b42c60b4178707074c2dfb4530aeb926c59d Mon Sep 17 00:00:00 2001
From: Aaron Jensen <aaronjensen@gmail.com>
Date: Mon, 8 Nov 2021 05:38:11 -0500
Subject: [PATCH] Use posix_spawn if possible on Darwin.

posix_spawn is less error-prone than vfork + execve, and can make
better use of system-specific enhancements like 'clone' on Linux.  Use
it if we don't need to configure a pseudoterminal.

* configure.ac (HAVE_SPAWN_H, HAVE_POSIX_SPAWN)
(HAVE_POSIX_SPAWN_FILE_ACTIONS_ADDCHDIR)
(HAVE_POSIX_SPAWN_FILE_ACTIONS_ADDCHDIR_NP)
(HAVE_POSIX_SPAWNATTR_SETFLAGS, HAVE_DECL_POSIX_SPAWN_SETSID): New
configuration variables.
* src/callproc.c (USABLE_POSIX_SPAWN): New configuration macro.
(emacs_posix_spawn_init_actions)
(emacs_posix_spawn_init_attributes, emacs_posix_spawn_init): New
helper functions.
(emacs_spawn): Use posix_spawn if possible.

Originally authored by: Philipp Stephani <phst@google.com>
---
 configure.ac   |  17 +++++
 src/callproc.c | 194 ++++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 210 insertions(+), 1 deletion(-)

diff --git a/configure.ac b/configure.ac
index 33e7037afe..c231c2ceae 100644
--- a/configure.ac
+++ b/configure.ac
@@ -4771,6 +4771,23 @@ AC_DEFUN
 dnl the current CFLAGS etc.
 AC_CHECK_FUNCS(snprintf)
 
+dnl posix_spawn.  The chdir and setsid functionality is relatively
+dnl recent, so we check for it specifically.
+AC_CHECK_HEADERS([spawn.h])
+AC_SUBST([HAVE_SPAWN_H])
+AC_CHECK_FUNCS([posix_spawn \
+                posix_spawn_file_actions_addchdir \
+                posix_spawn_file_actions_addchdir_np \
+                posix_spawnattr_setflags])
+AC_SUBST([HAVE_POSIX_SPAWN])
+AC_SUBST([HAVE_POSIX_SPAWN_FILE_ACTIONS_ADDCHDIR])
+AC_SUBST([HAVE_POSIX_SPAWN_FILE_ACTIONS_ADDCHDIR_NP])
+AC_SUBST([HAVE_POSIX_SPAWNATTR_SETFLAGS])
+AC_CHECK_DECLS([POSIX_SPAWN_SETSID], [], [], [[
+               #include <spawn.h>
+               ]])
+AC_SUBST([HAVE_DECL_POSIX_SPAWN_SETSID])
+
 dnl Check for glib.  This differs from other library checks in that
 dnl Emacs need not link to glib unless some other library is already
 dnl linking to glib.  Although glib provides no facilities that Emacs
diff --git a/src/callproc.c b/src/callproc.c
index fa43f97384..77590d99ae 100644
--- a/src/callproc.c
+++ b/src/callproc.c
@@ -28,6 +28,22 @@ Copyright (C) 1985-1988, 1993-1995, 1999-2021 Free Software Foundation,
 #include <sys/file.h>
 #include <fcntl.h>
 
+/* In order to be able to use `posix_spawn', it needs to support some
+   variant of `chdir' as well as `setsid'.  It is only enabled on
+   Darwin to work around vfork being deprecated.  */
+#if defined DARWIN_OS \
+  && defined HAVE_SPAWN_H && defined HAVE_POSIX_SPAWN        \
+  && defined HAVE_POSIX_SPAWNATTR_SETFLAGS                  \
+  && (defined HAVE_POSIX_SPAWN_FILE_ACTIONS_ADDCHDIR        \
+      || defined HAVE_POSIX_SPAWN_FILE_ACTIONS_ADDCHDIR_NP) \
+  && defined HAVE_DECL_POSIX_SPAWN_SETSID                   \
+  && HAVE_DECL_POSIX_SPAWN_SETSID == 1
+# include <spawn.h>
+# define USABLE_POSIX_SPAWN 1
+#else
+# define USABLE_POSIX_SPAWN 0
+#endif
+
 #include "lisp.h"
 
 #ifdef SETUP_SLAVE_PTY
@@ -1247,6 +1263,130 @@ child_setup (int in, int out, int err, char **new_argv, char **env,
 #endif  /* not WINDOWSNT */
 }
 
+#if USABLE_POSIX_SPAWN
+
+/* Set up ACTIONS and ATTRIBUTES for `posix_spawn'.  Return an error
+   number.  */
+
+static int
+emacs_posix_spawn_init_actions (posix_spawn_file_actions_t *actions,
+                                int std_in, int std_out, int std_err,
+                                const char *cwd)
+{
+  int error = posix_spawn_file_actions_init (actions);
+  if (error != 0)
+    return error;
+
+  error = posix_spawn_file_actions_adddup2 (actions, std_in,
+                                            STDIN_FILENO);
+  if (error != 0)
+    goto out;
+
+  error = posix_spawn_file_actions_adddup2 (actions, std_out,
+                                            STDOUT_FILENO);
+  if (error != 0)
+    goto out;
+
+  error = posix_spawn_file_actions_adddup2 (actions,
+                                            std_err < 0 ? std_out
+                                                        : std_err,
+                                            STDERR_FILENO);
+  if (error != 0)
+    goto out;
+
+  error =
+#ifdef HAVE_POSIX_SPAWN_FILE_ACTIONS_ADDCHDIR
+    posix_spawn_file_actions_addchdir
+#else
+    posix_spawn_file_actions_addchdir_np
+#endif
+    (actions, cwd);
+  if (error != 0)
+    goto out;
+
+ out:
+  if (error != 0)
+    posix_spawn_file_actions_destroy (actions);
+  return error;
+}
+
+static int
+emacs_posix_spawn_init_attributes (posix_spawnattr_t *attributes)
+{
+  int error = posix_spawnattr_init (attributes);
+  if (error != 0)
+    return error;
+
+  error = posix_spawnattr_setflags (attributes,
+                                    POSIX_SPAWN_SETSID
+                                      | POSIX_SPAWN_SETSIGDEF
+                                      | POSIX_SPAWN_SETSIGMASK);
+  if (error != 0)
+    goto out;
+
+  sigset_t sigdefault;
+  sigemptyset (&sigdefault);
+
+#ifdef DARWIN_OS
+  /* Work around a macOS bug, where SIGCHLD is apparently
+     delivered to a vforked child instead of to its parent.  See:
+     https://lists.gnu.org/r/emacs-devel/2017-05/msg00342.html
+  */
+  sigaddset (&sigdefault, SIGCHLD);
+#endif
+
+  sigaddset (&sigdefault, SIGINT);
+  sigaddset (&sigdefault, SIGQUIT);
+#ifdef SIGPROF
+  sigaddset (&sigdefault, SIGPROF);
+#endif
+
+  /* Emacs ignores SIGPIPE, but the child should not.  */
+  sigaddset (&sigdefault, SIGPIPE);
+  /* Likewise for SIGPROF.  */
+#ifdef SIGPROF
+  sigaddset (&sigdefault, SIGPROF);
+#endif
+
+  error = posix_spawnattr_setsigdefault (attributes, &sigdefault);
+  if (error != 0)
+    goto out;
+
+  /* Stop blocking SIGCHLD in the child.  */
+  sigset_t oldset;
+  error = pthread_sigmask (SIG_SETMASK, NULL, &oldset);
+  if (error != 0)
+    goto out;
+  error = posix_spawnattr_setsigmask (attributes, &oldset);
+  if (error != 0)
+    goto out;
+
+ out:
+  if (error != 0)
+    posix_spawnattr_destroy (attributes);
+
+  return error;
+}
+
+static int
+emacs_posix_spawn_init (posix_spawn_file_actions_t *actions,
+                        posix_spawnattr_t *attributes, int std_in,
+                        int std_out, int std_err, const char *cwd)
+{
+  int error = emacs_posix_spawn_init_actions (actions, std_in,
+                                              std_out, std_err, cwd);
+  if (error != 0)
+    return error;
+
+  error = emacs_posix_spawn_init_attributes (attributes);
+  if (error != 0)
+    return error;
+
+  return 0;
+}
+
+#endif
+
 /* Start a new asynchronous subprocess.  If successful, return zero
    and store the process identifier of the new process in *NEWPID.
    Use STDIN, STDOUT, and STDERR as standard streams for the new
@@ -1266,10 +1406,58 @@ emacs_spawn (pid_t *newpid, int std_in, int std_out, int std_err,
              char **argv, char **envp, const char *cwd,
              const char *pty, const sigset_t *oldset)
 {
+#if USABLE_POSIX_SPAWN
+  /* Prefer the simpler `posix_spawn' if available.  `posix_spawn'
+     doesn't yet support setting up pseudoterminals, so we fall back
+     to `vfork' if we're supposed to use a pseudoterminal.  */
+
+  bool use_posix_spawn = pty == NULL;
+
+  posix_spawn_file_actions_t actions;
+  posix_spawnattr_t attributes;
+
+  if (use_posix_spawn)
+    {
+      /* Initialize optional attributes before blocking. */
+      int error
+        = emacs_posix_spawn_init (&actions, &attributes, std_in,
+                                  std_out, std_err, cwd);
+      if (error != 0)
+	return error;
+    }
+#endif
+
   int pid;
+  int vfork_error;
 
   eassert (input_blocked_p ());
 
+#if USABLE_POSIX_SPAWN
+  if (use_posix_spawn)
+    {
+      vfork_error = posix_spawn (&pid, argv[0], &actions, &attributes,
+                                 argv, envp);
+      if (vfork_error != 0)
+	pid = -1;
+
+      int error = posix_spawn_file_actions_destroy (&actions);
+      if (error != 0)
+	{
+	  errno = error;
+	  emacs_perror ("posix_spawn_file_actions_destroy");
+	}
+
+      error = posix_spawnattr_destroy (&attributes);
+      if (error != 0)
+	{
+	  errno = error;
+	  emacs_perror ("posix_spawnattr_destroy");
+	}
+
+      goto fork_done;
+    }
+#endif
+
 #ifndef WINDOWSNT
   /* vfork, and prevent local vars from being clobbered by the vfork.  */
   pid_t *volatile newpid_volatile = newpid;
@@ -1413,7 +1601,11 @@ emacs_spawn (pid_t *newpid, int std_in, int std_out, int std_err,
 
   /* Back in the parent process.  */
 
-  int vfork_error = pid < 0 ? errno : 0;
+  vfork_error = pid < 0 ? errno : 0;
+
+#if USABLE_POSIX_SPAWN
+ fork_done:
+#endif
 
   if (pid < 0)
     {
-- 
2.33.1

Re: master 2c79a8f 2/2: Use posix_spawn if possible.

[Aaron Jensen]

On Mon, Nov 8, 2021 at 6:00 AM Aaron Jensen <aaronjensen@gmail.com> wrote:
>
> Hi all,
>
> Attached is the posix spawn patch adapted to be only effective on
> Darwin. I chose to leave the patch as-is aside from adding the
> additional conditional, but let me know if I should do something
> different. It wasn't clear where things stood in terms of whether or
> not we would ever want to use posix_spawn on non-darwin OSes.

Including Phillipp, the original author.

Re: master 2c79a8f 2/2: Use posix_spawn if possible.

[Alan Third]

On Mon, Nov 08, 2021 at 06:00:39AM -0500, Aaron Jensen wrote:
> Hi all,
> 
> Attached is the posix spawn patch adapted to be only effective on
> Darwin. I chose to leave the patch as-is aside from adding the
> additional conditional, but let me know if I should do something
> different. It wasn't clear where things stood in terms of whether or
> not we would ever want to use posix_spawn on non-darwin OSes.

Thanks for this. I'm inclined to wait a few more days to see if
Philipp has anything to add before pushing it.

-- 
Alan Third

Re: master 2c79a8f 2/2: Use posix_spawn if possible.

[Philipp]

> Am 08.11.2021 um 12:00 schrieb Aaron Jensen <aaronjensen@gmail.com>:
> 
> Hi all,
> 
> Attached is the posix spawn patch adapted to be only effective on
> Darwin. I chose to leave the patch as-is aside from adding the
> additional conditional, but let me know if I should do something
> different. It wasn't clear where things stood in terms of whether or
> not we would ever want to use posix_spawn on non-darwin OSes.

I guess either way is fine.  I'd slightly lean towards enabling it whenever it's available (i.e. remove the condition on DARWIN_OS):

1. The libc implementation of posix_spawn can make use of system-specific implementation improvements.  For example, GNU libc on GNU/Linux uses the `clone' system call instead of `vfork' to create a separate stack for the subprocess.
2. Since it's easier to run a continuous integration suite on free OSes, by exercising the posix_spawn code path on them we can improve test coverage.
3. Using posix_spawn is simpler since it's not possible to accidentally introduce undefined behavior (by calling an async-signal-unsafe function in the child).

My original motivation for dropping the posix_spawn branch was that we'll have to maintain the fork/exec branch for the foreseeable future and that there's no measurable benefit from using posix_spawn, so it would just be additional code to maintain.  Since that argument is no longer correct, we might as well use posix_spawn where possible.

Re: master 2c79a8f 2/2: Use posix_spawn if possible.

[Aaron Jensen]

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

On Tue, Nov 9, 2021 at 9:46 AM Philipp <p.stephani2@gmail.com> wrote:
>
> I guess either way is fine.  I'd slightly lean towards enabling it whenever it's available (i.e. remove the condition on DARWIN_OS):

I'm good with that, I only added it because I believe in an earlier
message in the thread that was called for. Patch for that attached.
It'd be best and totally fine with me if you wanted to commit this
with your name attached and not mine as I only did the conflict
resolution and am not familiar with the code itself.

[-- Attachment #2: 0001-Use-posix_spawn-if-possible.patch --]
[-- Type: application/octet-stream, Size: 8654 bytes --]

From f20deb3924b00dea21361ee7178c5892e8da01f9 Mon Sep 17 00:00:00 2001
From: Aaron Jensen <aaronjensen@gmail.com>
Date: Mon, 8 Nov 2021 05:38:11 -0500
Subject: [PATCH] Use posix_spawn if possible

posix_spawn is less error-prone than vfork + execve, and can make
better use of system-specific enhancements like 'clone' on Linux.  Use
it if we don't need to configure a pseudoterminal.

* configure.ac (HAVE_SPAWN_H, HAVE_POSIX_SPAWN)
(HAVE_POSIX_SPAWN_FILE_ACTIONS_ADDCHDIR)
(HAVE_POSIX_SPAWN_FILE_ACTIONS_ADDCHDIR_NP)
(HAVE_POSIX_SPAWNATTR_SETFLAGS, HAVE_DECL_POSIX_SPAWN_SETSID): New
configuration variables.
* src/callproc.c (USABLE_POSIX_SPAWN): New configuration macro.
(emacs_posix_spawn_init_actions)
(emacs_posix_spawn_init_attributes, emacs_posix_spawn_init): New
helper functions.
(emacs_spawn): Use posix_spawn if possible.

Originally authored by: Philipp Stephani <phst@google.com>
---
 configure.ac   |  17 +++++
 src/callproc.c | 192 ++++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 208 insertions(+), 1 deletion(-)

diff --git a/configure.ac b/configure.ac
index 33e7037afe..c231c2ceae 100644
--- a/configure.ac
+++ b/configure.ac
@@ -4771,6 +4771,23 @@ AC_DEFUN
 dnl the current CFLAGS etc.
 AC_CHECK_FUNCS(snprintf)
 
+dnl posix_spawn.  The chdir and setsid functionality is relatively
+dnl recent, so we check for it specifically.
+AC_CHECK_HEADERS([spawn.h])
+AC_SUBST([HAVE_SPAWN_H])
+AC_CHECK_FUNCS([posix_spawn \
+                posix_spawn_file_actions_addchdir \
+                posix_spawn_file_actions_addchdir_np \
+                posix_spawnattr_setflags])
+AC_SUBST([HAVE_POSIX_SPAWN])
+AC_SUBST([HAVE_POSIX_SPAWN_FILE_ACTIONS_ADDCHDIR])
+AC_SUBST([HAVE_POSIX_SPAWN_FILE_ACTIONS_ADDCHDIR_NP])
+AC_SUBST([HAVE_POSIX_SPAWNATTR_SETFLAGS])
+AC_CHECK_DECLS([POSIX_SPAWN_SETSID], [], [], [[
+               #include <spawn.h>
+               ]])
+AC_SUBST([HAVE_DECL_POSIX_SPAWN_SETSID])
+
 dnl Check for glib.  This differs from other library checks in that
 dnl Emacs need not link to glib unless some other library is already
 dnl linking to glib.  Although glib provides no facilities that Emacs
diff --git a/src/callproc.c b/src/callproc.c
index fa43f97384..cb69b8ddd8 100644
--- a/src/callproc.c
+++ b/src/callproc.c
@@ -28,6 +28,20 @@ Copyright (C) 1985-1988, 1993-1995, 1999-2021 Free Software Foundation,
 #include <sys/file.h>
 #include <fcntl.h>
 
+/* In order to be able to use `posix_spawn', it needs to support some
+   variant of `chdir' as well as `setsid'.  */
+#if defined HAVE_SPAWN_H && defined HAVE_POSIX_SPAWN        \
+  && defined HAVE_POSIX_SPAWNATTR_SETFLAGS                  \
+  && (defined HAVE_POSIX_SPAWN_FILE_ACTIONS_ADDCHDIR        \
+      || defined HAVE_POSIX_SPAWN_FILE_ACTIONS_ADDCHDIR_NP) \
+  && defined HAVE_DECL_POSIX_SPAWN_SETSID                   \
+  && HAVE_DECL_POSIX_SPAWN_SETSID == 1
+# include <spawn.h>
+# define USABLE_POSIX_SPAWN 1
+#else
+# define USABLE_POSIX_SPAWN 0
+#endif
+
 #include "lisp.h"
 
 #ifdef SETUP_SLAVE_PTY
@@ -1247,6 +1261,130 @@ child_setup (int in, int out, int err, char **new_argv, char **env,
 #endif  /* not WINDOWSNT */
 }
 
+#if USABLE_POSIX_SPAWN
+
+/* Set up ACTIONS and ATTRIBUTES for `posix_spawn'.  Return an error
+   number.  */
+
+static int
+emacs_posix_spawn_init_actions (posix_spawn_file_actions_t *actions,
+                                int std_in, int std_out, int std_err,
+                                const char *cwd)
+{
+  int error = posix_spawn_file_actions_init (actions);
+  if (error != 0)
+    return error;
+
+  error = posix_spawn_file_actions_adddup2 (actions, std_in,
+                                            STDIN_FILENO);
+  if (error != 0)
+    goto out;
+
+  error = posix_spawn_file_actions_adddup2 (actions, std_out,
+                                            STDOUT_FILENO);
+  if (error != 0)
+    goto out;
+
+  error = posix_spawn_file_actions_adddup2 (actions,
+                                            std_err < 0 ? std_out
+                                                        : std_err,
+                                            STDERR_FILENO);
+  if (error != 0)
+    goto out;
+
+  error =
+#ifdef HAVE_POSIX_SPAWN_FILE_ACTIONS_ADDCHDIR
+    posix_spawn_file_actions_addchdir
+#else
+    posix_spawn_file_actions_addchdir_np
+#endif
+    (actions, cwd);
+  if (error != 0)
+    goto out;
+
+ out:
+  if (error != 0)
+    posix_spawn_file_actions_destroy (actions);
+  return error;
+}
+
+static int
+emacs_posix_spawn_init_attributes (posix_spawnattr_t *attributes)
+{
+  int error = posix_spawnattr_init (attributes);
+  if (error != 0)
+    return error;
+
+  error = posix_spawnattr_setflags (attributes,
+                                    POSIX_SPAWN_SETSID
+                                      | POSIX_SPAWN_SETSIGDEF
+                                      | POSIX_SPAWN_SETSIGMASK);
+  if (error != 0)
+    goto out;
+
+  sigset_t sigdefault;
+  sigemptyset (&sigdefault);
+
+#ifdef DARWIN_OS
+  /* Work around a macOS bug, where SIGCHLD is apparently
+     delivered to a vforked child instead of to its parent.  See:
+     https://lists.gnu.org/r/emacs-devel/2017-05/msg00342.html
+  */
+  sigaddset (&sigdefault, SIGCHLD);
+#endif
+
+  sigaddset (&sigdefault, SIGINT);
+  sigaddset (&sigdefault, SIGQUIT);
+#ifdef SIGPROF
+  sigaddset (&sigdefault, SIGPROF);
+#endif
+
+  /* Emacs ignores SIGPIPE, but the child should not.  */
+  sigaddset (&sigdefault, SIGPIPE);
+  /* Likewise for SIGPROF.  */
+#ifdef SIGPROF
+  sigaddset (&sigdefault, SIGPROF);
+#endif
+
+  error = posix_spawnattr_setsigdefault (attributes, &sigdefault);
+  if (error != 0)
+    goto out;
+
+  /* Stop blocking SIGCHLD in the child.  */
+  sigset_t oldset;
+  error = pthread_sigmask (SIG_SETMASK, NULL, &oldset);
+  if (error != 0)
+    goto out;
+  error = posix_spawnattr_setsigmask (attributes, &oldset);
+  if (error != 0)
+    goto out;
+
+ out:
+  if (error != 0)
+    posix_spawnattr_destroy (attributes);
+
+  return error;
+}
+
+static int
+emacs_posix_spawn_init (posix_spawn_file_actions_t *actions,
+                        posix_spawnattr_t *attributes, int std_in,
+                        int std_out, int std_err, const char *cwd)
+{
+  int error = emacs_posix_spawn_init_actions (actions, std_in,
+                                              std_out, std_err, cwd);
+  if (error != 0)
+    return error;
+
+  error = emacs_posix_spawn_init_attributes (attributes);
+  if (error != 0)
+    return error;
+
+  return 0;
+}
+
+#endif
+
 /* Start a new asynchronous subprocess.  If successful, return zero
    and store the process identifier of the new process in *NEWPID.
    Use STDIN, STDOUT, and STDERR as standard streams for the new
@@ -1266,10 +1404,58 @@ emacs_spawn (pid_t *newpid, int std_in, int std_out, int std_err,
              char **argv, char **envp, const char *cwd,
              const char *pty, const sigset_t *oldset)
 {
+#if USABLE_POSIX_SPAWN
+  /* Prefer the simpler `posix_spawn' if available.  `posix_spawn'
+     doesn't yet support setting up pseudoterminals, so we fall back
+     to `vfork' if we're supposed to use a pseudoterminal.  */
+
+  bool use_posix_spawn = pty == NULL;
+
+  posix_spawn_file_actions_t actions;
+  posix_spawnattr_t attributes;
+
+  if (use_posix_spawn)
+    {
+      /* Initialize optional attributes before blocking. */
+      int error
+        = emacs_posix_spawn_init (&actions, &attributes, std_in,
+                                  std_out, std_err, cwd);
+      if (error != 0)
+	return error;
+    }
+#endif
+
   int pid;
+  int vfork_error;
 
   eassert (input_blocked_p ());
 
+#if USABLE_POSIX_SPAWN
+  if (use_posix_spawn)
+    {
+      vfork_error = posix_spawn (&pid, argv[0], &actions, &attributes,
+                                 argv, envp);
+      if (vfork_error != 0)
+	pid = -1;
+
+      int error = posix_spawn_file_actions_destroy (&actions);
+      if (error != 0)
+	{
+	  errno = error;
+	  emacs_perror ("posix_spawn_file_actions_destroy");
+	}
+
+      error = posix_spawnattr_destroy (&attributes);
+      if (error != 0)
+	{
+	  errno = error;
+	  emacs_perror ("posix_spawnattr_destroy");
+	}
+
+      goto fork_done;
+    }
+#endif
+
 #ifndef WINDOWSNT
   /* vfork, and prevent local vars from being clobbered by the vfork.  */
   pid_t *volatile newpid_volatile = newpid;
@@ -1413,7 +1599,11 @@ emacs_spawn (pid_t *newpid, int std_in, int std_out, int std_err,
 
   /* Back in the parent process.  */
 
-  int vfork_error = pid < 0 ? errno : 0;
+  vfork_error = pid < 0 ? errno : 0;
+
+#if USABLE_POSIX_SPAWN
+ fork_done:
+#endif
 
   if (pid < 0)
     {
-- 
2.33.1

Re: master 2c79a8f 2/2: Use posix_spawn if possible.

[Eli Zaretskii]

> From: Aaron Jensen <aaronjensen@gmail.com>
> Date: Tue, 9 Nov 2021 10:57:11 -0500
> Cc: Emacs developers <emacs-devel@gnu.org>, Eli Zaretskii <eliz@gnu.org>, Alan Third <alan@idiocy.org>, 
> 	YAMAMOTO Mitsuharu <mituharu@math.s.chiba-u.ac.jp>
> On Tue, Nov 9, 2021 at 9:46 AM Philipp <p.stephani2@gmail.com> wrote:
> >
> > I guess either way is fine.  I'd slightly lean towards enabling it whenever it's available (i.e. remove the condition on DARWIN_OS):
> 
> I'm good with that, I only added it because I believe in an earlier
> message in the thread that was called for.

Didn't you want to install this on the release branch?  I thought you
did, and that's why I asked for a patch that only affected macOS: on
the release branch we will not install such a significant change that
affects platforms which don't absolutely have to have it.

Re: master 2c79a8f 2/2: Use posix_spawn if possible.

[Aaron Jensen]

On Tue, Nov 9, 2021 at 12:05 PM Eli Zaretskii <eliz@gnu.org> wrote:
>
> > From: Aaron Jensen <aaronjensen@gmail.com>
> > Date: Tue, 9 Nov 2021 10:57:11 -0500
> > Cc: Emacs developers <emacs-devel@gnu.org>, Eli Zaretskii <eliz@gnu.org>, Alan Third <alan@idiocy.org>,
> >       YAMAMOTO Mitsuharu <mituharu@math.s.chiba-u.ac.jp>
> > On Tue, Nov 9, 2021 at 9:46 AM Philipp <p.stephani2@gmail.com> wrote:
> > >
> > > I guess either way is fine.  I'd slightly lean towards enabling it whenever it's available (i.e. remove the condition on DARWIN_OS):
> >
> > I'm good with that, I only added it because I believe in an earlier
> > message in the thread that was called for.
>
> Didn't you want to install this on the release branch?  I thought you
> did, and that's why I asked for a patch that only affected macOS: on
> the release branch we will not install such a significant change that
> affects platforms which don't absolutely have to have it.

Not me, but I think that's a good idea. The Darwin only patch should
install on emacs-28, so maybe we could install that there and the
all-platform patch on master?

Thanks,

Aaron

Re: master 2c79a8f 2/2: Use posix_spawn if possible.

[Philipp Stephani]

Am Mi., 3. Nov. 2021 um 04:24 Uhr schrieb Eli Zaretskii <eliz@gnu.org>:
>
> > Date: Tue, 2 Nov 2021 20:36:04 +0000
> > From: Alan Third <alan@idiocy.org>
> > Cc: p.stephani2@gmail.com, mituharu@math.s.chiba-u.ac.jp,
> >       emacs-devel@gnu.org
> >
> > > > Is there anything we need to do to get scratch/posix-spawn-no-gnulib
> > > > merged into master (or even emacs-28)?
> > >
> > > Does it solve the problem?  And which builds does it affect?
> >
> > As I understand it Yamamoto Mitsuharu has already merged it into the
> > Mac port and I've seen it reported that the problems are solved with
> > it.
> >
> > I don't have a Mac with the latest macOS on to test it myself.
> >
> > This is really only a problem on macOS, so we could limit the use of
> > posix_spawn to Darwin builds.
> >
> > I am concerned that there could be hidden issues with the branch since
> > it doesn't seem to have had much testing, so it's probably best going
> > into master, but I expect we'll see an increasing number of complaints
> > about performance as more people switch to macOS 12. Perhaps we should
> > keep it in mind as a possible update for 28.2, whenever that may be.
>
> It is fine with me to merge this to master, as a macOS-only feature,
> unless Philipp has reasons not to do that yet.  We can reconsider
> backporting to emacs-28 at a later date, if and when we get enough
> complaints.

Is there a specific reason why we'd want to restrict this to macOS on
master? If the concern is lack of test coverage, then using the branch
on as many OSes as possible would help address that.

Re: master 2c79a8f 2/2: Use posix_spawn if possible.

[Eli Zaretskii]

> From: Philipp Stephani <p.stephani2@gmail.com>
> Date: Wed, 10 Nov 2021 13:42:07 +0100
> Cc: Alan Third <alan@idiocy.org>, YAMAMOTO Mitsuharu <mituharu@math.s.chiba-u.ac.jp>, 
> 	Emacs developers <emacs-devel@gnu.org>
> 
> > > I am concerned that there could be hidden issues with the branch since
> > > it doesn't seem to have had much testing, so it's probably best going
> > > into master, but I expect we'll see an increasing number of complaints
> > > about performance as more people switch to macOS 12. Perhaps we should
> > > keep it in mind as a possible update for 28.2, whenever that may be.
> >
> > It is fine with me to merge this to master, as a macOS-only feature,
> > unless Philipp has reasons not to do that yet.  We can reconsider
> > backporting to emacs-28 at a later date, if and when we get enough
> > complaints.
> 
> Is there a specific reason why we'd want to restrict this to macOS on
> master? If the concern is lack of test coverage, then using the branch
> on as many OSes as possible would help address that.

Since you didn't ask to merge it when I was asked about this, I
presumed that the branch was not yet ready for prime time, from your
POV.  On macOS this seems to be urgent due to incompatible changes by
Apple, but other platforms don't have much to gain, which is why I
answered as I did.

Re: master 2c79a8f 2/2: Use posix_spawn if possible.

[Philipp]

> Am 10.11.2021 um 15:10 schrieb Eli Zaretskii <eliz@gnu.org>:
> 
>> From: Philipp Stephani <p.stephani2@gmail.com>
>> Date: Wed, 10 Nov 2021 13:42:07 +0100
>> Cc: Alan Third <alan@idiocy.org>, YAMAMOTO Mitsuharu <mituharu@math.s.chiba-u.ac.jp>, 
>> 	Emacs developers <emacs-devel@gnu.org>
>> 
>>>> I am concerned that there could be hidden issues with the branch since
>>>> it doesn't seem to have had much testing, so it's probably best going
>>>> into master, but I expect we'll see an increasing number of complaints
>>>> about performance as more people switch to macOS 12. Perhaps we should
>>>> keep it in mind as a possible update for 28.2, whenever that may be.
>>> 
>>> It is fine with me to merge this to master, as a macOS-only feature,
>>> unless Philipp has reasons not to do that yet.  We can reconsider
>>> backporting to emacs-28 at a later date, if and when we get enough
>>> complaints.
>> 
>> Is there a specific reason why we'd want to restrict this to macOS on
>> master? If the concern is lack of test coverage, then using the branch
>> on as many OSes as possible would help address that.
> 
> Since you didn't ask to merge it when I was asked about this, I
> presumed that the branch was not yet ready for prime time, from your
> POV.  On macOS this seems to be urgent due to incompatible changes by
> Apple, but other platforms don't have much to gain, which is why I
> answered as I did.

I think it's ready for prime time.

Re: master 2c79a8f 2/2: Use posix_spawn if possible.

[Eli Zaretskii]

> From: Philipp <p.stephani2@gmail.com>
> Date: Thu, 11 Nov 2021 18:52:22 +0100
> Cc: Alan Third <alan@idiocy.org>,
>  YAMAMOTO Mitsuharu <mituharu@math.s.chiba-u.ac.jp>, emacs-devel@gnu.org
> 
> >> Is there a specific reason why we'd want to restrict this to macOS on
> >> master? If the concern is lack of test coverage, then using the branch
> >> on as many OSes as possible would help address that.
> > 
> > Since you didn't ask to merge it when I was asked about this, I
> > presumed that the branch was not yet ready for prime time, from your
> > POV.  On macOS this seems to be urgent due to incompatible changes by
> > Apple, but other platforms don't have much to gain, which is why I
> > answered as I did.
> 
> I think it's ready for prime time.

Then please land it on master, and thanks.

Re: master 2c79a8f 2/2: Use posix_spawn if possible.

[Philipp]

> Am 11.11.2021 um 19:00 schrieb Eli Zaretskii <eliz@gnu.org>:
> 
>> From: Philipp <p.stephani2@gmail.com>
>> Date: Thu, 11 Nov 2021 18:52:22 +0100
>> Cc: Alan Third <alan@idiocy.org>,
>> YAMAMOTO Mitsuharu <mituharu@math.s.chiba-u.ac.jp>, emacs-devel@gnu.org
>> 
>>>> Is there a specific reason why we'd want to restrict this to macOS on
>>>> master? If the concern is lack of test coverage, then using the branch
>>>> on as many OSes as possible would help address that.
>>> 
>>> Since you didn't ask to merge it when I was asked about this, I
>>> presumed that the branch was not yet ready for prime time, from your
>>> POV.  On macOS this seems to be urgent due to incompatible changes by
>>> Apple, but other platforms don't have much to gain, which is why I
>>> answered as I did.
>> 
>> I think it's ready for prime time.
> 
> Then please land it on master, and thanks.

Done, thanks.   (This time I checked that it doesn't break the build on Windows, macOS, and GNU/Linux beforehand.)

Re: master 2c79a8f 2/2: Use posix_spawn if possible.

[Philipp]

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



> Am 09.11.2021 um 19:12 schrieb Aaron Jensen <aaronjensen@gmail.com>:
> 
> On Tue, Nov 9, 2021 at 12:05 PM Eli Zaretskii <eliz@gnu.org> wrote:
>> 
>>> From: Aaron Jensen <aaronjensen@gmail.com>
>>> Date: Tue, 9 Nov 2021 10:57:11 -0500
>>> Cc: Emacs developers <emacs-devel@gnu.org>, Eli Zaretskii <eliz@gnu.org>, Alan Third <alan@idiocy.org>,
>>>      YAMAMOTO Mitsuharu <mituharu@math.s.chiba-u.ac.jp>
>>> On Tue, Nov 9, 2021 at 9:46 AM Philipp <p.stephani2@gmail.com> wrote:
>>>> 
>>>> I guess either way is fine.  I'd slightly lean towards enabling it whenever it's available (i.e. remove the condition on DARWIN_OS):
>>> 
>>> I'm good with that, I only added it because I believe in an earlier
>>> message in the thread that was called for.
>> 
>> Didn't you want to install this on the release branch?  I thought you
>> did, and that's why I asked for a patch that only affected macOS: on
>> the release branch we will not install such a significant change that
>> affects platforms which don't absolutely have to have it.
> 
> Not me, but I think that's a good idea. The Darwin only patch should
> install on emacs-28, so maybe we could install that there and the
> all-platform patch on master?

For now I did the second part.  There's a small slowdown on GNU/Linux (because posix_spawn performs a few additional safety checks), and a large speedup on macOS, see the attached plot.

[-- Attachment #2.1: Type: text/html, Size: 2550 bytes --]

[-- Attachment #2.2: spawn-benchmark.png --]
[-- Type: image/png, Size: 29519 bytes --]

Re: master 2c79a8f 2/2: Use posix_spawn if possible.

[Aaron Jensen]

On Fri, Nov 12, 2021 at 6:48 AM Philipp <p.stephani2@gmail.com> wrote:

> For now I did the second part.  There's a small slowdown on GNU/Linux (because posix_spawn performs a few additional safety checks), and a large speedup on macOS, see the attached plot

Great, thanks. I've added the patch to the homebrew installed
emacs-plus for version 28, which is one of the more popular ways of
installing Emacs on macOS. I believe a future version of emacs-mac
will have the patch applied too, so there's not necessarily a rush to
patch 28 and we will be getting some testing.

Out of curiosity, was your mac test on an M1? That's a surprisingly
low time if it's Intel.

Thanks,

Aaron

Re: master 2c79a8f 2/2: Use posix_spawn if possible.

[Alan Third]

On Fri, Nov 12, 2021 at 08:42:57AM -0500, Aaron Jensen wrote:
> On Fri, Nov 12, 2021 at 6:48 AM Philipp <p.stephani2@gmail.com> wrote:
> 
> > For now I did the second part.  There's a small slowdown on GNU/Linux (because posix_spawn performs a few additional safety checks), and a large speedup on macOS, see the attached plot
> 
> Great, thanks. I've added the patch to the homebrew installed
> emacs-plus for version 28, which is one of the more popular ways of
> installing Emacs on macOS. I believe a future version of emacs-mac
> will have the patch applied too, so there's not necessarily a rush to
> patch 28 and we will be getting some testing.

I think emacsformacosx.com builds dwarf pretty much any other macOS
builds in terms of user-base, and IIRC they're totally vanilla, so we
still might want to install this on the release branch only for macOS.
-- 
Alan Third

Re: master 2c79a8f 2/2: Use posix_spawn if possible.

[Aaron Jensen]

On Fri, Nov 12, 2021 at 5:05 PM Alan Third <alan@idiocy.org> wrote:
>
> On Fri, Nov 12, 2021 at 08:42:57AM -0500, Aaron Jensen wrote:
> > On Fri, Nov 12, 2021 at 6:48 AM Philipp <p.stephani2@gmail.com> wrote:
> >
> > > For now I did the second part.  There's a small slowdown on GNU/Linux (because posix_spawn performs a few additional safety checks), and a large speedup on macOS, see the attached plot
> >
> > Great, thanks. I've added the patch to the homebrew installed
> > emacs-plus for version 28, which is one of the more popular ways of
> > installing Emacs on macOS. I believe a future version of emacs-mac
> > will have the patch applied too, so there's not necessarily a rush to
> > patch 28 and we will be getting some testing.
>
> I think emacsformacosx.com builds dwarf pretty much any other macOS
> builds in terms of user-base, and IIRC they're totally vanilla, so we
> still might want to install this on the release branch only for macOS.

Ah, I didn't think about that one, good point. Then yeah, that seems
like a good idea to install to emacs-28. If that's done, please let me
know so I can remove the patch from emacs-plus.

Thanks,

Aaron

Re: master 2c79a8f 2/2: Use posix_spawn if possible.

[Philipp]

> Am 13.11.2021 um 15:08 schrieb Aaron Jensen <aaronjensen@gmail.com>:
> 
> On Fri, Nov 12, 2021 at 5:05 PM Alan Third <alan@idiocy.org> wrote:
>> 
>> On Fri, Nov 12, 2021 at 08:42:57AM -0500, Aaron Jensen wrote:
>>> On Fri, Nov 12, 2021 at 6:48 AM Philipp <p.stephani2@gmail.com> wrote:
>>> 
>>>> For now I did the second part.  There's a small slowdown on GNU/Linux (because posix_spawn performs a few additional safety checks), and a large speedup on macOS, see the attached plot
>>> 
>>> Great, thanks. I've added the patch to the homebrew installed
>>> emacs-plus for version 28, which is one of the more popular ways of
>>> installing Emacs on macOS. I believe a future version of emacs-mac
>>> will have the patch applied too, so there's not necessarily a rush to
>>> patch 28 and we will be getting some testing.
>> 
>> I think emacsformacosx.com builds dwarf pretty much any other macOS
>> builds in terms of user-base, and IIRC they're totally vanilla, so we
>> still might want to install this on the release branch only for macOS.
> 
> Ah, I didn't think about that one, good point. Then yeah, that seems
> like a good idea to install to emacs-28. If that's done, please let me
> know so I can remove the patch from emacs-plus.

I've backported the commit now onto the release branch.

Re: master 2c79a8f 2/2: Use posix_spawn if possible.

[Aaron Jensen]

On Sat, Nov 13, 2021 at 11:03 AM Philipp <p.stephani2@gmail.com> wrote:
>
> I've backported the commit now onto the release branch.

Thanks, I submitted a PR to remove the patch from emacs-plus.

Re: master 2c79a8f 2/2: Use posix_spawn if possible.

[Dmitry Gutov]

On 12.11.2021 14:48, Philipp wrote:
> For now I did the second part.  There's a small slowdown on GNU/Linux 
> (because posix_spawn performs a few additional safety checks), and a 
> large speedup on macOS, see the attached plot.

Forgive me if I'm out of context, but the commit message mentions being 
able to use "system-specific enhancements like 'clone' on Linux", and 
yet here in this table we get some slowdown on Linux.

Are those enhancements still to be added, or is the difference 
considered minor enough not to bother?

Re: master 2c79a8f 2/2: Use posix_spawn if possible.

[Saulius Menkevicius]

Hi,

I know this has been merged a couple of months ago to `master` but I 
would like to report breakage that occurs due to that commit.

We have csharp-ls (C#) and fsautocomplete (F#) LSP servers that stopped 
working with that commit (git-bisected to 
a60053f8368e058229721f1bf1567c2b1676b239).

I did not delve too much into the details or prepare a minimal test case 
but this appears to be an interplay between dotnet runtime (v6) and 
posix_spawn.

Not sure if that warrants a revert but just a heads-up.

-Saulius

Re: master 2c79a8f 2/2: Use posix_spawn if possible.

[Eli Zaretskii]

On January 25, 2022 8:48:12 AM GMT+02:00, Saulius Menkevicius <sauliusmenkevicius@fastmail.com> wrote:
> Hi,
> 
> I know this has been merged a couple of months ago to `master` but I 
> would like to report breakage that occurs due to that commit.
> 
> We have csharp-ls (C#) and fsautocomplete (F#) LSP servers that stopped 
> working with that commit (git-bisected to 
> a60053f8368e058229721f1bf1567c2b1676b239).
> 
> I did not delve too much into the details or prepare a minimal test case 
> but this appears to be an interplay between dotnet runtime (v6) and 
> posix_spawn.
> 
> Not sure if that warrants a revert but just a heads-up.

Can you explain how dotnet runtime comes into play here?  Does Emacs invoke a dotnet process or something?

And on what OS does this happen?

Re: master 2c79a8f 2/2: Use posix_spawn if possible.

[Saulius Menkevicius]

Sorry I did not mention the platform, this happens on Linux/x64 and has 
been reported by multiple persons:

- https://github.com/razzmatazz/csharp-language-server/issues/12


The issue has been noticed when dotnet-based LSP servers are used with 
emacs/lsp-mode, -- in particular lsp-mode starts the server using 
`make-process` and then communicates over stdio. Link to the code that 
launches the server:

- https://github.com/emacs-lsp/lsp-mode/blob/master/lsp-mode.el#L6925


We have csharp-ls and fsac servers launched with the same mechanism as 
are for other languages -- which are working ok with posix_spawn 
enabled. It only breaks for those before-mentioned LSP servers that are 
implemented on top of dotnet and use dotnet runtime (same thing as JVM, 
but for C#/F#/CLR languages).

Now it appears, that switch to posix_spawn broke communication over 
stdio to those dotnet-based LSP servers for some technical reason, -- I 
didn't investigate yet why, because it is a bit over my head. I *think* 
there is an interplay between posix_spawn-based process launch 
implementation in emacs and dotnet runtime stdio abstractions/platform 
layer -- because otherwise other language servers work with that commit 
that enables posix_spawn, like those based on JVM too.


I know this is a bit of a corner case as posix_spawn brings performance 
benefits, but just FYI.

BR,

-Saulius Menkevicius


Am 25.01.22 um 10:41 schrieb Eli Zaretskii:
> On January 25, 2022 8:48:12 AM GMT+02:00, Saulius Menkevicius <sauliusmenkevicius@fastmail.com> wrote:
>> Hi,
>>
>> I know this has been merged a couple of months ago to `master` but I
>> would like to report breakage that occurs due to that commit.
>>
>> We have csharp-ls (C#) and fsautocomplete (F#) LSP servers that stopped
>> working with that commit (git-bisected to
>> a60053f8368e058229721f1bf1567c2b1676b239).
>>
>> I did not delve too much into the details or prepare a minimal test case
>> but this appears to be an interplay between dotnet runtime (v6) and
>> posix_spawn.
>>
>> Not sure if that warrants a revert but just a heads-up.
> Can you explain how dotnet runtime comes into play here?  Does Emacs invoke a dotnet process or something?
>
> And on what OS does this happen?

Re: master 2c79a8f 2/2: Use posix_spawn if possible.

[Jostein Kjønigsen]

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

Like Saulius says, this is fairly technical and far above at least my head.

I still think it's worth discussing if we *need* this change for Linux. 
When the original commit was landed, (from what I can tell) it was 
because it was needed for Mac, and assumed harmless for other platforms. 
The merge to master was then meant as a testing-ground to see if it 
would cause issues.

And well.. Here's at least one issue :)

For perspective, over the last couple of years, Emacs as a 
editor/platform to work with .NET has improved tremendously to the point 
that Emacs is my primary editor for anything .NET. It would be a shame 
to see that completely break on Emacs 29, and being forced to use VSCode 
to get work done.

As things stand now, I think it sounds easier to revert this change (for 
Linux only) than trying to convince Microsoft to change the .NET runtime 
to better interop with Emacs on Linux :)

My 2 cents.

--
Kind regards
Jostein Kjønigsen

On 25.01.2022 09:58, Saulius Menkevicius wrote:
> Sorry I did not mention the platform, this happens on Linux/x64 and 
> has been reported by multiple persons:
>
> - https://github.com/razzmatazz/csharp-language-server/issues/12
>
>
> The issue has been noticed when dotnet-based LSP servers are used with 
> emacs/lsp-mode, -- in particular lsp-mode starts the server using 
> `make-process` and then communicates over stdio. Link to the code that 
> launches the server:
>
> - https://github.com/emacs-lsp/lsp-mode/blob/master/lsp-mode.el#L6925
>
>
> We have csharp-ls and fsac servers launched with the same mechanism as 
> are for other languages -- which are working ok with posix_spawn 
> enabled. It only breaks for those before-mentioned LSP servers that 
> are implemented on top of dotnet and use dotnet runtime (same thing as 
> JVM, but for C#/F#/CLR languages).
>
> Now it appears, that switch to posix_spawn broke communication over 
> stdio to those dotnet-based LSP servers for some technical reason, -- 
> I didn't investigate yet why, because it is a bit over my head. I 
> *think* there is an interplay between posix_spawn-based process launch 
> implementation in emacs and dotnet runtime stdio abstractions/platform 
> layer -- because otherwise other language servers work with that 
> commit that enables posix_spawn, like those based on JVM too.
>
>
> I know this is a bit of a corner case as posix_spawn brings 
> performance benefits, but just FYI.
>
> BR,
>
> -Saulius Menkevicius
>
>
> Am 25.01.22 um 10:41 schrieb Eli Zaretskii:
>> On January 25, 2022 8:48:12 AM GMT+02:00, Saulius Menkevicius 
>> <sauliusmenkevicius@fastmail.com> wrote:
>>> Hi,
>>>
>>> I know this has been merged a couple of months ago to `master` but I
>>> would like to report breakage that occurs due to that commit.
>>>
>>> We have csharp-ls (C#) and fsautocomplete (F#) LSP servers that stopped
>>> working with that commit (git-bisected to
>>> a60053f8368e058229721f1bf1567c2b1676b239).
>>>
>>> I did not delve too much into the details or prepare a minimal test 
>>> case
>>> but this appears to be an interplay between dotnet runtime (v6) and
>>> posix_spawn.
>>>
>>> Not sure if that warrants a revert but just a heads-up.
>> Can you explain how dotnet runtime comes into play here?  Does Emacs 
>> invoke a dotnet process or something?
>>
>> And on what OS does this happen?
>
-- 
Vennlig hilsen
*Jostein Kjønigsen*

jostein@kjonigsen.net 🍵 jostein@gmail.com 🍵 jostein@hufleslufs.no
https://jostein.kjønigsen.no <https://jostein.kjønigsen.no>

[-- Attachment #2: Type: text/html, Size: 5451 bytes --]

Re: master 2c79a8f 2/2: Use posix_spawn if possible.

[Po Lu]

Jostein Kjønigsen <jostein@secure.kjonigsen.net> writes:

> Like Saulius says, this is fairly technical and far above at least my head.
>
> I still think it's worth discussing if we *need* this change for
> Linux. When the original commit was landed, (from what I can tell) it
> was because it was needed for Mac, and assumed harmless for other
> platforms. The merge to master was then meant as a testing-ground to
> see if it would cause issues.

FWIW, I think use of posix_spawn should only be enabled on macOS, as a
work around for Apple's recent changes to vfork.  There is no reason to
risk needlessly breaking things for people.

Re: master 2c79a8f 2/2: Use posix_spawn if possible.

[Eli Zaretskii]

> Date: Tue, 25 Jan 2022 10:58:51 +0200
> Cc: p.stephani2@gmail.com, alan@idiocy.org, mituharu@math.s.chiba-u.ac.jp
> From: Saulius Menkevicius <sauliusmenkevicius@fastmail.com>
> 
> Sorry I did not mention the platform, this happens on Linux/x64 and has 
> been reported by multiple persons:
> 
> - https://github.com/razzmatazz/csharp-language-server/issues/12
> 
> 
> The issue has been noticed when dotnet-based LSP servers are used with 
> emacs/lsp-mode, -- in particular lsp-mode starts the server using 
> `make-process` and then communicates over stdio. Link to the code that 
> launches the server:
> 
> - https://github.com/emacs-lsp/lsp-mode/blob/master/lsp-mode.el#L6925
> 
> 
> We have csharp-ls and fsac servers launched with the same mechanism as 
> are for other languages -- which are working ok with posix_spawn 
> enabled. It only breaks for those before-mentioned LSP servers that are 
> implemented on top of dotnet and use dotnet runtime (same thing as JVM, 
> but for C#/F#/CLR languages).
> 
> Now it appears, that switch to posix_spawn broke communication over 
> stdio to those dotnet-based LSP servers for some technical reason, -- I 
> didn't investigate yet why, because it is a bit over my head. I *think* 
> there is an interplay between posix_spawn-based process launch 
> implementation in emacs and dotnet runtime stdio abstractions/platform 
> layer -- because otherwise other language servers work with that commit 
> that enables posix_spawn, like those based on JVM too.

What is special about dotnet's runtime stdio usage?

Re: master 2c79a8f 2/2: Use posix_spawn if possible.

[Saulius Menkevicius]

I certainly cannot answer that, it probably does some kind of sniffing 
on FDs and changes behaviour.

To actually figure that out I would need to build a minimal test fixture 
for this bug/issue and submit to dotnet/runtime repo on github for them 
to check and/or fix it.

-Saulius

Am 25.01.22 um 14:22 schrieb Eli Zaretskii:
>> Date: Tue, 25 Jan 2022 10:58:51 +0200
>> Cc: p.stephani2@gmail.com, alan@idiocy.org, mituharu@math.s.chiba-u.ac.jp
>> From: Saulius Menkevicius <sauliusmenkevicius@fastmail.com>
>>
>> Sorry I did not mention the platform, this happens on Linux/x64 and has
>> been reported by multiple persons:
>>
>> - https://github.com/razzmatazz/csharp-language-server/issues/12
>>
>>
>> The issue has been noticed when dotnet-based LSP servers are used with
>> emacs/lsp-mode, -- in particular lsp-mode starts the server using
>> `make-process` and then communicates over stdio. Link to the code that
>> launches the server:
>>
>> - https://github.com/emacs-lsp/lsp-mode/blob/master/lsp-mode.el#L6925
>>
>>
>> We have csharp-ls and fsac servers launched with the same mechanism as
>> are for other languages -- which are working ok with posix_spawn
>> enabled. It only breaks for those before-mentioned LSP servers that are
>> implemented on top of dotnet and use dotnet runtime (same thing as JVM,
>> but for C#/F#/CLR languages).
>>
>> Now it appears, that switch to posix_spawn broke communication over
>> stdio to those dotnet-based LSP servers for some technical reason, -- I
>> didn't investigate yet why, because it is a bit over my head. I *think*
>> there is an interplay between posix_spawn-based process launch
>> implementation in emacs and dotnet runtime stdio abstractions/platform
>> layer -- because otherwise other language servers work with that commit
>> that enables posix_spawn, like those based on JVM too.
> What is special about dotnet's runtime stdio usage?

Re: master 2c79a8f 2/2: Use posix_spawn if possible.

[Stefan Monnier]

> I know this has been merged a couple of months ago to `master` but I would
> like to report breakage that occurs due to that commit.
>
> We have csharp-ls (C#) and fsautocomplete (F#) LSP servers that stopped
> working with that commit (git-bisected to
> a60053f8368e058229721f1bf1567c2b1676b239).
>
> I did not delve too much into the details or prepare a minimal test case but
> this appears to be an interplay between dotnet runtime (v6) and posix_spawn.
>
> Not sure if that warrants a revert but just a heads-up.

Whatever may be decided w.r.t this, could you `M-x report-emacs-bug` so
we get a bug-nb for this problem?
[ And of course, please include as many details as possible about when/where
  the bug shows up, and even ideally some kind of recipe, as usual.  ]


        Stefan

Re: master 2c79a8f 2/2: Use posix_spawn if possible.

[Eli Zaretskii]

> Date: Tue, 25 Jan 2022 14:25:23 +0200
> From: Saulius Menkevicius <sauliusmenkevicius@fastmail.com>
> Cc: p.stephani2@gmail.com, alan@idiocy.org, mituharu@math.s.chiba-u.ac.jp,
>  emacs-devel@gnu.org
> 
> I certainly cannot answer that, it probably does some kind of sniffing 
> on FDs and changes behaviour.
> 
> To actually figure that out I would need to build a minimal test fixture 
> for this bug/issue and submit to dotnet/runtime repo on github for them 
> to check and/or fix it.

I think there's no way around this.  We need at least to understand
what part of posix_spawn code interferes with pipe-based I/O used by
these LSP servers, and why.

Re: master 2c79a8f 2/2: Use posix_spawn if possible.

[Matt Armstrong]

Eli Zaretskii <eliz@gnu.org> writes:

>> Date: Tue, 25 Jan 2022 14:25:23 +0200
>> From: Saulius Menkevicius <sauliusmenkevicius@fastmail.com>
>> Cc: p.stephani2@gmail.com, alan@idiocy.org, mituharu@math.s.chiba-u.ac.jp,
>>  emacs-devel@gnu.org
>> 
>> I certainly cannot answer that, it probably does some kind of sniffing 
>> on FDs and changes behaviour.
>> 
>> To actually figure that out I would need to build a minimal test fixture 
>> for this bug/issue and submit to dotnet/runtime repo on github for them 
>> to check and/or fix it.
>
> I think there's no way around this.  We need at least to understand
> what part of posix_spawn code interferes with pipe-based I/O used by
> these LSP servers, and why.

I don't find an emacs bug filed for this issue.  Saulius, it would be
good to file one.

This issue tickled a memory I had of Python moving away from posix_spawn
due to various portability issues: https://bugs.python.org/issue35823.
The issues they ran into and solved may inform this investigation.

Re: master 2c79a8f 2/2: Use posix_spawn if possible.

[Saulius Menkevičius]

I did not register it yet. I was thinking about making a reduced test suite for this (small dotnet app+emacs code) to replicate the issue first.

And then I am waiting for a M1 MacBook to have shipped to me to test if the commit to switch to posix_spawn broke just Linux or macOS too. I have reports of x64/Linux breakage only at this point.

—Saulius Menkevičius

> Am 2022-01-28 um 19:12 schrieb Matt Armstrong <matt@rfc20.org>:
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
>>> Date: Tue, 25 Jan 2022 14:25:23 +0200
>>> From: Saulius Menkevicius <sauliusmenkevicius@fastmail.com>
>>> Cc: p.stephani2@gmail.com, alan@idiocy.org, mituharu@math.s.chiba-u.ac.jp,
>>> emacs-devel@gnu.org
>>> 
>>> I certainly cannot answer that, it probably does some kind of sniffing 
>>> on FDs and changes behaviour.
>>> 
>>> To actually figure that out I would need to build a minimal test fixture 
>>> for this bug/issue and submit to dotnet/runtime repo on github for them 
>>> to check and/or fix it.
>> 
>> I think there's no way around this.  We need at least to understand
>> what part of posix_spawn code interferes with pipe-based I/O used by
>> these LSP servers, and why.
> 
> I don't find an emacs bug filed for this issue.  Saulius, it would be
> good to file one.
> 
> This issue tickled a memory I had of Python moving away from posix_spawn
> due to various portability issues: https://bugs.python.org/issue35823.
> The issues they ran into and solved may inform this investigation.

Re: master 2c79a8f 2/2: Use posix_spawn if possible.

[Eli Zaretskii]

> From: Matt Armstrong <matt@rfc20.org>
> Cc: p.stephani2@gmail.com, alan@idiocy.org, mituharu@math.s.chiba-u.ac.jp,
>  emacs-devel@gnu.org
> Date: Fri, 28 Jan 2022 09:12:22 -0800
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> To actually figure that out I would need to build a minimal test fixture 
> >> for this bug/issue and submit to dotnet/runtime repo on github for them 
> >> to check and/or fix it.
> >
> > I think there's no way around this.  We need at least to understand
> > what part of posix_spawn code interferes with pipe-based I/O used by
> > these LSP servers, and why.
> 
> I don't find an emacs bug filed for this issue.  Saulius, it would be
> good to file one.
> 
> This issue tickled a memory I had of Python moving away from posix_spawn
> due to various portability issues: https://bugs.python.org/issue35823.
> The issues they ran into and solved may inform this investigation.

Thanks.

I see nothing there about C#, nor even about problems with stdio
redirection in subprocesses.  There's some reference to closing file
descriptors above 2, but AFAIU the problems in this bug report are
related to descriptors that aren't above 2.

Re: master 2c79a8f 2/2: Use posix_spawn if possible.

[Saulius Menkevicius]

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

I did a bit more investigation (and still don't have a reproduction 
vehicle) but it seems the problem has to do with signals (SIGCHLD in 
particular) rather than stdio redirection.

Stack trace in the child process shows it has launched a subprocess 
which has since exited but the child did not receive (?) SIGCHLD and 
appears to be blocked.

Dotnet stack trace is

|[bob@fedora emacs]$ dotnet stack report -p 53193 Thread (0xCFC9): 
[Native Frames] 
System.Private.CoreLib!System.Threading.WaitHandle.WaitOneNoCheck(int32) 
System.Private.CoreLib!System.Threading.WaitHandle.WaitOne(int32) 
System.Diagnostics.Process!System.Diagnostics.ProcessWaitState.WaitForExit(int32) 
System.Diagnostics.Process!System.Diagnostics.Process.WaitForExitCore(int32) 
Microsoft.Build.Locator!Microsoft.Build.Locator.DotNetSdkLocationHelper+<GetDotNetBasePaths>d__5.MoveNext() 
Microsoft.Build.Locator!Microsoft.Build.Locator.DotNetSdkLocationHelper+<GetInstances>d__4.MoveNext() 
Microsoft.Build.Locator!Microsoft.Build.Locator.MSBuildLocator+<GetInstances>d__20.MoveNext() 
System.Linq!System.Linq.Enumerable.TryGetFirst(class 
System.Collections.Generic.IEnumerable`1<!!0>,bool&) 
System.Linq!System.Linq.Enumerable.FirstOrDefault(class 
System.Collections.Generic.IEnumerable`1<!!0>) 
Microsoft.Build.Locator!Microsoft.Build.Locator.MSBuildLocator.RegisterDefaults() 
CSharpLanguageServer!CSharpLanguageServer.Program.entry(class 
System.String[]) |

Where "ps axl" shows there is a zombie process waiting to be collected 
but is not.

|0 1000 53193 53129 20 0 3437284 59416 - Ssl ? 0:00 
/home/bob/src/csharp-language-server/src/CSharpLanguageServer/bin/Debug/net6.0/CSharpLanguageServer 
0 1000 53203 53193 20 0 0 0 - Z ? 0:00 [dotnet] <defunct> |


Related emacs src/callproc.cs has code that has this comment:

/* Stop blocking SIGCHLD in the child.  */

But I really don't know what should I do to attempt to fix this/find the 
cause.

-Saulius

Am 29.01.22 um 10:26 schrieb Eli Zaretskii:
>> From: Matt Armstrong<matt@rfc20.org>
>> Cc:p.stephani2@gmail.com,alan@idiocy.org,mituharu@math.s.chiba-u.ac.jp,
>>   emacs-devel@gnu.org
>> Date: Fri, 28 Jan 2022 09:12:22 -0800
>>
>> Eli Zaretskii<eliz@gnu.org>  writes:
>>
>>>> To actually figure that out I would need to build a minimal test fixture
>>>> for this bug/issue and submit to dotnet/runtime repo on github for them
>>>> to check and/or fix it.
>>> I think there's no way around this.  We need at least to understand
>>> what part of posix_spawn code interferes with pipe-based I/O used by
>>> these LSP servers, and why.
>> I don't find an emacs bug filed for this issue.  Saulius, it would be
>> good to file one.
>>
>> This issue tickled a memory I had of Python moving away from posix_spawn
>> due to various portability issues:https://bugs.python.org/issue35823.
>> The issues they ran into and solved may inform this investigation.
> Thanks.
>
> I see nothing there about C#, nor even about problems with stdio
> redirection in subprocesses.  There's some reference to closing file
> descriptors above 2, but AFAIU the problems in this bug report are
> related to descriptors that aren't above 2.

[-- Attachment #2: Type: text/html, Size: 4596 bytes --]

Re: master 2c79a8f 2/2: Use posix_spawn if possible.

[Robert Pluim]

>>>>> On Mon, 31 Jan 2022 22:48:33 +0200, Saulius Menkevicius <sauliusmenkevicius@fastmail.com> said:

    Saulius> I did a bit more investigation (and still don't have a reproduction
    Saulius> vehicle) but it seems the problem has to do with signals (SIGCHLD in
    Saulius> particular) rather than stdio redirection.

    Saulius> Stack trace in the child process shows it has launched a subprocess
    Saulius> which has since exited but the child did not receive (?) SIGCHLD and
    Saulius> appears to be blocked.

    Saulius> Dotnet stack trace is

    Saulius> |[bob@fedora emacs]$ dotnet stack report -p 53193 Thread (0xCFC9):
    Saulius>  [Native Frames]
    Saulius>  System.Private.CoreLib!System.Threading.WaitHandle.WaitOneNoCheck(int32)
    Saulius>  System.Private.CoreLib!System.Threading.WaitHandle.WaitOne(int32)
    Saulius>  System.Diagnostics.Process!System.Diagnostics.ProcessWaitState.WaitForExit(int32)
    Saulius>  System.Diagnostics.Process!System.Diagnostics.Process.WaitForExitCore(int32)
    Saulius>  Microsoft.Build.Locator!Microsoft.Build.Locator.DotNetSdkLocationHelper+<GetDotNetBasePaths>d__5.MoveNext()
    Saulius>  Microsoft.Build.Locator!Microsoft.Build.Locator.DotNetSdkLocationHelper+<GetInstances>d__4.MoveNext()
    Saulius>  Microsoft.Build.Locator!Microsoft.Build.Locator.MSBuildLocator+<GetInstances>d__20.MoveNext()
    Saulius>  System.Linq!System.Linq.Enumerable.TryGetFirst(class
    Saulius>  System.Collections.Generic.IEnumerable`1<!!0>,bool&)
    Saulius>  System.Linq!System.Linq.Enumerable.FirstOrDefault(class
    Saulius>  System.Collections.Generic.IEnumerable`1<!!0>)
    Saulius>  Microsoft.Build.Locator!Microsoft.Build.Locator.MSBuildLocator.RegisterDefaults()
    Saulius>  CSharpLanguageServer!CSharpLanguageServer.Program.entry(class
    Saulius>  System.String[]) |

    Saulius> Where "ps axl" shows there is a zombie process waiting to be collected
    Saulius> but is not.

    Saulius> |0 1000 53193 53129 20 0 3437284 59416 - Ssl ? 0:00
    Saulius>  /home/bob/src/csharp-language-server/src/CSharpLanguageServer/bin/Debug/net6.0/CSharpLanguageServer
    Saulius>  0 1000 53203 53193 20 0 0 0 - Z ? 0:00 [dotnet] <defunct> |


    Saulius> Related emacs src/callproc.cs has code that has this comment:

    Saulius> /* Stop blocking SIGCHLD in the child.  */

    Saulius> But I really don't know what should I do to attempt to fix this/find
    Saulius> the cause.

Despite the comment, that code doesnʼt actually unblock SIGCHLD, it
just sets the child mask to be the same as the parent, and SIGCHLD and
SIGINT are blocked in the parent at this point.

Try the following:

diff --git a/src/callproc.c b/src/callproc.c
index 4d3b0bb8e0..2b4e8977a3 100644
--- a/src/callproc.c
+++ b/src/callproc.c
@@ -1378,6 +1378,12 @@ emacs_posix_spawn_init_attributes (posix_spawnattr_t *attributes)
   /* Stop blocking SIGCHLD in the child.  */
   sigset_t oldset;
   error = pthread_sigmask (SIG_SETMASK, NULL, &oldset);
+  if (error != 0)
+    goto out;
+  error = sigdelset (&oldset, SIGCHLD);
+  if (error != 0)
+    goto out;
+  error = sigdelset (&oldset, SIGINT);
   if (error != 0)
     goto out;
   error = posix_spawnattr_setsigmask (attributes, &oldset);

Re: master 2c79a8f 2/2: Use posix_spawn if possible.

[Saulius Menkevicius]

Thanks Robert!

I applied your patch and that has actually has fixed the issue. So I 
suggest this patch be applied on master.

-Saulius

Am 01.02.22 um 11:59 schrieb Robert Pluim:
>>>>>> On Mon, 31 Jan 2022 22:48:33 +0200, Saulius Menkevicius <sauliusmenkevicius@fastmail.com> said:
>      Saulius> I did a bit more investigation (and still don't have a reproduction
>      Saulius> vehicle) but it seems the problem has to do with signals (SIGCHLD in
>      Saulius> particular) rather than stdio redirection.
>
>      Saulius> Stack trace in the child process shows it has launched a subprocess
>      Saulius> which has since exited but the child did not receive (?) SIGCHLD and
>      Saulius> appears to be blocked.
>
>      Saulius> Dotnet stack trace is
>
>      Saulius> |[bob@fedora emacs]$ dotnet stack report -p 53193 Thread (0xCFC9):
>      Saulius>  [Native Frames]
>      Saulius>  System.Private.CoreLib!System.Threading.WaitHandle.WaitOneNoCheck(int32)
>      Saulius>  System.Private.CoreLib!System.Threading.WaitHandle.WaitOne(int32)
>      Saulius>  System.Diagnostics.Process!System.Diagnostics.ProcessWaitState.WaitForExit(int32)
>      Saulius>  System.Diagnostics.Process!System.Diagnostics.Process.WaitForExitCore(int32)
>      Saulius>  Microsoft.Build.Locator!Microsoft.Build.Locator.DotNetSdkLocationHelper+<GetDotNetBasePaths>d__5.MoveNext()
>      Saulius>  Microsoft.Build.Locator!Microsoft.Build.Locator.DotNetSdkLocationHelper+<GetInstances>d__4.MoveNext()
>      Saulius>  Microsoft.Build.Locator!Microsoft.Build.Locator.MSBuildLocator+<GetInstances>d__20.MoveNext()
>      Saulius>  System.Linq!System.Linq.Enumerable.TryGetFirst(class
>      Saulius>  System.Collections.Generic.IEnumerable`1<!!0>,bool&)
>      Saulius>  System.Linq!System.Linq.Enumerable.FirstOrDefault(class
>      Saulius>  System.Collections.Generic.IEnumerable`1<!!0>)
>      Saulius>  Microsoft.Build.Locator!Microsoft.Build.Locator.MSBuildLocator.RegisterDefaults()
>      Saulius>  CSharpLanguageServer!CSharpLanguageServer.Program.entry(class
>      Saulius>  System.String[]) |
>
>      Saulius> Where "ps axl" shows there is a zombie process waiting to be collected
>      Saulius> but is not.
>
>      Saulius> |0 1000 53193 53129 20 0 3437284 59416 - Ssl ? 0:00
>      Saulius>  /home/bob/src/csharp-language-server/src/CSharpLanguageServer/bin/Debug/net6.0/CSharpLanguageServer
>      Saulius>  0 1000 53203 53193 20 0 0 0 - Z ? 0:00 [dotnet] <defunct> |
>
>
>      Saulius> Related emacs src/callproc.cs has code that has this comment:
>
>      Saulius> /* Stop blocking SIGCHLD in the child.  */
>
>      Saulius> But I really don't know what should I do to attempt to fix this/find
>      Saulius> the cause.
>
> Despite the comment, that code doesnʼt actually unblock SIGCHLD, it
> just sets the child mask to be the same as the parent, and SIGCHLD and
> SIGINT are blocked in the parent at this point.
>
> Try the following:
>
> diff --git a/src/callproc.c b/src/callproc.c
> index 4d3b0bb8e0..2b4e8977a3 100644
> --- a/src/callproc.c
> +++ b/src/callproc.c
> @@ -1378,6 +1378,12 @@ emacs_posix_spawn_init_attributes (posix_spawnattr_t *attributes)
>     /* Stop blocking SIGCHLD in the child.  */
>     sigset_t oldset;
>     error = pthread_sigmask (SIG_SETMASK, NULL, &oldset);
> +  if (error != 0)
> +    goto out;
> +  error = sigdelset (&oldset, SIGCHLD);
> +  if (error != 0)
> +    goto out;
> +  error = sigdelset (&oldset, SIGINT);
>     if (error != 0)
>       goto out;
>     error = posix_spawnattr_setsigmask (attributes, &oldset);

Re: master 2c79a8f 2/2: Use posix_spawn if possible.

[Robert Pluim]

>>>>> On Tue, 1 Feb 2022 20:30:18 +0200, Saulius Menkevicius <sauliusmenkevicius@fastmail.com> said:

    Saulius> Thanks Robert!
    Saulius> I applied your patch and that has actually has fixed the issue. So I
    Saulius> suggest this patch be applied on master.

I can do that, if Eli/Lars agree. What should we do for emacs-28?
Apply it there or switch back to fork/vfork?

Robert
-- 

Re: master 2c79a8f 2/2: Use posix_spawn if possible.

[Eli Zaretskii]

> From: Robert Pluim <rpluim@gmail.com>
> Cc: Eli Zaretskii <eliz@gnu.org>,  Matt Armstrong <matt@rfc20.org>,
>   p.stephani2@gmail.com,  alan@idiocy.org,  mituharu@math.s.chiba-u.ac.jp,
>   emacs-devel@gnu.org
> Date: Tue, 01 Feb 2022 20:23:23 +0100
> 
> >>>>> On Tue, 1 Feb 2022 20:30:18 +0200, Saulius Menkevicius <sauliusmenkevicius@fastmail.com> said:
> 
>     Saulius> Thanks Robert!
>     Saulius> I applied your patch and that has actually has fixed the issue. So I
>     Saulius> suggest this patch be applied on master.
> 
> I can do that, if Eli/Lars agree.

Are there any downsides to the patch?  If not, I see no reasons not to
install that.

> What should we do for emacs-28?

Nothing, because on emacs-28 we use posix_spawn only on macOS.

Re: master 2c79a8f 2/2: Use posix_spawn if possible.

[Robert Pluim]

>>>>> On Tue, 01 Feb 2022 21:52:18 +0200, Eli Zaretskii <eliz@gnu.org> said:

    >> From: Robert Pluim <rpluim@gmail.com>
    >> Cc: Eli Zaretskii <eliz@gnu.org>,  Matt Armstrong <matt@rfc20.org>,
    >> p.stephani2@gmail.com,  alan@idiocy.org,  mituharu@math.s.chiba-u.ac.jp,
    >> emacs-devel@gnu.org
    >> Date: Tue, 01 Feb 2022 20:23:23 +0100
    >> 
    >> >>>>> On Tue, 1 Feb 2022 20:30:18 +0200, Saulius Menkevicius <sauliusmenkevicius@fastmail.com> said:
    >> 
    Saulius> Thanks Robert!
    Saulius> I applied your patch and that has actually has fixed the issue. So I
    Saulius> suggest this patch be applied on master.
    >> 
    >> I can do that, if Eli/Lars agree.

    Eli> Are there any downsides to the patch?  If not, I see no reasons not to
    Eli> install that.

Not as far as I can tell, but I shall test on a few more platforms
before pushing.

    >> What should we do for emacs-28?

    Eli> Nothing, because on emacs-28 we use posix_spawn only on macOS.

Right, Iʼd missed that, master it is then. We could put it in 28.2, I
guess, but I hadn't noticed any issues with subprocesses on emacs-28
on macOS.

Robert
-- 

Re: master 2c79a8f 2/2: Use posix_spawn if possible.

[Saulius Menkevičius]

I’ve just received a new Mac (arm64/M1) for myself so I can test if 28 has it broken on macOS too.

—Saulius Menkevičius

> Am 2022-02-02 um 10:30 schrieb Robert Pluim <rpluim@gmail.com>:
> 
> 
>> 
>>>>>> On Tue, 01 Feb 2022 21:52:18 +0200, Eli Zaretskii <eliz@gnu.org> said:
> 
>>> From: Robert Pluim <rpluim@gmail.com>
>>> Cc: Eli Zaretskii <eliz@gnu.org>,  Matt Armstrong <matt@rfc20.org>,
>>> p.stephani2@gmail.com,  alan@idiocy.org,  mituharu@math.s.chiba-u.ac.jp,
>>> emacs-devel@gnu.org
>>> Date: Tue, 01 Feb 2022 20:23:23 +0100
>>> 
>>>>>>>> On Tue, 1 Feb 2022 20:30:18 +0200, Saulius Menkevicius <sauliusmenkevicius@fastmail.com> said:
>>> 
>    Saulius> Thanks Robert!
>    Saulius> I applied your patch and that has actually has fixed the issue. So I
>    Saulius> suggest this patch be applied on master.
>>> 
>>> I can do that, if Eli/Lars agree.
> 
>    Eli> Are there any downsides to the patch?  If not, I see no reasons not to
>    Eli> install that.
> 
> Not as far as I can tell, but I shall test on a few more platforms
> before pushing.
> 
>>> What should we do for emacs-28?
> 
>    Eli> Nothing, because on emacs-28 we use posix_spawn only on macOS.
> 
> Right, Iʼd missed that, master it is then. We could put it in 28.2, I
> guess, but I hadn't noticed any issues with subprocesses on emacs-28
> on macOS.
> 
> Robert
> -- 

Re: master 2c79a8f 2/2: Use posix_spawn if possible.

[Saulius Menkevicius]

Hello Robert, Eli,

I can confirm that this patch is needed for emacs-28 too, as currently emacs on macOS blocks SIGCHLD the same way as it does on Linux.

Applying Robert’s patch on emacs-28 fixes the issue on macOS.

Tested on macOS 12.2 / aarch64.

-Saulius

> Am 2022-02-02 um 10:54 schrieb Saulius Menkevičius <sauliusmenkevicius@fastmail.com>:
> 
> I’ve just received a new Mac (arm64/M1) for myself so I can test if 28 has it broken on macOS too.
> 
> —Saulius Menkevičius
> 
>> Am 2022-02-02 um 10:30 schrieb Robert Pluim <rpluim@gmail.com>:
>> 
>> 
>>> 
>>>>>>> On Tue, 01 Feb 2022 21:52:18 +0200, Eli Zaretskii <eliz@gnu.org> said:
>> 
>>>> From: Robert Pluim <rpluim@gmail.com>
>>>> Cc: Eli Zaretskii <eliz@gnu.org>,  Matt Armstrong <matt@rfc20.org>,
>>>> p.stephani2@gmail.com,  alan@idiocy.org,  mituharu@math.s.chiba-u.ac.jp,
>>>> emacs-devel@gnu.org
>>>> Date: Tue, 01 Feb 2022 20:23:23 +0100
>>>> 
>>>>>>>>> On Tue, 1 Feb 2022 20:30:18 +0200, Saulius Menkevicius <sauliusmenkevicius@fastmail.com> said:
>>>> 
>>   Saulius> Thanks Robert!
>>   Saulius> I applied your patch and that has actually has fixed the issue. So I
>>   Saulius> suggest this patch be applied on master.
>>>> 
>>>> I can do that, if Eli/Lars agree.
>> 
>>   Eli> Are there any downsides to the patch?  If not, I see no reasons not to
>>   Eli> install that.
>> 
>> Not as far as I can tell, but I shall test on a few more platforms
>> before pushing.
>> 
>>>> What should we do for emacs-28?
>> 
>>   Eli> Nothing, because on emacs-28 we use posix_spawn only on macOS.
>> 
>> Right, Iʼd missed that, master it is then. We could put it in 28.2, I
>> guess, but I hadn't noticed any issues with subprocesses on emacs-28
>> on macOS.
>> 
>> Robert
>> -- 

Re: master 2c79a8f 2/2: Use posix_spawn if possible.

[Robert Pluim]

>>>>> On Mon, 7 Feb 2022 23:12:27 +0200, Saulius Menkevicius <sauliusmenkevicius@fastmail.com> said:

    Saulius> Hello Robert, Eli,
    Saulius> I can confirm that this patch is needed for emacs-28 too, as currently
    Saulius> emacs on macOS blocks SIGCHLD the same way as it does on Linux.

    Saulius> Applying Robert’s patch on emacs-28 fixes the issue on macOS.

    Saulius> Tested on macOS 12.2 / aarch64.

OK. I haven't found any problems with it so far. Eli, where do you
want me to push it? (I could argue itʼs a regression, since emacs-27
used fork/vfork on macOS, and thus didnʼt have this issue).

Robert
-- 

Re: master 2c79a8f 2/2: Use posix_spawn if possible.

[Eli Zaretskii]

> From: Saulius Menkevicius <sauliusmenkevicius@fastmail.com>
> Date: Mon, 7 Feb 2022 23:12:27 +0200
> Cc: Alan Third <alan@idiocy.org>, emacs-devel@gnu.org, p.stephani2@gmail.com,
>  Matt Armstrong <matt@rfc20.org>, Eli Zaretskii <eliz@gnu.org>,
>  mituharu@math.s.chiba-u.ac.jp
> 
> Hello Robert, Eli,
> 
> I can confirm that this patch is needed for emacs-28 too, as currently emacs on macOS blocks SIGCHLD the same way as it does on Linux.
> 
> Applying Robert’s patch on emacs-28 fixes the issue on macOS.

How important is this on macOS?  I _really_ don't want to start
experiments with signals on the release branch, I prefer to tell users
C# is broken on macOS unless they either build without posix_spawn or
use the master branch.

Re: master 2c79a8f 2/2: Use posix_spawn if possible.

[Saulius Menkevicius]

I believe this is not a problem related to C#/dotnet but generally with 
how signals are blocked on child processes launched with posix_spawn in 
the current implementation.

I will try to provide a minimal replication code that does not have 
anything to do with dotnet/C#.

-Saulius

Am 08.02.22 um 14:12 schrieb Eli Zaretskii:
>> From: Saulius Menkevicius <sauliusmenkevicius@fastmail.com>
>> Date: Mon, 7 Feb 2022 23:12:27 +0200
>> Cc: Alan Third <alan@idiocy.org>, emacs-devel@gnu.org, p.stephani2@gmail.com,
>>   Matt Armstrong <matt@rfc20.org>, Eli Zaretskii <eliz@gnu.org>,
>>   mituharu@math.s.chiba-u.ac.jp
>>
>> Hello Robert, Eli,
>>
>> I can confirm that this patch is needed for emacs-28 too, as currently emacs on macOS blocks SIGCHLD the same way as it does on Linux.
>>
>> Applying Robert’s patch on emacs-28 fixes the issue on macOS.
> How important is this on macOS?  I _really_ don't want to start
> experiments with signals on the release branch, I prefer to tell users
> C# is broken on macOS unless they either build without posix_spawn or
> use the master branch.

Re: master 2c79a8f 2/2: Use posix_spawn if possible.

[Robert Pluim]

>>>>> On Tue, 8 Feb 2022 14:18:43 +0200, Saulius Menkevicius <sauliusmenkevicius@fastmail.com> said:

    Saulius> I believe this is not a problem related to C#/dotnet but generally
    Saulius> with how signals are blocked on child processes launched with
    Saulius> posix_spawn in the current implementation.

(I could argue that if dotnet wants to ensure it receives SIGCHLD, it
should unblock it itself, but thatʼs a different discussion, we
definitely have an Emacs bug)

And come to think of it, the posix_spawn code path is only exercised
when not asking for a pseudo tty. Since the default value for
`process-connection-type' is t => pty, Iʼm assuming that the elisp
code in question is binding that to nil, or using the :connection-type
argument to `make-process'.

Would it be possible to test using ptys? If that works thatʼs a
workaround that requires no changes to emacs-28.

Thanks

Robert
-- 

Re: master 2c79a8f 2/2: Use posix_spawn if possible.

[Saulius Menkevicius]

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

Robert you are correct about ``:connection-type''. In particular lsp-mode specifies ``:connection-type pty'' here:

- https://github.com/emacs-lsp/lsp-mode/blob/master/lsp-mode.el#L6932 <https://github.com/emacs-lsp/lsp-mode/blob/master/lsp-mode.el#L6932>

As a workaround fsharp/csharp language servers can use shell wrapper for launching the server binaries, as "#!/bin/bash" will apparently normalize signal mask before launching sub-subprocess and things work alright from thereon.

This is not as nice as having things just working before this change to `make-process`, but at least we have some way to work around this issue..

-Saulius

> Am 2022-02-08 um 16:59 schrieb Robert Pluim <rpluim@gmail.com>:
> 
>>>>>> On Tue, 8 Feb 2022 14:18:43 +0200, Saulius Menkevicius <sauliusmenkevicius@fastmail.com> said:
> 
>    Saulius> I believe this is not a problem related to C#/dotnet but generally
>    Saulius> with how signals are blocked on child processes launched with
>    Saulius> posix_spawn in the current implementation.
> 
> (I could argue that if dotnet wants to ensure it receives SIGCHLD, it
> should unblock it itself, but thatʼs a different discussion, we
> definitely have an Emacs bug)
> 
> And come to think of it, the posix_spawn code path is only exercised
> when not asking for a pseudo tty. Since the default value for
> `process-connection-type' is t => pty, Iʼm assuming that the elisp
> code in question is binding that to nil, or using the :connection-type
> argument to `make-process'.
> 
> Would it be possible to test using ptys? If that works thatʼs a
> workaround that requires no changes to emacs-28.
> 
> Thanks
> 
> Robert
> -- 


[-- Attachment #2: Type: text/html, Size: 2885 bytes --]

Re: master 2c79a8f 2/2: Use posix_spawn if possible.

[Robert Pluim]

>>>>> On Tue, 8 Feb 2022 23:09:20 +0200, Saulius Menkevicius <sauliusmenkevicius@fastmail.com> said:

    Saulius> Robert you are correct about ``:connection-type''. In particular
    Saulius> lsp-mode specifies ``:connection-type pty'' here:

':connection-type pipe', you mean. Does it still work if you use
'pty'?

    Saulius> - https://github.com/emacs-lsp/lsp-mode/blob/master/lsp-mode.el#L6932
    Saulius> <https://github.com/emacs-lsp/lsp-mode/blob/master/lsp-mode.el#L6932>

    Saulius> As a workaround fsharp/csharp language servers can use shell wrapper
    Saulius> for launching the server binaries, as "#!/bin/bash" will apparently
    Saulius> normalize signal mask before launching sub-subprocess and things work
    Saulius> alright from thereon.

Thatʼs to be expected. bash is a caring parent :-)

    Saulius> This is not as nice as having things just working before this change
    Saulius> to `make-process`, but at least we have some way to work around this
    Saulius> issue..

OK. In that case Iʼll put it only in master.

Robert
-- 

Re: master 2c79a8f 2/2: Use posix_spawn if possible.

[Saulius Menkevicius]

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

Correct, I meant ":connection-type pipe“ when I referred to lsp-mode. My mistake. Curiously setting :connect-type to ’pty does not fix my issues on macOS+emacs-28 for me. I might be doing something wrong though.. Applying Robert’s patch does fix it on emacs-28 too.

And I was mistaken about /bin/bash reseting signal mask, ksh does it though.. https://unix.stackexchange.com/questions/123768/calling-sigprocmask-from-bash <https://unix.stackexchange.com/questions/123768/calling-sigprocmask-from-bash>.

So my workarounds to fix sigmask are as follows:
- on linux, I will wrap invocation of server process with „/usr/bin/env --default-signal“
- on macos, I will wrap invocation of server process with „/bin/ksh -c“
- .. not sure what is needed on *BSDs, probably the same thing as on macOS if they have /bin/ksh in default installation?

Are there any chances of getting this patch to fix signal mask to default after posix_spawn from Robert getting merged to master and/or emacs-28?

-Saulius


> Am 2022-02-09 um 10:48 schrieb Robert Pluim <rpluim@gmail.com>:
> 
>>>>>> On Tue, 8 Feb 2022 23:09:20 +0200, Saulius Menkevicius <sauliusmenkevicius@fastmail.com> said:
> 
>    Saulius> Robert you are correct about ``:connection-type''. In particular
>    Saulius> lsp-mode specifies ``:connection-type pty'' here:
> 
> ':connection-type pipe', you mean. Does it still work if you use
> 'pty'?
> 
>    Saulius> - https://github.com/emacs-lsp/lsp-mode/blob/master/lsp-mode.el#L6932
>    Saulius> <https://github.com/emacs-lsp/lsp-mode/blob/master/lsp-mode.el#L6932>
> 
>    Saulius> As a workaround fsharp/csharp language servers can use shell wrapper
>    Saulius> for launching the server binaries, as "#!/bin/bash" will apparently
>    Saulius> normalize signal mask before launching sub-subprocess and things work
>    Saulius> alright from thereon.
> 
> Thatʼs to be expected. bash is a caring parent :-)
> 
>    Saulius> This is not as nice as having things just working before this change
>    Saulius> to `make-process`, but at least we have some way to work around this
>    Saulius> issue..
> 
> OK. In that case Iʼll put it only in master.
> 
> Robert
> -- 


[-- Attachment #2: Type: text/html, Size: 3901 bytes --]

Re: master 2c79a8f 2/2: Use posix_spawn if possible.

[Eli Zaretskii]

> From: Saulius Menkevicius <sauliusmenkevicius@fastmail.com>
> Date: Sat, 12 Feb 2022 10:44:21 +0200
> Cc: Eli Zaretskii <eliz@gnu.org>,
>  Alan Third <alan@idiocy.org>,
>  Emacs developers <emacs-devel@gnu.org>,
>  p.stephani2@gmail.com,
>  Matt Armstrong <matt@rfc20.org>,
>  mituharu@math.s.chiba-u.ac.jp
> 
> Are there any chances of getting this patch to fix signal mask to default after posix_spawn from Robert
> getting merged to master and/or emacs-28?

From my POV, it's too late to make such changes on the emacs-28
branch.  We are too close to a release.

Re: master 2c79a8f 2/2: Use posix_spawn if possible.

[Saulius Menkevicius]

Very understandable.

OK, will proceed to apply workarounds in lsp-mode.

> Am 2022-02-12 um 10:59 schrieb Eli Zaretskii <eliz@gnu.org>:
> 
>> From: Saulius Menkevicius <sauliusmenkevicius@fastmail.com>
>> Date: Sat, 12 Feb 2022 10:44:21 +0200
>> Cc: Eli Zaretskii <eliz@gnu.org>,
>> Alan Third <alan@idiocy.org>,
>> Emacs developers <emacs-devel@gnu.org>,
>> p.stephani2@gmail.com,
>> Matt Armstrong <matt@rfc20.org>,
>> mituharu@math.s.chiba-u.ac.jp
>> 
>> Are there any chances of getting this patch to fix signal mask to default after posix_spawn from Robert
>> getting merged to master and/or emacs-28?
> 
> From my POV, it's too late to make such changes on the emacs-28
> branch.  We are too close to a release.