changeset 45057:d6e99a446eea

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.

--- a/mercurial/commandserver.py	Mon Jul 06 17:51:18 2020 +0200
+++ b/mercurial/commandserver.py	Sat Jun 27 21:46:23 2020 +0900
@@ -244,8 +244,23 @@
 
         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 @@
         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 @@
         )
 
         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

--- a/mercurial/configitems.py	Mon Jul 06 17:51:18 2020 +0200
+++ b/mercurial/configitems.py	Sat Jun 27 21:46:23 2020 +0900
@@ -212,6 +212,9 @@
     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(

--- a/mercurial/helptext/config.txt	Mon Jul 06 17:51:18 2020 +0200
+++ b/mercurial/helptext/config.txt	Sat Jun 27 21:46:23 2020 +0900
@@ -408,6 +408,18 @@
 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``
 ---------
 

--- a/tests/test-commandserver.t	Mon Jul 06 17:51:18 2020 +0200
+++ b/tests/test-commandserver.t	Sat Jun 27 21:46:23 2020 +0900
@@ -734,6 +734,51 @@
   $ 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