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

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.

[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.

[PATCH] posix_spawn blocks SIGCHLD in spawned processes (was: Re: master 2c79a8f 2/2: Use posix_spawn if possible.)

[Jürgen Hötzel]

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


Robert Pluim <rpluim@gmail.com> writes:

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

> 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;

^^^ This is the root-cause of the issue:
A new/invalid signal mask is introduced.
The correct oldset mask from emacs_spawn(...,const sigset_t *oldset) is
simply ignored.

>    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);
IMO the correct way to fix this issue is to use the oldset passed by
emacs_spawn (patch enclosed) just like the fork/exec implementation does.

I guess without a fix many forking commands (that don't examine and
change mask of blocked signals) will "hang" in Emacs.

"dotnet" is just a prominent one.

Jürgen


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: [PATCH] Use correct signal oldset in posix_spawn implementation --]
[-- Type: text/x-patch, Size: 2748 bytes --]

From f691f4c304d226f99ac0377de3b6186154c0d8fd Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?J=C3=BCrgen=20H=C3=B6tzel?= <juergen@archlinux.org>
Date: Fri, 4 Mar 2022 10:08:14 +0100
Subject: [PATCH] Use correct signal oldset in posix_spawn implementation

* src/callproc.c (emacs_spawn): Pass oldset parameter.
(emacs_posix_spawn_init_attributes): Use correct oldset.
(emacs_posix_spawn_init): remove intermediate function.
---
 src/callproc.c | 35 +++++++++--------------------------
 1 file changed, 9 insertions(+), 26 deletions(-)

diff --git a/src/callproc.c b/src/callproc.c
index 018c9ce690..0922e10f01 100644
--- a/src/callproc.c
+++ b/src/callproc.c
@@ -1335,7 +1335,8 @@ emacs_posix_spawn_init_actions (posix_spawn_file_actions_t *actions,
 }
 
 static int
-emacs_posix_spawn_init_attributes (posix_spawnattr_t *attributes)
+emacs_posix_spawn_init_attributes (posix_spawnattr_t *attributes,
+				   const sigset_t *oldset)
 {
   int error = posix_spawnattr_init (attributes);
   if (error != 0)
@@ -1377,11 +1378,7 @@ emacs_posix_spawn_init_attributes (posix_spawnattr_t *attributes)
     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);
+  error = posix_spawnattr_setsigmask (attributes, oldset);
   if (error != 0)
     goto out;
 
@@ -1392,23 +1389,6 @@ emacs_posix_spawn_init_attributes (posix_spawnattr_t *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
@@ -1443,9 +1423,12 @@ emacs_spawn (pid_t *newpid, int std_in, int std_out, int std_err,
   if (use_posix_spawn)
     {
       /* Initialize optional attributes before blocking. */
-      int error
-        = emacs_posix_spawn_init (&actions, &attributes, std_in,
-                                  std_out, std_err, 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, oldset);
       if (error != 0)
 	return error;
     }
-- 
2.35.1

Re: [PATCH] posix_spawn blocks SIGCHLD in spawned processes

[Robert Pluim]

>>>>> On Fri, 04 Mar 2022 09:38:25 +0100, Jürgen Hötzel <juergen@hoetzel.info> said:
    Jürgen> IMO the correct way to fix this issue is to use the oldset passed by
    Jürgen> emacs_spawn (patch enclosed) just like the fork/exec implementation does.

Sure, that would work as well.

    Jürgen> I guess without a fix many forking commands (that don't examine and
    Jürgen> change mask of blocked signals) will "hang" in Emacs.

Not really. Itʼs a code path thatʼs non-default, since
process-connection-type defaults to t => use pty's, and the command in
question has to care about its children.  But it needs to be fixed
regardless.

I donʼt have a test case, but the patch looks good to me.

Robert
-- 

Re: [PATCH] posix_spawn blocks SIGCHLD in spawned processes

[Jürgen Hötzel]

Robert Pluim <rpluim@gmail.com> writes:

>>>>>> On Fri, 04 Mar 2022 09:38:25 +0100, Jürgen Hötzel <juergen@hoetzel.info> said:
>     Jürgen> IMO the correct way to fix this issue is to use the oldset passed by
>     Jürgen> emacs_spawn (patch enclosed) just like the fork/exec implementation does.
>
> Sure, that would work as well.
>
>     Jürgen> I guess without a fix many forking commands (that don't examine and
>     Jürgen> change mask of blocked signals) will "hang" in Emacs.
>
> Not really. Itʼs a code path thatʼs non-default, since
> process-connection-type defaults to t => use pty's, and the command in

‘call-process’ invokes emacs_spawn 'call-process' with pty set to NULL
so ‘process-connection-type’ doesn't have an effect here.

A minimal C example to reproduce the HANG is

--8<---------------cut here---------------start hang.c------------->8---
#include <signal.h>
#include <stdio.h>
#include <sys/wait.h>
#include <unistd.h>

void chld_handler(int signum) { printf("\nInside CHLD handler function\n"); }

int main(int argc, char *argv[]) {
  sigset_t blocked, old;

  signal(SIGCHLD, chld_handler); // Setup Child handler
  sigemptyset(&blocked);
  sigaddset(&blocked, SIGCHLD);
  sigprocmask(SIG_BLOCK, &blocked, &old);
  if (!fork()) {
    return 0; /* child signals SIGCHLD */
  }
  sigsuspend(&old); /* WAITS for ever when SIGCHILD was originally blocked */
  printf("Parent terminates\n");
  return 0;
}
--8<---------------cut here---------------end hang.c--------------->8---


(call-process "chldtest" nil t nil) will never return.

Regards, Jürgen

Re: [PATCH] posix_spawn blocks SIGCHLD in spawned processes

[Robert Pluim]

>>>>> On Fri, 04 Mar 2022 16:41:15 +0100, Jürgen Hötzel <juergen@hoetzel.info> said:

    Jürgen> Robert Pluim <rpluim@gmail.com> writes:

    >>>>>>> On Fri, 04 Mar 2022 09:38:25 +0100, Jürgen Hötzel <juergen@hoetzel.info> said:
    Jürgen> IMO the correct way to fix this issue is to use the oldset passed by
    Jürgen> emacs_spawn (patch enclosed) just like the fork/exec implementation does.
    >> 
    >> Sure, that would work as well.
    >> 
    Jürgen> I guess without a fix many forking commands (that don't examine and
    Jürgen> change mask of blocked signals) will "hang" in Emacs.
    >> 
    >> Not really. Itʼs a code path thatʼs non-default, since
    >> process-connection-type defaults to t => use pty's, and the command in

    Jürgen> ‘call-process’ invokes emacs_spawn 'call-process' with pty set to NULL
    Jürgen> so ‘process-connection-type’ doesn't have an effect here.

Yes. And this change to use emacs_spawn has been in master for a
while, and we've had exactly one complaint, which leads me to suspect
that either people donʼt `call-process' processes that are affected by
the incorrect SIGCHLD handling, or that `make-process' is mostly used
with the default setting of `process-connection-type'.

Robert
-- 

Re: [PATCH] posix_spawn blocks SIGCHLD in spawned processes

[Philipp Stephani]

> Am 04.03.2022 um 16:41 schrieb Jürgen Hötzel <juergen@hoetzel.info>:
> 
> 
> Robert Pluim <rpluim@gmail.com> writes:
> 
>>>>>>> On Fri, 04 Mar 2022 09:38:25 +0100, Jürgen Hötzel <juergen@hoetzel.info> said:
>> Jürgen> IMO the correct way to fix this issue is to use the oldset passed by
>> Jürgen> emacs_spawn (patch enclosed) just like the fork/exec implementation does.
>> 
>> Sure, that would work as well.
>> 
>> Jürgen> I guess without a fix many forking commands (that don't examine and
>> Jürgen> change mask of blocked signals) will "hang" in Emacs.
>> 
>> Not really. Itʼs a code path thatʼs non-default, since
>> process-connection-type defaults to t => use pty's, and the command in
> 
> ‘call-process’ invokes emacs_spawn 'call-process' with pty set to NULL
> so ‘process-connection-type’ doesn't have an effect here.
> 
> A minimal C example to reproduce the HANG is
> 
> --8<---------------cut here---------------start hang.c------------->8---
> #include <signal.h>
> #include <stdio.h>
> #include <sys/wait.h>
> #include <unistd.h>
> 
> void chld_handler(int signum) { printf("\nInside CHLD handler function\n"); }
> 
> int main(int argc, char *argv[]) {
> sigset_t blocked, old;
> 
> signal(SIGCHLD, chld_handler); // Setup Child handler
> sigemptyset(&blocked);
> sigaddset(&blocked, SIGCHLD);
> sigprocmask(SIG_BLOCK, &blocked, &old);
> if (!fork()) {
> return 0; /* child signals SIGCHLD */
> }
> sigsuspend(&old); /* WAITS for ever when SIGCHILD was originally blocked */
> printf("Parent terminates\n");
> return 0;
> }
> --8<---------------cut here---------------end hang.c--------------->8---
> 
> 
> (call-process "chldtest" nil t nil) will never return.

If not already done, would you mind installing this example as a new regression test?  This area seems subtle enough that we'd benefit from testing the behavior explicitly.

Re: [PATCH] posix_spawn blocks SIGCHLD in spawned processes

[Robert Pluim]

>>>>> On Sun, 17 Apr 2022 20:22:13 +0200, Philipp Stephani <p.stephani2@gmail.com> said:

    Philipp> If not already done, would you mind installing this example as a new
    Philipp> regression test?  This area seems subtle enough that we'd benefit from
    Philipp> testing the behavior explicitly.

I donʼt think our test suite has support for compiling C programs
(Emacs C modules yes, but not standalone programs).

Robert
-- 

Re: [PATCH] posix_spawn blocks SIGCHLD in spawned processes

[Philipp Stephani]

> Am 19.04.2022 um 16:36 schrieb Robert Pluim <rpluim@gmail.com>:
> 
>>>>>> On Sun, 17 Apr 2022 20:22:13 +0200, Philipp Stephani <p.stephani2@gmail.com> said:
> 
>    Philipp> If not already done, would you mind installing this example as a new
>    Philipp> regression test?  This area seems subtle enough that we'd benefit from
>    Philipp> testing the behavior explicitly.
> 
> I donʼt think our test suite has support for compiling C programs
> (Emacs C modules yes, but not standalone programs).

I wouldn't expect that we'd need specific support for that, it's just another Make target.

Re: [PATCH] posix_spawn blocks SIGCHLD in spawned processes

[Eli Zaretskii]

> From: Robert Pluim <rpluim@gmail.com>
> Gmane-Reply-To-List: yes
> Date: Tue, 19 Apr 2022 16:36:03 +0200
> Cc: Alan Third <alan@idiocy.org>,
>  Jürgen Hötzel <juergen@hoetzel.info>,
>  emacs-devel@gnu.org, Saulius Menkevicius <sauliusmenkevicius@fastmail.com>,
>  Matt Armstrong <matt@rfc20.org>, Eli Zaretskii <eliz@gnu.org>,
>  YAMAMOTO Mitsuharu <mituharu@math.s.chiba-u.ac.jp>
> 
> >>>>> On Sun, 17 Apr 2022 20:22:13 +0200, Philipp Stephani <p.stephani2@gmail.com> said:
> 
>     Philipp> If not already done, would you mind installing this example as a new
>     Philipp> regression test?  This area seems subtle enough that we'd benefit from
>     Philipp> testing the behavior explicitly.
> 
> I donʼt think our test suite has support for compiling C programs
> (Emacs C modules yes, but not standalone programs).

You could perhaps use Emacs as such a "C program"?

Re: [PATCH] posix_spawn blocks SIGCHLD in spawned processes

[Robert Pluim]

>>>>> On Tue, 19 Apr 2022 19:07:35 +0300, Eli Zaretskii <eliz@gnu.org> said:
    >> I donʼt think our test suite has support for compiling C programs
    >> (Emacs C modules yes, but not standalone programs).

    Eli> You could perhaps use Emacs as such a "C program"?

Yes, except that running emacs from emacs doesnʼt fail the way the
example program does 🙂

Robert
-- 

Re: [PATCH] posix_spawn blocks SIGCHLD in spawned processes

[Robert Pluim]

>>>>> On Fri, 04 Mar 2022 11:07:52 +0100, Robert Pluim <rpluim@gmail.com> said:

>>>>> On Fri, 04 Mar 2022 09:38:25 +0100, Jürgen Hötzel <juergen@hoetzel.info> said:
    Jürgen> IMO the correct way to fix this issue is to use the oldset passed by
    Jürgen> emacs_spawn (patch enclosed) just like the fork/exec implementation does.

    Robert> Sure, that would work as well.

    Jürgen> I guess without a fix many forking commands (that don't examine and
    Jürgen> change mask of blocked signals) will "hang" in Emacs.

    Robert> Not really. Itʼs a code path thatʼs non-default, since
    Robert> process-connection-type defaults to t => use pty's, and the command in
    Robert> question has to care about its children.  But it needs to be fixed
    Robert> regardless.

    Robert> I donʼt have a test case, but the patch looks good to me.

Now applied to master as 8103b060d8 in Jürgen's name.

Robert
-- 

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