Patchwork dispatch: treat SIGPIPE as a termination signal (BC)

login
register
mail settings
Submitter Simon Farnsworth
Date Feb. 7, 2017, 8:08 p.m.
Message ID <e1150763706818fffa62.1486498109@devvm022.lla2.facebook.com>
Download mbox | patch
Permalink /patch/18340/
State Rejected
Headers show

Comments

Simon Farnsworth - Feb. 7, 2017, 8:08 p.m.
# HG changeset patch
# User Simon Farnsworth <simonfar@fb.com>
# Date 1486498104 28800
#      Tue Feb 07 12:08:24 2017 -0800
# Node ID e1150763706818fffa62c81a72a88574d20caea1
# Parent  1f51b4658f21bbb797e922d155c1046eddccf91d
dispatch: treat SIGPIPE as a termination signal (BC)

pager previously set SIGPIPE to immediately exit the process, ignoring any
Python @atexit handlers, exception handling etc - just instant termination.

Simply removing this as per changeset aaa751585325 meant that when you
aborted a long running pager command, Mercurial would not quit; instead, we
should add SIGPIPE to the list of termination signals in dispatch.py.

This is a slight BC break, as previously, a process that was piping data
into or out of Mercurial would not kill Mercurial if it died before closing
its end of the pipe, whereas it will now cause Mercurial to exit.
Jun Wu - Feb. 7, 2017, 8:17 p.m.
This approach looks good to me. The only problem is it will print "killed!"
on SIGPIPE so maybe a follow-up to make it silent.

Excerpts from Simon Farnsworth's message of 2017-02-07 12:08:29 -0800:
> # HG changeset patch
> # User Simon Farnsworth <simonfar@fb.com>
> # Date 1486498104 28800
> #      Tue Feb 07 12:08:24 2017 -0800
> # Node ID e1150763706818fffa62c81a72a88574d20caea1
> # Parent  1f51b4658f21bbb797e922d155c1046eddccf91d
> dispatch: treat SIGPIPE as a termination signal (BC)
> 
> pager previously set SIGPIPE to immediately exit the process, ignoring any
> Python @atexit handlers, exception handling etc - just instant termination.
> 
> Simply removing this as per changeset aaa751585325 meant that when you
> aborted a long running pager command, Mercurial would not quit; instead, we
> should add SIGPIPE to the list of termination signals in dispatch.py.
> 
> This is a slight BC break, as previously, a process that was piping data
> into or out of Mercurial would not kill Mercurial if it died before closing
> its end of the pipe, whereas it will now cause Mercurial to exit.
> 
> diff --git a/mercurial/dispatch.py b/mercurial/dispatch.py
> --- a/mercurial/dispatch.py
> +++ b/mercurial/dispatch.py
> @@ -147,7 +147,7 @@
>  
>      ui = req.ui
>      try:
> -        for name in 'SIGBREAK', 'SIGHUP', 'SIGTERM':
> +        for name in 'SIGBREAK', 'SIGHUP', 'SIGTERM', 'SIGPIPE':
>              num = getattr(signal, name, None)
>              if num:
>                  signal.signal(num, catchterm)
Jun Wu - Feb. 7, 2017, 8:23 p.m.
Actually that's not a problem since the output is redirected to the pager
and it won't get printed out.

Excerpts from Jun Wu's message of 2017-02-07 12:17:00 -0800:
> This approach looks good to me. The only problem is it will print "killed!"
> on SIGPIPE so maybe a follow-up to make it silent.
> 
> Excerpts from Simon Farnsworth's message of 2017-02-07 12:08:29 -0800:
> > # HG changeset patch
> > # User Simon Farnsworth <simonfar@fb.com>
> > # Date 1486498104 28800
> > #      Tue Feb 07 12:08:24 2017 -0800
> > # Node ID e1150763706818fffa62c81a72a88574d20caea1
> > # Parent  1f51b4658f21bbb797e922d155c1046eddccf91d
> > dispatch: treat SIGPIPE as a termination signal (BC)
> > 
> > pager previously set SIGPIPE to immediately exit the process, ignoring any
> > Python @atexit handlers, exception handling etc - just instant termination.
> > 
> > Simply removing this as per changeset aaa751585325 meant that when you
> > aborted a long running pager command, Mercurial would not quit; instead, we
> > should add SIGPIPE to the list of termination signals in dispatch.py.
> > 
> > This is a slight BC break, as previously, a process that was piping data
> > into or out of Mercurial would not kill Mercurial if it died before closing
> > its end of the pipe, whereas it will now cause Mercurial to exit.
> > 
> > diff --git a/mercurial/dispatch.py b/mercurial/dispatch.py
> > --- a/mercurial/dispatch.py
> > +++ b/mercurial/dispatch.py
> > @@ -147,7 +147,7 @@
> >  
> >      ui = req.ui
> >      try:
> > -        for name in 'SIGBREAK', 'SIGHUP', 'SIGTERM':
> > +        for name in 'SIGBREAK', 'SIGHUP', 'SIGTERM', 'SIGPIPE':
> >              num = getattr(signal, name, None)
> >              if num:
> >                  signal.signal(num, catchterm)
Simon Farnsworth - Feb. 7, 2017, 8:28 p.m.
On 07/02/2017 20:17, Jun Wu wrote:
> This approach looks good to me. The only problem is it will print "killed!"
> on SIGPIPE so maybe a follow-up to make it silent.
>

Is there a practical case where "killed!" on stderr when the command is 
aborted due to SIGPIPE is a bad thing?

In the pager case, this is only visible if stderr is redirected, but 
stdout is not - if stderr isn't redirected, then the pager (which has 
just exited - otherwise we wouldn't be getting SIGPIPE) will be sent the 
string, and won't print it.

In other cases, I think the extra message is desirable (and justifies 
the BC marker). It tells you that where old Mercurial would continue 
processing after your stdin feed or stdout capture died unexpectedly, 
this Mercurial now quits.

> Excerpts from Simon Farnsworth's message of 2017-02-07 12:08:29 -0800:
>> # HG changeset patch
>> # User Simon Farnsworth <simonfar@fb.com>
>> # Date 1486498104 28800
>> #      Tue Feb 07 12:08:24 2017 -0800
>> # Node ID e1150763706818fffa62c81a72a88574d20caea1
>> # Parent  1f51b4658f21bbb797e922d155c1046eddccf91d
>> dispatch: treat SIGPIPE as a termination signal (BC)
>>
>> pager previously set SIGPIPE to immediately exit the process, ignoring any
>> Python @atexit handlers, exception handling etc - just instant termination.
>>
>> Simply removing this as per changeset aaa751585325 meant that when you
>> aborted a long running pager command, Mercurial would not quit; instead, we
>> should add SIGPIPE to the list of termination signals in dispatch.py.
>>
>> This is a slight BC break, as previously, a process that was piping data
>> into or out of Mercurial would not kill Mercurial if it died before closing
>> its end of the pipe, whereas it will now cause Mercurial to exit.
>>
>> diff --git a/mercurial/dispatch.py b/mercurial/dispatch.py
>> --- a/mercurial/dispatch.py
>> +++ b/mercurial/dispatch.py
>> @@ -147,7 +147,7 @@
>>
>>      ui = req.ui
>>      try:
>> -        for name in 'SIGBREAK', 'SIGHUP', 'SIGTERM':
>> +        for name in 'SIGBREAK', 'SIGHUP', 'SIGTERM', 'SIGPIPE':
>>              num = getattr(signal, name, None)
>>              if num:
>>                  signal.signal(num, catchterm)
Augie Fackler - Feb. 8, 2017, 4:01 a.m.
> On Feb 7, 2017, at 15:28, Simon Farnsworth <simonfar@fb.com> wrote:
> 
> On 07/02/2017 20:17, Jun Wu wrote:
>> This approach looks good to me. The only problem is it will print "killed!"
>> on SIGPIPE so maybe a follow-up to make it silent.
>> 
> 
> Is there a practical case where "killed!" on stderr when the command is aborted due to SIGPIPE is a bad thing?
> 
> In the pager case, this is only visible if stderr is redirected, but stdout is not - if stderr isn't redirected, then the pager (which has just exited - otherwise we wouldn't be getting SIGPIPE) will be sent the string, and won't print it.
> 
> In other cases, I think the extra message is desirable (and justifies the BC marker). It tells you that where old Mercurial would continue processing after your stdin feed or stdout capture died unexpectedly, this Mercurial now quits.

Jun, to clarify this resolves what you spotted with the previous change and I should take it, yes?

Thanks!

> 
>> Excerpts from Simon Farnsworth's message of 2017-02-07 12:08:29 -0800:
>>> # HG changeset patch
>>> # User Simon Farnsworth <simonfar@fb.com>
>>> # Date 1486498104 28800
>>> #      Tue Feb 07 12:08:24 2017 -0800
>>> # Node ID e1150763706818fffa62c81a72a88574d20caea1
>>> # Parent  1f51b4658f21bbb797e922d155c1046eddccf91d
>>> dispatch: treat SIGPIPE as a termination signal (BC)
>>> 
>>> pager previously set SIGPIPE to immediately exit the process, ignoring any
>>> Python @atexit handlers, exception handling etc - just instant termination.
>>> 
>>> Simply removing this as per changeset aaa751585325 meant that when you
>>> aborted a long running pager command, Mercurial would not quit; instead, we
>>> should add SIGPIPE to the list of termination signals in dispatch.py.
>>> 
>>> This is a slight BC break, as previously, a process that was piping data
>>> into or out of Mercurial would not kill Mercurial if it died before closing
>>> its end of the pipe, whereas it will now cause Mercurial to exit.
>>> 
>>> diff --git a/mercurial/dispatch.py b/mercurial/dispatch.py
>>> --- a/mercurial/dispatch.py
>>> +++ b/mercurial/dispatch.py
>>> @@ -147,7 +147,7 @@
>>> 
>>>     ui = req.ui
>>>     try:
>>> -        for name in 'SIGBREAK', 'SIGHUP', 'SIGTERM':
>>> +        for name in 'SIGBREAK', 'SIGHUP', 'SIGTERM', 'SIGPIPE':
>>>             num = getattr(signal, name, None)
>>>             if num:
>>>                 signal.signal(num, catchterm)
> 
> -- 
> Simon Farnsworth
Jun Wu - Feb. 8, 2017, 4:23 a.m.
Excerpts from Augie Fackler's message of 2017-02-07 23:01:44 -0500:
> 
> > On Feb 7, 2017, at 15:28, Simon Farnsworth <simonfar@fb.com> wrote:
> > 
> > On 07/02/2017 20:17, Jun Wu wrote:
> >> This approach looks good to me. The only problem is it will print "killed!"
> >> on SIGPIPE so maybe a follow-up to make it silent.
> >> 
> > 
> > Is there a practical case where "killed!" on stderr when the command is aborted due to SIGPIPE is a bad thing?
> > 
> > In the pager case, this is only visible if stderr is redirected, but stdout is not - if stderr isn't redirected, then the pager (which has just exited - otherwise we wouldn't be getting SIGPIPE) will be sent the string, and won't print it.
> > 
> > In other cases, I think the extra message is desirable (and justifies the BC marker). It tells you that where old Mercurial would continue processing after your stdin feed or stdout capture died unexpectedly, this Mercurial now quits.
> 
> Jun, to clarify this resolves what you spotted with the previous change and I should take it, yes?

Yes. Thanks!

> 
> Thanks!
> 
> > 
> >> Excerpts from Simon Farnsworth's message of 2017-02-07 12:08:29 -0800:
> >>> # HG changeset patch
> >>> # User Simon Farnsworth <simonfar@fb.com>
> >>> # Date 1486498104 28800
> >>> #      Tue Feb 07 12:08:24 2017 -0800
> >>> # Node ID e1150763706818fffa62c81a72a88574d20caea1
> >>> # Parent  1f51b4658f21bbb797e922d155c1046eddccf91d
> >>> dispatch: treat SIGPIPE as a termination signal (BC)
> >>> 
> >>> pager previously set SIGPIPE to immediately exit the process, ignoring any
> >>> Python @atexit handlers, exception handling etc - just instant termination.
> >>> 
> >>> Simply removing this as per changeset aaa751585325 meant that when you
> >>> aborted a long running pager command, Mercurial would not quit; instead, we
> >>> should add SIGPIPE to the list of termination signals in dispatch.py.
> >>> 
> >>> This is a slight BC break, as previously, a process that was piping data
> >>> into or out of Mercurial would not kill Mercurial if it died before closing
> >>> its end of the pipe, whereas it will now cause Mercurial to exit.
> >>> 
> >>> diff --git a/mercurial/dispatch.py b/mercurial/dispatch.py
> >>> --- a/mercurial/dispatch.py
> >>> +++ b/mercurial/dispatch.py
> >>> @@ -147,7 +147,7 @@
> >>> 
> >>>     ui = req.ui
> >>>     try:
> >>> -        for name in 'SIGBREAK', 'SIGHUP', 'SIGTERM':
> >>> +        for name in 'SIGBREAK', 'SIGHUP', 'SIGTERM', 'SIGPIPE':
> >>>             num = getattr(signal, name, None)
> >>>             if num:
> >>>                 signal.signal(num, catchterm)
> > 
> > -- 
> > Simon Farnsworth
Simon Farnsworth - Feb. 8, 2017, 3:28 p.m.
Please scrub this - it breaks `hg serve`. I'll send a v2 shortly, that 
moves the SIGPIPE handling into pager and thus has no BC implications.

On 07/02/2017 20:08, Simon Farnsworth wrote:
> # HG changeset patch
> # User Simon Farnsworth <simonfar@fb.com>
> # Date 1486498104 28800
> #      Tue Feb 07 12:08:24 2017 -0800
> # Node ID e1150763706818fffa62c81a72a88574d20caea1
> # Parent  1f51b4658f21bbb797e922d155c1046eddccf91d
> dispatch: treat SIGPIPE as a termination signal (BC)
>
> pager previously set SIGPIPE to immediately exit the process, ignoring any
> Python @atexit handlers, exception handling etc - just instant termination.
>
> Simply removing this as per changeset aaa751585325 meant that when you
> aborted a long running pager command, Mercurial would not quit; instead, we
> should add SIGPIPE to the list of termination signals in dispatch.py.
>
> This is a slight BC break, as previously, a process that was piping data
> into or out of Mercurial would not kill Mercurial if it died before closing
> its end of the pipe, whereas it will now cause Mercurial to exit.
>
> diff --git a/mercurial/dispatch.py b/mercurial/dispatch.py
> --- a/mercurial/dispatch.py
> +++ b/mercurial/dispatch.py
> @@ -147,7 +147,7 @@
>
>      ui = req.ui
>      try:
> -        for name in 'SIGBREAK', 'SIGHUP', 'SIGTERM':
> +        for name in 'SIGBREAK', 'SIGHUP', 'SIGTERM', 'SIGPIPE':
>              num = getattr(signal, name, None)
>              if num:
>                  signal.signal(num, catchterm)
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://urldefense.proofpoint.com/v2/url?u=https-3A__www.mercurial-2Dscm.org_mailman_listinfo_mercurial-2Ddevel&d=DwIGaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=mEgSWILcY4c4W3zjApBQLA&m=I0DTIf8TaBNbZo11b6eM-M9WIaAj0Va9TR_a9fvAsvk&s=629TWBre1WtsIXDvJQh3UKr9baaDbd6LlVXnmQeyP5E&e=
>

Patch

diff --git a/mercurial/dispatch.py b/mercurial/dispatch.py
--- a/mercurial/dispatch.py
+++ b/mercurial/dispatch.py
@@ -147,7 +147,7 @@ 
 
     ui = req.ui
     try:
-        for name in 'SIGBREAK', 'SIGHUP', 'SIGTERM':
+        for name in 'SIGBREAK', 'SIGHUP', 'SIGTERM', 'SIGPIPE':
             num = getattr(signal, name, None)
             if num:
                 signal.signal(num, catchterm)