Patchwork D11087: sigpipe-remote: simply delegate pipe forwarding to subprocess we can kill

login
register
mail settings
Submitter phabricator
Date July 12, 2021, 4:03 a.m.
Message ID <differential-rev-PHID-DREV-ujeckqjpsagkcxx2d2me-req@mercurial-scm.org>
Download mbox | patch
Permalink /patch/49401/
State Superseded
Headers show

Comments

phabricator - July 12, 2021, 4:03 a.m.
marmoute created this revision.
Herald added a reviewer: hg-reviewers.
Herald added a subscriber: mercurial-patches.

REVISION SUMMARY
  Instead of using sophisticated logics with thread a non blocking pipes, we
  simply spawn two new process in charge of reading the pipe and sending the
  result to the client. When it is time to cut the pipe we violently kill them
  without any remorse. This close the pipe regardless of any in progress `os.read`
  call.
  
  Ironically this is the very same things as what the initial shell setup was
  doing, but in Python.
  
  This makes the test pass run properly on Windows. This also reveal that the
  Windows behavior is broken as the transaction is not properly rollback. However
  this is an adventure for another time. Making the test behave properly was
  enough effort.

REPOSITORY
  rHG Mercurial

BRANCH
  default

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

AFFECTED FILES
  tests/test-transaction-rollback-on-sigpipe.t
  tests/testlib/sigpipe-remote.py
  tests/testlib/sigpipe-worker.py

CHANGE DETAILS




To: marmoute, #hg-reviewers
Cc: mercurial-patches, mercurial-devel

Patch

diff --git a/tests/testlib/sigpipe-worker.py b/tests/testlib/sigpipe-worker.py
new file mode 100755
--- /dev/null
+++ b/tests/testlib/sigpipe-worker.py
@@ -0,0 +1,19 @@ 
+#!/usr/bin/env python3
+#
+# This is literally `cat` but in python, one char at a time.
+#
+# see sigpipe-remote.py for details.
+from __future__ import print_function
+
+import io
+import os
+import sys
+
+
+if isinstance(sys.stdout.buffer, io.BufferedWriter):
+    print('SIGPIPE-WORKER: script need unbuffered output', file=sys.stderr)
+    sys.exit(255)
+
+while True:
+    c = os.read(sys.stdin.fileno(), 1)
+    os.write(sys.stdout.fileno(), c)
diff --git a/tests/testlib/sigpipe-remote.py b/tests/testlib/sigpipe-remote.py
--- a/tests/testlib/sigpipe-remote.py
+++ b/tests/testlib/sigpipe-remote.py
@@ -5,7 +5,6 @@ 
 import os
 import subprocess
 import sys
-import threading
 import time
 
 # we cannot use mercurial.testing as long as python2 is not dropped as the test
@@ -71,14 +70,6 @@ 
     return s.decode('latin-1')
 
 
-piped_stdout = os.pipe2(os.O_NONBLOCK | os.O_CLOEXEC)
-piped_stderr = os.pipe2(os.O_NONBLOCK | os.O_CLOEXEC)
-
-stdout_writer = os.fdopen(piped_stdout[1], "rb")
-stdout_reader = os.fdopen(piped_stdout[0], "rb")
-stderr_writer = os.fdopen(piped_stderr[1], "rb")
-stderr_reader = os.fdopen(piped_stderr[0], "rb")
-
 debug_stream.write(b'SIGPIPE-HELPER: Starting\n')
 
 TESTLIB_DIR = os.path.dirname(sys.argv[0])
@@ -91,67 +82,44 @@ 
     SYNCFILE1,
 )
 
-cmd = ['hg']
-cmd += sys.argv[1:]
-sub = subprocess.Popen(
-    cmd,
-    bufsize=0,
-    close_fds=True,
-    stdin=sys.stdin,
-    stdout=stdout_writer,
-    stderr=stderr_writer,
-)
-
-debug_stream.write(b'SIGPIPE-HELPER: Mercurial started\n')
+try:
+    cmd = ['hg']
+    cmd += sys.argv[1:]
+    sub = subprocess.Popen(
+        cmd,
+        bufsize=0,
+        close_fds=True,
+        stdin=sys.stdin,
+        stdout=subprocess.PIPE,
+        stderr=subprocess.PIPE,
+    )
 
-
-shut_down = threading.Event()
-
-close_lock = threading.Lock()
-
+    basedir = os.path.dirname(sys.argv[0])
+    worker = os.path.join(basedir, 'sigpipe-worker.py')
 
-def _read(stream):
-    try:
-        return stream.read()
-    except ValueError:
-        # read on closed file
-        return None
-
+    cmd = [sys.executable, worker]
 
-def forward_stdout():
-    while not shut_down.is_set():
-        c = _read(stdout_reader)
-        while c is not None:
-            sys.stdout.buffer.write(c)
-            c = _read(stdout_reader)
-        time.sleep(0.001)
-    with close_lock:
-        if not stdout_reader.closed:
-            stdout_reader.close()
-            debug_stream.write(b'SIGPIPE-HELPER: stdout closed\n')
-
+    stdout_worker = subprocess.Popen(
+        cmd,
+        bufsize=0,
+        close_fds=True,
+        stdin=sub.stdout,
+        stdout=sys.stdout,
+        stderr=sys.stderr,
+    )
 
-def forward_stderr():
-    while not shut_down.is_set():
-        c = _read(stderr_reader)
-        if c is not None:
-            sys.stderr.buffer.write(c)
-            c = _read(stderr_reader)
-        time.sleep(0.001)
-    with close_lock:
-        if not stderr_reader.closed:
-            stderr_reader.close()
-            debug_stream.write(b'SIGPIPE-HELPER: stderr closed\n')
-
-
-stdout_thread = threading.Thread(target=forward_stdout, daemon=True)
-stderr_thread = threading.Thread(target=forward_stderr, daemon=True)
-
-try:
-    stdout_thread.start()
-    stderr_thread.start()
-
+    stderr_worker = subprocess.Popen(
+        cmd,
+        bufsize=0,
+        close_fds=True,
+        stdin=sub.stderr,
+        stdout=sys.stderr,
+        stderr=sys.stderr,
+    )
     debug_stream.write(b'SIGPIPE-HELPER: Redirection in place\n')
+    os.close(sub.stdout.fileno())
+    os.close(sub.stderr.fileno())
+    debug_stream.write(b'SIGPIPE-HELPER: pipes closed in main\n')
 
     try:
         wait_file(sysbytes(SYNCFILE1))
@@ -160,18 +128,16 @@ 
         debug_stream.write(b'SIGPIPE-HELPER: wait failed: %s\n' % msg)
     else:
         debug_stream.write(b'SIGPIPE-HELPER: SYNCFILE1 detected\n')
-    with close_lock:
-        if not stdout_reader.closed:
-            stdout_reader.close()
-        if not stderr_reader.closed:
-            stderr_reader.close()
-        sys.stdin.close()
-        debug_stream.write(b'SIGPIPE-HELPER: pipes closed\n')
+    stdout_worker.kill()
+    stderr_worker.kill()
+    stdout_worker.wait(10)
+    stderr_worker.wait(10)
+    debug_stream.write(b'SIGPIPE-HELPER: worker killed\n')
+
     debug_stream.write(b'SIGPIPE-HELPER: creating SYNCFILE2\n')
     write_file(sysbytes(SYNCFILE2))
 finally:
     debug_stream.write(b'SIGPIPE-HELPER: Shutting down\n')
-    shut_down.set()
     if not sys.stdin.closed:
         sys.stdin.close()
     try:
@@ -179,6 +145,11 @@ 
     except subprocess.TimeoutExpired:
         msg = b'SIGPIPE-HELPER: Server process failed to terminate\n'
         debug_stream.write(msg)
+        sub.kill()
+        sub.wait()
+        msg = b'SIGPIPE-HELPER: Server process killed\n'
     else:
-        debug_stream.write(b'SIGPIPE-HELPER: Server process terminated\n')
+        msg = b'SIGPIPE-HELPER: Server process terminated with status %d\n'
+        msg %= sub.returncode
+        debug_stream.write(msg)
     debug_stream.write(b'SIGPIPE-HELPER: Shut down\n')
diff --git a/tests/test-transaction-rollback-on-sigpipe.t b/tests/test-transaction-rollback-on-sigpipe.t
--- a/tests/test-transaction-rollback-on-sigpipe.t
+++ b/tests/test-transaction-rollback-on-sigpipe.t
@@ -26,37 +26,55 @@ 
   > [hooks]
   > pretxnchangegroup.00-break-things=sh "$RUNTESTDIR/testlib/wait-on-file" 10 "$SYNCFILE2" "$SYNCFILE1"
   > pretxnchangegroup.01-output-things=echo "some remote output to be forward to the closed pipe"
+  > pretxnchangegroup.02-output-things=echo "some more remote output"
   > EOF
 
   $ hg --cwd ./remote tip -T '{node|short}\n'
   000000000000
   $ cd local
   $ echo foo > foo ; hg commit -qAm "commit"
-  $ hg push -e "\"$PYTHON\" \"$TESTDIR/dummyssh\"" --remotecmd "$remotecmd"
-  pushing to ssh://user@dummy/$TESTTMP/remote
-  searching for changes
-  remote: adding changesets (py3 !)
-  remote: adding manifests (py3 !)
-  remote: adding file changes (py3 !)
-  remote: adding changesets (no-py3 no-chg !)
-  remote: adding manifests (no-py3 no-chg !)
-  remote: adding file changes (no-py3 no-chg !)
+
+(use quiet to avoid flacky output from the server)
+
+  $ hg push --quiet -e "\"$PYTHON\" \"$TESTDIR/dummyssh\"" --remotecmd "$remotecmd"
   abort: stream ended unexpectedly (got 0 bytes, expected 4)
   [255]
   $ cat $SIGPIPE_REMOTE_DEBUG_FILE
   SIGPIPE-HELPER: Starting
-  SIGPIPE-HELPER: Mercurial started
   SIGPIPE-HELPER: Redirection in place
+  SIGPIPE-HELPER: pipes closed in main
   SIGPIPE-HELPER: SYNCFILE1 detected
-  SIGPIPE-HELPER: pipes closed
+  SIGPIPE-HELPER: worker killed
   SIGPIPE-HELPER: creating SYNCFILE2
   SIGPIPE-HELPER: Shutting down
-  SIGPIPE-HELPER: Server process terminated
+  SIGPIPE-HELPER: Server process terminated with status 255 (no-windows !)
+  SIGPIPE-HELPER: Server process terminated with status 1 (windows !)
   SIGPIPE-HELPER: Shut down
 
 The remote should be left in a good state
   $ hg --cwd ../remote tip -T '{node|short}\n'
   000000000000
+
+#if windows
+
+XXX-Windows Broken behavior to be fixed
+
+Behavior on Windows is broken and should be fixed. However this is a fairly
+corner case situation and no data are being corrupted. This would affect
+central repository being hosted on a Windows machine and accessed using ssh.
+
+This was catch as we setup new CI for Windows. Making the test pass on Windows
+was enough of a pain that fixing the behavior set aside for now. Dear and
+honorable reader, feel free to fix it.
+
+  $ hg --cwd ../remote recover
+  rolling back interrupted transaction
+  (verify step skipped, run `hg verify` to check your repository content)
+
+#else
+
   $ hg --cwd ../remote recover
   no interrupted transaction available
   [1]
+
+#endif