[PATCH 1 of 2] cmdserver: add option to not exit from message loop on SIGINT

classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH 1 of 2] cmdserver: add option to not exit from message loop on SIGINT

Yuya Nishihara
# HG changeset patch
# User Yuya Nishihara <[hidden email]>
# Date 1593261983 -32400
#      Sat Jun 27 21:46:23 2020 +0900
# Node ID 4b402b5a3c2ec00bc4e730b46817fcbf937c8d84
# Parent  d2227d4c9e6b97b84b5c0c24b00f6ac3330be3fa
cmdserver: add option to not exit from message loop on SIGINT

Sending SIGINT to server is the only way to interrupt a command running in
command-server process. SIGINT will be caught at dispatch.dispatch() if
we're lucky. Otherwise it will terminate the serer process. This is
fundamentally unreliable as signals are delivered asynchronously.

"cmdserver.shutdown-on-interrupt=False" mitigate the issue by making the
server basically block SIGINT.

diff --git a/mercurial/commandserver.py b/mercurial/commandserver.py
--- a/mercurial/commandserver.py
+++ b/mercurial/commandserver.py
@@ -244,8 +244,23 @@ class server(object):
 
         self.client = fin
 
+        # If shutdown-on-interrupt is off, the default SIGINT handler is
+        # removed so that client-server communication wouldn't be interrupted.
+        # For example, 'runcommand' handler will issue three short read()s.
+        # If one of the first two read()s were interrupted, the communication
+        # channel would be left at dirty state and the subsequent request
+        # wouldn't be parsed. So catching KeyboardInterrupt isn't enough.
+        self._shutdown_on_interrupt = ui.configbool(
+            b'cmdserver', b'shutdown-on-interrupt'
+        )
+        self._old_inthandler = None
+        if not self._shutdown_on_interrupt:
+            self._old_inthandler = signal.signal(signal.SIGINT, signal.SIG_IGN)
+
     def cleanup(self):
         """release and restore resources taken during server session"""
+        if not self._shutdown_on_interrupt:
+            signal.signal(signal.SIGINT, self._old_inthandler)
 
     def _read(self, size):
         if not size:
@@ -278,6 +293,32 @@ class server(object):
         else:
             return []
 
+    def _dispatchcommand(self, req):
+        from . import dispatch  # avoid cycle
+
+        if self._shutdown_on_interrupt:
+            # no need to restore SIGINT handler as it is unmodified.
+            return dispatch.dispatch(req)
+
+        try:
+            signal.signal(signal.SIGINT, self._old_inthandler)
+            return dispatch.dispatch(req)
+        except error.SignalInterrupt:
+            # propagate SIGBREAK, SIGHUP, or SIGTERM.
+            raise
+        except KeyboardInterrupt:
+            # SIGINT may be received out of the try-except block of dispatch(),
+            # so catch it as last ditch. Another KeyboardInterrupt may be
+            # raised while handling exceptions here, but there's no way to
+            # avoid that except for doing everything in C.
+            pass
+        finally:
+            signal.signal(signal.SIGINT, signal.SIG_IGN)
+        # On KeyboardInterrupt, print error message and exit *after* SIGINT
+        # handler removed.
+        req.ui.error(_(b'interrupted!\n'))
+        return -1
+
     def runcommand(self):
         """ reads a list of \0 terminated arguments, executes
         and writes the return code to the result channel """
@@ -318,7 +359,10 @@ class server(object):
         )
 
         try:
-            ret = dispatch.dispatch(req) & 255
+            ret = self._dispatchcommand(req) & 255
+            # If shutdown-on-interrupt is off, it's important to write the
+            # result code *after* SIGINT handler removed. If the result code
+            # were lost, the client wouldn't be able to continue processing.
             self.cresult.write(struct.pack(b'>i', int(ret)))
         finally:
             # restore old cwd
diff --git a/mercurial/configitems.py b/mercurial/configitems.py
--- a/mercurial/configitems.py
+++ b/mercurial/configitems.py
@@ -212,6 +212,9 @@ coreconfigitem(
     default=lambda: [b'chgserver', b'cmdserver', b'repocache'],
 )
 coreconfigitem(
+    b'cmdserver', b'shutdown-on-interrupt', default=True,
+)
+coreconfigitem(
     b'color', b'.*', default=None, generic=True,
 )
 coreconfigitem(
diff --git a/mercurial/helptext/config.txt b/mercurial/helptext/config.txt
--- a/mercurial/helptext/config.txt
+++ b/mercurial/helptext/config.txt
@@ -408,6 +408,18 @@ Supported arguments:
 If no suitable authentication entry is found, the user is prompted
 for credentials as usual if required by the remote.
 
+``cmdserver``
+-------------
+
+Controls command server settings. (ADVANCED)
+
+``shutdown-on-interrupt``
+    If set to false, the server's main loop will continue running after
+    SIGINT received. ``runcommand`` requests can still be interrupted by
+    SIGINT. Close the write end of the pipe to shut down the server
+    process gracefully.
+    (default: True)
+
 ``color``
 ---------
 
diff --git a/tests/test-commandserver.t b/tests/test-commandserver.t
--- a/tests/test-commandserver.t
+++ b/tests/test-commandserver.t
@@ -734,6 +734,51 @@ don't fall back to cwd if invalid -R pat
   $ cd ..
 
 
+#if no-windows
+
+option to not shutdown on SIGINT:
+
+  $ cat <<'EOF' > dbgint.py
+  > import os
+  > import signal
+  > import time
+  > from mercurial import commands, registrar
+  > cmdtable = {}
+  > command = registrar.command(cmdtable)
+  > @command(b"debugsleep", norepo=True)
+  > def debugsleep(ui):
+  >     time.sleep(1)
+  > @command(b"debugsuicide", norepo=True)
+  > def debugsuicide(ui):
+  >     os.kill(os.getpid(), signal.SIGINT)
+  >     time.sleep(1)
+  > EOF
+
+  >>> import signal
+  >>> import time
+  >>> from hgclient import checkwith, readchannel, runcommand
+  >>> @checkwith(extraargs=[b'--config', b'cmdserver.shutdown-on-interrupt=False',
+  ...                       b'--config', b'extensions.dbgint=dbgint.py'])
+  ... def nointr(server):
+  ...     readchannel(server)
+  ...     server.send_signal(signal.SIGINT)  # server won't be terminated
+  ...     time.sleep(1)
+  ...     runcommand(server, [b'debugsleep'])
+  ...     server.send_signal(signal.SIGINT)  # server won't be terminated
+  ...     runcommand(server, [b'debugsleep'])
+  ...     runcommand(server, [b'debugsuicide'])  # command can be interrupted
+  ...     server.send_signal(signal.SIGTERM)  # server will be terminated
+  ...     time.sleep(1)
+  *** runcommand debugsleep
+  *** runcommand debugsleep
+  *** runcommand debugsuicide
+  interrupted!
+  killed!
+   [255]
+
+#endif
+
+
 structured message channel:
 
   $ cat <<'EOF' >> repo2/.hg/hgrc
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

[PATCH 2 of 2] cmdserver: document message-encodings and channel output options

Yuya Nishihara
# HG changeset patch
# User Yuya Nishihara <[hidden email]>
# Date 1593333152 -32400
#      Sun Jun 28 17:32:32 2020 +0900
# Node ID e2d5e74938e320a2c9579489527603efc4f300c7
# Parent  4b402b5a3c2ec00bc4e730b46817fcbf937c8d84
cmdserver: document message-encodings and channel output options

While writing the previous patch, I noticed these options are undocumented.
In my testing, a separate status/error message channel works well in GUI
frontend as we no longer have to sort out data and message from mixed outputs.
So let's mark it as not experimental.

diff --git a/mercurial/commandserver.py b/mercurial/commandserver.py
--- a/mercurial/commandserver.py
+++ b/mercurial/commandserver.py
@@ -191,7 +191,6 @@ class channeledinput(object):
 
 
 def _selectmessageencoder(ui):
-    # experimental config: cmdserver.message-encodings
     encnames = ui.configlist(b'cmdserver', b'message-encodings')
     for n in encnames:
         f = _messageencoders.get(n)
@@ -234,9 +233,6 @@ class server(object):
             self.ui = self.ui.copy()
             setuplogging(self.ui, repo=None, fp=self.cdebug)
 
-        # TODO: add this to help/config.txt when stabilized
-        # ``channel``
-        #   Use separate channel for structured output. (Command-server only)
         self.cmsg = None
         if ui.config(b'ui', b'message-output') == b'channel':
             encname, encfn = _selectmessageencoder(ui)
diff --git a/mercurial/configitems.py b/mercurial/configitems.py
--- a/mercurial/configitems.py
+++ b/mercurial/configitems.py
@@ -204,7 +204,7 @@ coreconfigitem(
     b'cmdserver', b'max-repo-cache', default=0, experimental=True,
 )
 coreconfigitem(
-    b'cmdserver', b'message-encodings', default=list, experimental=True,
+    b'cmdserver', b'message-encodings', default=list,
 )
 coreconfigitem(
     b'cmdserver',
diff --git a/mercurial/helptext/config.txt b/mercurial/helptext/config.txt
--- a/mercurial/helptext/config.txt
+++ b/mercurial/helptext/config.txt
@@ -413,6 +413,12 @@ for credentials as usual if required by
 
 Controls command server settings. (ADVANCED)
 
+``message-encodings``
+    List of encodings for the ``m`` (message) channel. The first encoding
+    supported by the server will be selected and advertised in the hello
+    message. This is useful only when ``ui.message-output`` is set to
+    ``channel``. Supported encodings are ``cbor``.
+
 ``shutdown-on-interrupt``
     If set to false, the server's main loop will continue running after
     SIGINT received. ``runcommand`` requests can still be interrupted by
@@ -2383,6 +2389,8 @@ User interface controls.
 ``message-output``
     Where to write status and error messages. (default: ``stdio``)
 
+    ``channel``
+      Use separate channel for structured output. (Command-server only)
     ``stderr``
       Everything to stderr.
     ``stdio``
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 2 of 2] cmdserver: document message-encodings and channel output options

Augie Fackler-2
queued, thanks

> On Jun 28, 2020, at 05:10, Yuya Nishihara <[hidden email]> wrote:
>
> # HG changeset patch
> # User Yuya Nishihara <[hidden email]>
> # Date 1593333152 -32400
> #      Sun Jun 28 17:32:32 2020 +0900
> # Node ID e2d5e74938e320a2c9579489527603efc4f300c7
> # Parent  4b402b5a3c2ec00bc4e730b46817fcbf937c8d84
> cmdserver: document message-encodings and channel output options
>
> While writing the previous patch, I noticed these options are undocumented.
> In my testing, a separate status/error message channel works well in GUI
> frontend as we no longer have to sort out data and message from mixed outputs.
> So let's mark it as not experimental.
>
> diff --git a/mercurial/commandserver.py b/mercurial/commandserver.py
> --- a/mercurial/commandserver.py
> +++ b/mercurial/commandserver.py
> @@ -191,7 +191,6 @@ class channeledinput(object):
>
>
> def _selectmessageencoder(ui):
> -    # experimental config: cmdserver.message-encodings
>     encnames = ui.configlist(b'cmdserver', b'message-encodings')
>     for n in encnames:
>         f = _messageencoders.get(n)
> @@ -234,9 +233,6 @@ class server(object):
>             self.ui = self.ui.copy()
>             setuplogging(self.ui, repo=None, fp=self.cdebug)
>
> -        # TODO: add this to help/config.txt when stabilized
> -        # ``channel``
> -        #   Use separate channel for structured output. (Command-server only)
>         self.cmsg = None
>         if ui.config(b'ui', b'message-output') == b'channel':
>             encname, encfn = _selectmessageencoder(ui)
> diff --git a/mercurial/configitems.py b/mercurial/configitems.py
> --- a/mercurial/configitems.py
> +++ b/mercurial/configitems.py
> @@ -204,7 +204,7 @@ coreconfigitem(
>     b'cmdserver', b'max-repo-cache', default=0, experimental=True,
> )
> coreconfigitem(
> -    b'cmdserver', b'message-encodings', default=list, experimental=True,
> +    b'cmdserver', b'message-encodings', default=list,
> )
> coreconfigitem(
>     b'cmdserver',
> diff --git a/mercurial/helptext/config.txt b/mercurial/helptext/config.txt
> --- a/mercurial/helptext/config.txt
> +++ b/mercurial/helptext/config.txt
> @@ -413,6 +413,12 @@ for credentials as usual if required by
>
> Controls command server settings. (ADVANCED)
>
> +``message-encodings``
> +    List of encodings for the ``m`` (message) channel. The first encoding
> +    supported by the server will be selected and advertised in the hello
> +    message. This is useful only when ``ui.message-output`` is set to
> +    ``channel``. Supported encodings are ``cbor``.
> +
> ``shutdown-on-interrupt``
>     If set to false, the server's main loop will continue running after
>     SIGINT received. ``runcommand`` requests can still be interrupted by
> @@ -2383,6 +2389,8 @@ User interface controls.
> ``message-output``
>     Where to write status and error messages. (default: ``stdio``)
>
> +    ``channel``
> +      Use separate channel for structured output. (Command-server only)
>     ``stderr``
>       Everything to stderr.
>     ``stdio``
> _______________________________________________
> Mercurial-devel mailing list
> [hidden email]
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel