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 New
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

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