Patchwork pager: exit cleanly on SIGPIPE (BC)

login
register
mail settings
Submitter Simon Farnsworth
Date Feb. 8, 2017, 3:47 p.m.
Message ID <d50cda2a403786836d1f.1486568859@devvm022.lla2.facebook.com>
Download mbox | patch
Permalink /patch/18348/
State Accepted
Headers show

Comments

Simon Farnsworth - Feb. 8, 2017, 3:47 p.m.
# HG changeset patch
# User Simon Farnsworth <simonfar@fb.com>
# Date 1486568650 28800
#      Wed Feb 08 07:44:10 2017 -0800
# Node ID d50cda2a403786836d1f0d5c99401599dc4f43ec
# Parent  1f51b4658f21bbb797e922d155c1046eddccf91d
pager: exit cleanly on SIGPIPE (BC)

Changeset aaa751585325 removes SIGPIPE handling completely. This is wrong,
as it means that Mercurial does not exit when the pager does. Instead, raise
SignalInterrupt when SIGPIPE happens with a pager attached, to trigger the
normal exit path.

This will cause "killed!" to be printed to stderr (hence the BC warning),
but in the normal pager use case (where the pager gets both stderr and
stdout), this messsage is lost as we only get SIGPIPE when the pager quits.
Simon Farnsworth - Feb. 8, 2017, 3:53 p.m.
This replaces "dispatch: treat SIGPIPE as a termination signal (BC)".

I'm now debugging test-paths.t, so that I can stop being acclimatised to 
the red failure prompt after running tests, to improve my chances of 
noticing this sort of issue.

Simon

On 08/02/2017 15:47, Simon Farnsworth wrote:
> # HG changeset patch
> # User Simon Farnsworth <simonfar@fb.com>
> # Date 1486568650 28800
> #      Wed Feb 08 07:44:10 2017 -0800
> # Node ID d50cda2a403786836d1f0d5c99401599dc4f43ec
> # Parent  1f51b4658f21bbb797e922d155c1046eddccf91d
> pager: exit cleanly on SIGPIPE (BC)
>
> Changeset aaa751585325 removes SIGPIPE handling completely. This is wrong,
> as it means that Mercurial does not exit when the pager does. Instead, raise
> SignalInterrupt when SIGPIPE happens with a pager attached, to trigger the
> normal exit path.
>
> This will cause "killed!" to be printed to stderr (hence the BC warning),
> but in the normal pager use case (where the pager gets both stderr and
> stdout), this messsage is lost as we only get SIGPIPE when the pager quits.
>
> diff --git a/hgext/pager.py b/hgext/pager.py
> --- a/hgext/pager.py
> +++ b/hgext/pager.py
> @@ -72,6 +72,7 @@
>      commands,
>      dispatch,
>      encoding,
> +    error,
>      extensions,
>      util,
>      )
> @@ -114,6 +115,9 @@
>          os.dup2(stderrfd, util.stderr.fileno())
>          pager.wait()
>
> +def catchterm(*args):
> +    raise error.SignalInterrupt
> +
>  def uisetup(ui):
>      class pagerui(ui.__class__):
>          def _runpager(self, pagercmd):
> @@ -153,6 +157,8 @@
>          if usepager:
>              ui.setconfig('ui', 'formatted', ui.formatted(), 'pager')
>              ui.setconfig('ui', 'interactive', False, 'pager')
> +            if util.safehasattr(signal, "SIGPIPE"):
> +                signal.signal(signal.SIGPIPE, catchterm)
>              ui._runpager(p)
>          return orig(ui, options, cmd, cmdfunc)
>
> _______________________________________________
> 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=JGdkzd8E23MJxg59Iu-jc-4lKtVDPSGtiCxGk7Tvyfk&s=26T_I6At53KfO_UPKKWvM8tgFUbZStBE44m6jbEfsrk&e=
>
David Soria Parra - Feb. 8, 2017, 8:17 p.m.
On Wed, Feb 08, 2017 at 07:47:39AM -0800, Simon Farnsworth wrote:
> # HG changeset patch
> # User Simon Farnsworth <simonfar@fb.com>
> # Date 1486568650 28800
> #      Wed Feb 08 07:44:10 2017 -0800
> # Node ID d50cda2a403786836d1f0d5c99401599dc4f43ec
> # Parent  1f51b4658f21bbb797e922d155c1046eddccf91d
> pager: exit cleanly on SIGPIPE (BC)
> 

LGTM
Jun Wu - Feb. 10, 2017, 12:24 p.m.
I forgot to mention this looks good to me too (as I saw dsp replied first).

It's an improved version of "dispatch: treat SIGPIPE as a termination signal
(BC)".

Excerpts from Simon Farnsworth's message of 2017-02-08 07:47:39 -0800:
> # HG changeset patch
> # User Simon Farnsworth <simonfar@fb.com>
> # Date 1486568650 28800
> #      Wed Feb 08 07:44:10 2017 -0800
> # Node ID d50cda2a403786836d1f0d5c99401599dc4f43ec
> # Parent  1f51b4658f21bbb797e922d155c1046eddccf91d
> pager: exit cleanly on SIGPIPE (BC)
> 
> Changeset aaa751585325 removes SIGPIPE handling completely. This is wrong,
> as it means that Mercurial does not exit when the pager does. Instead, raise
> SignalInterrupt when SIGPIPE happens with a pager attached, to trigger the
> normal exit path.
> 
> This will cause "killed!" to be printed to stderr (hence the BC warning),
> but in the normal pager use case (where the pager gets both stderr and
> stdout), this messsage is lost as we only get SIGPIPE when the pager quits.
> 
> diff --git a/hgext/pager.py b/hgext/pager.py
> --- a/hgext/pager.py
> +++ b/hgext/pager.py
> @@ -72,6 +72,7 @@
>      commands,
>      dispatch,
>      encoding,
> +    error,
>      extensions,
>      util,
>      )
> @@ -114,6 +115,9 @@
>          os.dup2(stderrfd, util.stderr.fileno())
>          pager.wait()
>  
> +def catchterm(*args):
> +    raise error.SignalInterrupt
> +
>  def uisetup(ui):
>      class pagerui(ui.__class__):
>          def _runpager(self, pagercmd):
> @@ -153,6 +157,8 @@
>          if usepager:
>              ui.setconfig('ui', 'formatted', ui.formatted(), 'pager')
>              ui.setconfig('ui', 'interactive', False, 'pager')
> +            if util.safehasattr(signal, "SIGPIPE"):
> +                signal.signal(signal.SIGPIPE, catchterm)
>              ui._runpager(p)
>          return orig(ui, options, cmd, cmdfunc)
>
Yuya Nishihara - Feb. 11, 2017, 1:44 p.m.
On Wed, 8 Feb 2017 07:47:39 -0800, Simon Farnsworth wrote:
> # HG changeset patch
> # User Simon Farnsworth <simonfar@fb.com>
> # Date 1486568650 28800
> #      Wed Feb 08 07:44:10 2017 -0800
> # Node ID d50cda2a403786836d1f0d5c99401599dc4f43ec
> # Parent  1f51b4658f21bbb797e922d155c1046eddccf91d
> pager: exit cleanly on SIGPIPE (BC)

Queued per reviews, thanks.

> Changeset aaa751585325 removes SIGPIPE handling completely. This is wrong,
> as it means that Mercurial does not exit when the pager does. Instead, raise
> SignalInterrupt when SIGPIPE happens with a pager attached, to trigger the
> normal exit path.

Your comment on V1 "it breaks `hg serve`" reminded me of the weirdness of
SIGPIPE. If we did send() in paged hg process for example, the process could
be killed by SIGPIPE. With this patch, we still have a similar problem, but
the process should exit cleanly.

http://stackoverflow.com/questions/108183/

Patch

diff --git a/hgext/pager.py b/hgext/pager.py
--- a/hgext/pager.py
+++ b/hgext/pager.py
@@ -72,6 +72,7 @@ 
     commands,
     dispatch,
     encoding,
+    error,
     extensions,
     util,
     )
@@ -114,6 +115,9 @@ 
         os.dup2(stderrfd, util.stderr.fileno())
         pager.wait()
 
+def catchterm(*args):
+    raise error.SignalInterrupt
+
 def uisetup(ui):
     class pagerui(ui.__class__):
         def _runpager(self, pagercmd):
@@ -153,6 +157,8 @@ 
         if usepager:
             ui.setconfig('ui', 'formatted', ui.formatted(), 'pager')
             ui.setconfig('ui', 'interactive', False, 'pager')
+            if util.safehasattr(signal, "SIGPIPE"):
+                signal.signal(signal.SIGPIPE, catchterm)
             ui._runpager(p)
         return orig(ui, options, cmd, cmdfunc)