git does not wait on hook exes (^C)

git does not wait on hook exes (^C)

[Anthony Sottile]

Here's a small example:

```bash
#!/usr/bin/env bash
foo() {
    echo zzz part 2
    sleep 1
    echo exiting now
}
trap foo SIGINT

echo zzz part 1
sleep 10
```

```console
$ git --version
git version 2.25.GIT
$ git commit --allow-empty -m foo
zzz part 1
^Czzz part 2

$ exiting now
```

- I pressed ^C during the first sleep
- control was returned back to my terminal
- the hook script was still running in the background

Anthony

Re: git does not wait on hook exes (^C)

[brian m. carlson]

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

On 2020-02-20 at 21:14:55, Anthony Sottile wrote:
> Here's a small example:
> 
> ```bash
> #!/usr/bin/env bash
> foo() {
>     echo zzz part 2
>     sleep 1
>     echo exiting now
> }
> trap foo SIGINT
> 
> echo zzz part 1
> sleep 10
> ```
> 
> ```console
> $ git --version
> git version 2.25.GIT
> $ git commit --allow-empty -m foo
> zzz part 1
> ^Czzz part 2
> 
> $ exiting now
> ```
> 
> - I pressed ^C during the first sleep
> - control was returned back to my terminal
> - the hook script was still running in the background

I believe the way that SIGINT works on a terminal is that it sends the
signal to all processes in the foreground process group.  So my guess of
what's happening here is that Git and your script both get SIGINT, Git
cleans up and exits quickly, leaving your script running.

If so, I'm not sure that Git can do much here.  If Git waited for the
hook to exit, then a broken or hung hook would cause Git to hang
indefinitely, which is not what the user intended when they pressed
Ctrl-C.  Usually what the user wants is an immediate return to the
terminal in such a case, and I think most users would consider it a bug
if Git were to wait for its children.

Certainly I'm open to hearing other views on this, though.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]

Re: git does not wait on hook exes (^C)

[Anthony Sottile]

On Thu, Feb 20, 2020 at 4:01 PM brian m. carlson
<sandals@crustytoothpaste.net> wrote:
>
> On 2020-02-20 at 21:14:55, Anthony Sottile wrote:
> > Here's a small example:
> >
> > ```bash
> > #!/usr/bin/env bash
> > foo() {
> >     echo zzz part 2
> >     sleep 1
> >     echo exiting now
> > }
> > trap foo SIGINT
> >
> > echo zzz part 1
> > sleep 10
> > ```
> >
> > ```console
> > $ git --version
> > git version 2.25.GIT
> > $ git commit --allow-empty -m foo
> > zzz part 1
> > ^Czzz part 2
> >
> > $ exiting now
> > ```
> >
> > - I pressed ^C during the first sleep
> > - control was returned back to my terminal
> > - the hook script was still running in the background
>
> I believe the way that SIGINT works on a terminal is that it sends the
> signal to all processes in the foreground process group.  So my guess of
> what's happening here is that Git and your script both get SIGINT, Git
> cleans up and exits quickly, leaving your script running.
>
> If so, I'm not sure that Git can do much here.  If Git waited for the
> hook to exit, then a broken or hung hook would cause Git to hang
> indefinitely, which is not what the user intended when they pressed
> Ctrl-C.  Usually what the user wants is an immediate return to the
> terminal in such a case, and I think most users would consider it a bug
> if Git were to wait for its children.
>
> Certainly I'm open to hearing other views on this, though.
> --
> brian m. carlson: Houston, Texas, US
> OpenPGP: https://keybase.io/bk2204


Taking git out of the situation:

Create a shell script:

```bash
#!/usr/bin/env bash
.git/hooks/pre-commit
```

```console
$ ./t.sh
zzz part 1
^Czzz part 2
exiting
$
```

that works fine (and is the expected case for `subprocess` calls for
example in python)

Anthony

Re: git does not wait on hook exes (^C)

[brian m. carlson]

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

On 2020-02-21 at 00:12:18, Anthony Sottile wrote:
> On Thu, Feb 20, 2020 at 4:01 PM brian m. carlson
> <sandals@crustytoothpaste.net> wrote:
> > I believe the way that SIGINT works on a terminal is that it sends the
> > signal to all processes in the foreground process group.  So my guess of
> > what's happening here is that Git and your script both get SIGINT, Git
> > cleans up and exits quickly, leaving your script running.
> >
> > If so, I'm not sure that Git can do much here.  If Git waited for the
> > hook to exit, then a broken or hung hook would cause Git to hang
> > indefinitely, which is not what the user intended when they pressed
> > Ctrl-C.  Usually what the user wants is an immediate return to the
> > terminal in such a case, and I think most users would consider it a bug
> > if Git were to wait for its children.
> 
> Taking git out of the situation:
> 
> Create a shell script:
> 
> ```bash
> #!/usr/bin/env bash
> .git/hooks/pre-commit
> ```
> 
> ```console
> $ ./t.sh
> zzz part 1
> ^Czzz part 2
> exiting
> $
> ```
> 
> that works fine (and is the expected case for `subprocess` calls for
> example in python)

Yeah, I think this is the case I was discussing up above, where the
parent process waits for the subprocess to exit.  If I modify the foo
function in your hook to also have a "sleep 10", then the parent process
hangs until the child process exits, which again would mean that Git
would hang indefinitely if the hook hung.

Can you maybe tell us a little more about your use case?  What are you
doing in your hook that makes this case come up?  Why is your hook
trapping SIGINT?
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]

Re: git does not wait on hook exes (^C)

[Anthony Sottile]

On Thu, Feb 20, 2020 at 5:11 PM brian m. carlson
<sandals@crustytoothpaste.net> wrote:
>
> On 2020-02-21 at 00:12:18, Anthony Sottile wrote:
> > On Thu, Feb 20, 2020 at 4:01 PM brian m. carlson
> > <sandals@crustytoothpaste.net> wrote:
> > > I believe the way that SIGINT works on a terminal is that it sends the
> > > signal to all processes in the foreground process group.  So my guess of
> > > what's happening here is that Git and your script both get SIGINT, Git
> > > cleans up and exits quickly, leaving your script running.
> > >
> > > If so, I'm not sure that Git can do much here.  If Git waited for the
> > > hook to exit, then a broken or hung hook would cause Git to hang
> > > indefinitely, which is not what the user intended when they pressed
> > > Ctrl-C.  Usually what the user wants is an immediate return to the
> > > terminal in such a case, and I think most users would consider it a bug
> > > if Git were to wait for its children.
> >
> > Taking git out of the situation:
> >
> > Create a shell script:
> >
> > ```bash
> > #!/usr/bin/env bash
> > .git/hooks/pre-commit
> > ```
> >
> > ```console
> > $ ./t.sh
> > zzz part 1
> > ^Czzz part 2
> > exiting
> > $
> > ```
> >
> > that works fine (and is the expected case for `subprocess` calls for
> > example in python)
>
> Yeah, I think this is the case I was discussing up above, where the
> parent process waits for the subprocess to exit.  If I modify the foo
> function in your hook to also have a "sleep 10", then the parent process
> hangs until the child process exits, which again would mean that Git
> would hang indefinitely if the hook hung.
>
> Can you maybe tell us a little more about your use case?  What are you
> doing in your hook that makes this case come up?  Why is your hook
> trapping SIGINT?
> --
> brian m. carlson: Houston, Texas, US
> OpenPGP: https://keybase.io/bk2204

My hook in question is a python process: https://pre-commit.com

It doesn't really do all that much on SIGINT but prints "(^C)
Interrupted" and offers a crash log when receiving ^C -- this races
with the git process terminating and causes terminal spew (sometimes
with pretty bad consequences with input breaking until `reset`
depending on which thing wins the tty reset race).

Anthony

Re: git does not wait on hook exes (^C)

[brian m. carlson]

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

On 2020-02-21 at 01:20:51, Anthony Sottile wrote:
> My hook in question is a python process: https://pre-commit.com
> 
> It doesn't really do all that much on SIGINT but prints "(^C)
> Interrupted" and offers a crash log when receiving ^C -- this races
> with the git process terminating and causes terminal spew (sometimes
> with pretty bad consequences with input breaking until `reset`
> depending on which thing wins the tty reset race).

Thanks, this is helpful context.  I don't know that Git waiting for the
process is going to fix the broken terminal state, although it will
likely fix the jumbled output.

I'm not planning on writing a patch, but I think an interesting patch to
see might be if we called wait(2) in a loop in a SIGINT handler but
didn't reinstall the signal handler, which means that a second Ctrl-C
would kill Git.  I believe that's what certain other programs do, and
that might address many of the problems in both scenarios.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]

Re: git does not wait on hook exes (^C)

[Jeff King]

On Fri, Feb 21, 2020 at 02:25:58AM +0000, brian m. carlson wrote:

> On 2020-02-21 at 01:20:51, Anthony Sottile wrote:
> > My hook in question is a python process: https://pre-commit.com
> > 
> > It doesn't really do all that much on SIGINT but prints "(^C)
> > Interrupted" and offers a crash log when receiving ^C -- this races
> > with the git process terminating and causes terminal spew (sometimes
> > with pretty bad consequences with input breaking until `reset`
> > depending on which thing wins the tty reset race).
> 
> Thanks, this is helpful context.  I don't know that Git waiting for the
> process is going to fix the broken terminal state, although it will
> likely fix the jumbled output.
> 
> I'm not planning on writing a patch, but I think an interesting patch to
> see might be if we called wait(2) in a loop in a SIGINT handler but
> didn't reinstall the signal handler, which means that a second Ctrl-C
> would kill Git.  I believe that's what certain other programs do, and
> that might address many of the problems in both scenarios.

The run-command struct has a clean_on_exit flag, as well as a
wait_after_clean flag, that would do what you want: when we're killed by
SIGINT, we'd pass the signal on to the child and then wait for to
finish.  That first step should generally be unnecessary for SIGINT
(since as you noted, it will usually be delivered to the whole process
group), but it shouldn't hurt.

To get the double-^C behavior, I think cleanup_children_on_signal()
would have to be reordered to pop the signal handler first before
calling cleanup_children().

I'm not quite convinced that's all worth doing, or wouldn't have other
unforeseen consequences. But if anybody is interested in experimenting,
I think the patch would only be a few lines (set those flags when
running hooks, and then that reordering).

-Peff

Re: git does not wait on hook exes (^C)

[Anthony Sottile]

On Thu, Feb 20, 2020 at 9:32 PM Jeff King <peff@peff.net> wrote:
>
> On Fri, Feb 21, 2020 at 02:25:58AM +0000, brian m. carlson wrote:
>
> > On 2020-02-21 at 01:20:51, Anthony Sottile wrote:
> > > My hook in question is a python process: https://pre-commit.com
> > >
> > > It doesn't really do all that much on SIGINT but prints "(^C)
> > > Interrupted" and offers a crash log when receiving ^C -- this races
> > > with the git process terminating and causes terminal spew (sometimes
> > > with pretty bad consequences with input breaking until `reset`
> > > depending on which thing wins the tty reset race).
> >
> > Thanks, this is helpful context.  I don't know that Git waiting for the
> > process is going to fix the broken terminal state, although it will
> > likely fix the jumbled output.
> >
> > I'm not planning on writing a patch, but I think an interesting patch to
> > see might be if we called wait(2) in a loop in a SIGINT handler but
> > didn't reinstall the signal handler, which means that a second Ctrl-C
> > would kill Git.  I believe that's what certain other programs do, and
> > that might address many of the problems in both scenarios.
>
> The run-command struct has a clean_on_exit flag, as well as a
> wait_after_clean flag, that would do what you want: when we're killed by
> SIGINT, we'd pass the signal on to the child and then wait for to
> finish.  That first step should generally be unnecessary for SIGINT
> (since as you noted, it will usually be delivered to the whole process
> group), but it shouldn't hurt.
>
> To get the double-^C behavior, I think cleanup_children_on_signal()
> would have to be reordered to pop the signal handler first before
> calling cleanup_children().
>
> I'm not quite convinced that's all worth doing, or wouldn't have other
> unforeseen consequences. But if anybody is interested in experimenting,
> I think the patch would only be a few lines (set those flags when
> running hooks, and then that reordering).
>
> -Peff

The small patch at least solves my issues and prevents the zombie
processes so I've sent it to the mailing list -- thanks for the
pointer.  I had found `wait_after_clean` in my initial investigation
but had missed the `clean_on_exit` flag!

Anthony