Patchwork D3716: ui: add an unsafeoperation context manager that can block SIGINT

login
register
mail settings
Submitter phabricator
Date June 12, 2018, 3:31 p.m.
Message ID <differential-rev-PHID-DREV-4acd6hejoj3eqsz6iwq4-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/32086/
State Superseded
Headers show

Comments

phabricator - June 12, 2018, 3:31 p.m.
durin42 created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  The blocking of SIGINT is not done by default, but my hope is that we
  will one day. This was inspired by Facebook's "nointerrupt" extension,
  which is a bit more heavy-handed than this (whole commands are treated
  as unsafe to interrupt). A future patch will enable this for varying
  bits of Mercurial that are performing unsafe operations.
  
  .. api::
  
    New context manager ``ui.unsafeoperation()`` to mark portions of a command
    as potentially unsafe places to interrupt Mercurial with Control-C or
    similar.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D3716

AFFECTED FILES
  mercurial/configitems.py
  mercurial/ui.py

CHANGE DETAILS




To: durin42, #hg-reviewers
Cc: mercurial-devel
phabricator - June 12, 2018, 5:08 p.m.
martinvonz added inline comments.

INLINE COMMENTS

> ui.py:347-350
> +        enabled = self.configbool('experimental', 'nointerrupt')
> +        inter = self.interactive() or not self.configbool(
> +            'experimental', 'nointerrupt-interactiveonly')
> +        if not (enabled and inter and self._oldsiginthandler is None):

it took me a while to parse these conditions. perhaps it's clearer like this?

  enabled = self.configbool('experimental', 'nointerrupt')
  if (enabled and
      self.configbool('experimental', 'nointerrupt-interactiveonly')):
      enabled = self.interactive()

> ui.py:348
> +        enabled = self.configbool('experimental', 'nointerrupt')
> +        inter = self.interactive() or not self.configbool(
> +            'experimental', 'nointerrupt-interactiveonly')

nit: does "inter" mean "interrupt" or "interactive" here? since both make sense in this context, it might be better to spell it out

> ui.py:357
> +
> +        def disablesiginthandler(*args):
> +            self.warn(self.config('experimental', 'nointerrupt-message') + '\n')

nit: maybe `disable*d*siginthandler()`? (i would suggest `warningsiginthandler()`, but it's too easy to read the "warning" as noun and not as verb that i meant it to be)

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D3716

To: durin42, #hg-reviewers
Cc: martinvonz, mercurial-devel
phabricator - June 12, 2018, 9:32 p.m.
durin42 added a comment.


  Good suggestions, integrated them. :)

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D3716

To: durin42, #hg-reviewers
Cc: martinvonz, mercurial-devel
phabricator - June 16, 2018, 8:07 p.m.
indygreg requested changes to this revision.
indygreg added a comment.
This revision now requires changes to proceed.


  I agree with @yuja that we should move this to `util.py` or one of its siblings and rename it to `uninterruptable` or some such.
  
  That being said, signal handlers are process global. And we do need to maintain persistent state so things don't get out of whack if we have overlapping calls, potentially from multiple threads (e.g. in the case of hgweb). So maybe `ui.py` - or even a global variable in that module - is the proper place for such state.
  
  Also, I wonder if we shouldn't delay signal handling instead of ignoring it. e.g. our new signal handler would print that the signal was received and then `raise KeyboardInterrupt` at context manager exit time. Otherwise, the handling of `SIGINT` is timing dependent and doesn't consistently result in an aborted process.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D3716

To: durin42, #hg-reviewers, indygreg
Cc: indygreg, yuja, martinvonz, mercurial-devel
Yuya Nishihara - June 17, 2018, 4:51 a.m.
>   I agree with @yuja that we should move this to `util.py` or one of its
> siblings and rename it to `uninterruptable` or some such.

procutil.py would be a better place.

> That being said, signal handlers are process global. And we do need to
> maintain persistent state so things don't get out of whack if we have
> overlapping calls, potentially from multiple threads (e.g. in the case
> of hgweb). So maybe `ui.py` - or even a global variable in that module -
> is the proper place for such state.

I think `ui`-level blocking flag is fine. We won't have to care about
overlapped requests from threads because `signal.signal()` just doesn't
work in sub threads.

What I had in mind was something like the following:

```
class ui:
    @contextmanager
    def uninterruptable(self):
        if already blocked or not enabled:
            yield
            return
        with procutil.uninterruptable(warn=self.warn, onlysigint=True,
                                      allowdoublesigint=True):
            set blocked
            yield
            clear blocked
```
phabricator - June 17, 2018, 4:53 a.m.
yuja added a comment.


  >   I agree with @yuja that we should move this to `util.py` or one of its
  > 
  > siblings and rename it to `uninterruptable` or some such.
  
  procutil.py would be a better place.
  
  > That being said, signal handlers are process global. And we do need to
  >  maintain persistent state so things don't get out of whack if we have
  >  overlapping calls, potentially from multiple threads (e.g. in the case
  >  of hgweb). So maybe `ui.py` - or even a global variable in that module -
  >  is the proper place for such state.
  
  I think `ui`-level blocking flag is fine. We won't have to care about
  overlapped requests from threads because `signal.signal()` just doesn't
  work in sub threads.
  
  What I had in mind was something like the following:
  
    class ui:
        @contextmanager
        def uninterruptable(self):
            if already blocked or not enabled:
                yield
                return
            with procutil.uninterruptable(warn=self.warn, onlysigint=True,
                                          allowdoublesigint=True):
                set blocked
                yield
                clear blocked

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D3716

To: durin42, #hg-reviewers, indygreg
Cc: indygreg, yuja, martinvonz, mercurial-devel
phabricator - June 19, 2018, 6:37 a.m.
spectral added a comment.


  In https://phab.mercurial-scm.org/D3716#58934, @indygreg wrote:
  
  > I agree with @yuja that we should move this to `util.py` or one of its siblings and rename it to `uninterruptable` or some such.
  >
  > That being said, signal handlers are process global. And we do need to maintain persistent state so things don't get out of whack if we have overlapping calls, potentially from multiple threads (e.g. in the case of hgweb). So maybe `ui.py` - or even a global variable in that module - is the proper place for such state.
  >
  > Also, I wonder if we shouldn't delay signal handling instead of ignoring it. e.g. our new signal handler would print that the signal was received and then `raise KeyboardInterrupt` at context manager exit time. Otherwise, the handling of `SIGINT` is timing dependent and doesn't consistently result in an aborted process.
  
  
  I like this idea.  I think bazel does something similar, I know blaze does: one ctrl-c says something like "Shutting down...", three will do a rather forceful shutdown (not kill-9 forceful, but it won't wait for it to get to a convenient spot).  Something like that would work well here imho.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D3716

To: durin42, #hg-reviewers, indygreg
Cc: spectral, indygreg, yuja, martinvonz, mercurial-devel
phabricator - June 27, 2018, 3:48 p.m.
durin42 added a comment.


  I'm not in love with uninterruptible as a name (I think unsafeoperation is more explicit) but I don't feel strongly. The rest of the suggestions I liked, so the series is updated and ready for another look.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D3716

To: durin42, #hg-reviewers, indygreg
Cc: spectral, indygreg, yuja, martinvonz, mercurial-devel
phabricator - July 3, 2018, 4:48 p.m.
martinvonz added inline comments.

INLINE COMMENTS

> ui.py:369
> +
> +
>      def formatter(self, topic, opts):

dropping this line (or maybe it was the one above, I'm not sure ;)) in flight

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D3716

To: durin42, #hg-reviewers, indygreg
Cc: spectral, indygreg, yuja, martinvonz, mercurial-devel
phabricator - July 27, 2018, 6:23 p.m.
mharbison72 added a comment.


  Do we need a different strategy for Windows, or just eat the error?  (See the unsupported signal message)
  
  https://buildbot.mercurial-scm.org/builders/Win7%20x86_64%20hg%20tests/builds/808/steps/run-tests.py%20%28python%202.7.13%29/logs/stdio

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D3716

To: durin42, #hg-reviewers, indygreg
Cc: mharbison72, spectral, indygreg, yuja, martinvonz, mercurial-devel
phabricator - July 28, 2018, 6:27 a.m.
durin42 added a comment.


  >   Do we need a different strategy for Windows, or just eat the error?  (See the unsupported signal message)
  >    
  >   https://buildbot.mercurial-scm.org/builders/Win7%20x86_64%20hg%20tests/builds/808/steps/run-tests.py%20%28python%202.7.13%29/logs/stdio
  
  I'm not sure. We could probably just not support this on windows for now, or at least disable the test on Windows?

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D3716

To: durin42, #hg-reviewers, indygreg
Cc: mharbison72, spectral, indygreg, yuja, martinvonz, mercurial-devel
phabricator - July 28, 2018, 7:05 p.m.
mharbison72 added a comment.


  In https://phab.mercurial-scm.org/D3716#61963, @durin42 wrote:
  
  > >   Do we need a different strategy for Windows, or just eat the error?  (See the unsupported signal message)
  > >    
  > >   https://buildbot.mercurial-scm.org/builders/Win7%20x86_64%20hg%20tests/builds/808/steps/run-tests.py%20%28python%202.7.13%29/logs/stdio
  >
  > I'm not sure. We could probably just not support this on windows for now, or at least disable the test on Windows?
  
  
  I’m ok with disabling the test as long as it doesn’t explode on Windows without turning the experimental knobs.  It sounded like future would would depend on this, and I wasn’t sure if that landed yet.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D3716

To: durin42, #hg-reviewers, indygreg
Cc: mharbison72, spectral, indygreg, yuja, martinvonz, mercurial-devel
Augie Fackler - July 29, 2018, 1:50 a.m.
>>>  Do we need a different strategy for Windows, or just eat the error?  (See the unsupported signal message)
>>> 
>>>  https://buildbot.mercurial-scm.org/builders/Win7%20x86_64%20hg%20tests/builds/808/steps/run-tests.py%20%28python%202.7.13%29/logs/stdio
>> 
>> I'm not sure. We could probably just not support this on windows for now, or at least disable the test on Windows?
> 
> 
>  I’m ok with disabling the test as long as it doesn’t explode on Windows without turning the experimental knobs.  It sounded like future would would depend on this, and I wasn’t sure if that landed yet.

We’ve already made some use of it in core - anything that calls strip will hit this context manager, as will the (experimental) narrow extension.
Yuya Nishihara - July 29, 2018, 2:09 a.m.
> >  I’m ok with disabling the test as long as it doesn’t explode on Windows without turning the experimental knobs.  It sounded like future would would depend on this, and I wasn’t sure if that landed yet.
> 
> We’ve already made some use of it in core - anything that calls strip will hit this context manager, as will the (experimental) narrow extension.

AFAIK, it's test-only failure, which uses subprocess.send_signal(). There's
no sane way to generate Ctrl+C signal programatically on Windows, but SIGINT
handler should work at least as bad as it was before.

Patch

diff --git a/mercurial/ui.py b/mercurial/ui.py
--- a/mercurial/ui.py
+++ b/mercurial/ui.py
@@ -224,6 +224,7 @@ 
         self._colormode = None
         self._terminfoparams = {}
         self._styles = {}
+        self._oldsiginthandler = None
 
         if src:
             self.fout = src.fout
@@ -334,6 +335,39 @@ 
             self._blockedtimes[key + '_blocked'] += \
                 (util.timer() - starttime) * 1000
 
+    @contextlib.contextmanager
+    def unsafeoperation(self):
+        """Mark an operation as unsafe.
+
+        Most operations on a repository are safe to interrupt, but a
+        few are risky (for example repair.strip). This context manager
+        lets you advise Mercurial that something risky is happening so
+        that control-C etc can be blocked if desired.
+        """
+        enabled = self.configbool('experimental', 'nointerrupt')
+        inter = self.interactive() or not self.configbool(
+            'experimental', 'nointerrupt-interactiveonly')
+        if not (enabled and inter and self._oldsiginthandler is None):
+            # if nointerrupt support is turned off, the process isn't
+            # interactive, or we're already in an unsafeoperation
+            # block, do nothing.
+            yield
+            return
+
+        def disablesiginthandler(*args):
+            self.warn(self.config('experimental', 'nointerrupt-message') + '\n')
+            signal.signal(signal.SIGINT, self._oldsiginthandler)
+            self._oldsiginthandler = None
+
+        try:
+            self._oldsiginthandler = signal.getsignal(signal.SIGINT)
+            signal.signal(signal.SIGINT, disablesiginthandler)
+            yield
+        finally:
+            if self._oldsiginthandler is not None:
+                signal.signal(signal.SIGINT, self._oldsiginthandler)
+            self._oldsiginthandler = None
+
     def formatter(self, topic, opts):
         return formatter.formatter(self, self, topic, opts)
 
diff --git a/mercurial/configitems.py b/mercurial/configitems.py
--- a/mercurial/configitems.py
+++ b/mercurial/configitems.py
@@ -560,6 +560,17 @@ 
 coreconfigitem('experimental', 'mergedriver',
     default=None,
 )
+coreconfigitem('experimental', 'nointerrupt', default=False)
+_nointmsg = """
+==========================
+Interrupting Mercurial may leave your repo in a bad state.
+If you really want to interrupt your current command, press
+CTRL-C again.
+==========================
+""".strip()
+coreconfigitem('experimental', 'nointerrupt-message', default=_nointmsg)
+coreconfigitem('experimental', 'nointerrupt-interactiveonly', default=True)
+
 coreconfigitem('experimental', 'obsmarkers-exchange-debug',
     default=False,
 )