| 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
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= >
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
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) >
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)