Patchwork [STABLE] bundle2: fail faster when interrupted

login
register
mail settings
Submitter Gregory Szorc
Date Aug. 26, 2016, 2:54 a.m.
Message ID <fe13f99c4ecebf0f276b.1472180040@ubuntu-vm-main>
Download mbox | patch
Permalink /patch/16443/
State Accepted
Headers show

Comments

Gregory Szorc - Aug. 26, 2016, 2:54 a.m.
# HG changeset patch
# User Gregory Szorc <gregory.szorc@gmail.com>
# Date 1472179994 25200
#      Thu Aug 25 19:53:14 2016 -0700
# Branch stable
# Node ID fe13f99c4ecebf0f276b793f2ec1f4350805cb77
# Parent  5004ef47f437b3bafdd3800551c638a0bf4bb99b
bundle2: fail faster when interrupted

Before this patch, bundle2 application attempted to consume remaining
bundle2 part data when the process is interrupted (SIGINT) or when
sys.exit is called (translated into a SystemExit exception). This
meant that if one of these occurred when applying a say 1 GB
changegroup bundle2 part being downloaded over a network, it may take
Mercurial *several minutes* to terminate after a SIGINT because the
process is waiting on the network to stream megabytes of data. This is
not a great user experience and a regression from bundle1. Furthermore,
many process supervisors tend to only give processes a finite amount of
time to exit after delivering SIGINT: if processes take too long to
self-terminate, a SIGKILL is issued and Mercurial has no opportunity to
clean up. This would mean orphaned locks and transactions. Not good.

This patch changes the bundle2 application behavior to fail faster
when an interrupt or system exit is requested. It does so by not
catching BaseException (which includes KeyboardInterrupt and
SystemExit) and by explicitly checking for these conditions in
yet another handler which would also seek to the end of the current
bundle2 part on failure.

The end result of this patch is that SIGINT is now reacted to
significantly faster: the active transaction is rolled back
immediately without waiting for incoming bundle2 data to be consumed.
This restores the pre-bundle2 behavior and makes Mercurial treat
signals with the urgency they deserve.
Pierre-Yves David - Aug. 26, 2016, 9:58 p.m.
On 08/26/2016 04:54 AM, Gregory Szorc wrote:
> # HG changeset patch
> # User Gregory Szorc <gregory.szorc@gmail.com>
> # Date 1472179994 25200
> #      Thu Aug 25 19:53:14 2016 -0700
> # Branch stable
> # Node ID fe13f99c4ecebf0f276b793f2ec1f4350805cb77
> # Parent  5004ef47f437b3bafdd3800551c638a0bf4bb99b
> bundle2: fail faster when interrupted

That was an oversight…
Nice catch. I've pushed your fix. Thanks a lot.

Patch

diff --git a/mercurial/bundle2.py b/mercurial/bundle2.py
--- a/mercurial/bundle2.py
+++ b/mercurial/bundle2.py
@@ -348,17 +348,17 @@  def processbundle(repo, unbundler, trans
         msg.append('\n')
         repo.ui.debug(''.join(msg))
     iterparts = enumerate(unbundler.iterparts())
     part = None
     nbpart = 0
     try:
         for nbpart, part in iterparts:
             _processpart(op, part)
-    except BaseException as exc:
+    except Exception as exc:
         for nbpart, part in iterparts:
             # consume the bundle content
             part.seek(0, 2)
         # Small hack to let caller code distinguish exceptions from bundle2
         # processing from processing the old format. This is mostly
         # needed to handle different return codes to unbundle according to the
         # type of bundle. We should probably clean up or drop this return code
         # craziness in a future version.
@@ -377,16 +377,17 @@  def processbundle(repo, unbundler, trans
     return op
 
 def _processpart(op, part):
     """process a single part from a bundle
 
     The part is guaranteed to have been fully consumed when the function exits
     (even if an exception is raised)."""
     status = 'unknown' # used by debug output
+    hardabort = False
     try:
         try:
             handler = parthandlermapping.get(part.type)
             if handler is None:
                 status = 'unsupported-type'
                 raise error.BundleUnknownFeatureError(parttype=part.type)
             indebug(op.ui, 'found a handler for part %r' % part.type)
             unknownparams = part.mandatorykeys - handler.params
@@ -431,19 +432,25 @@  def _processpart(op, part):
             handler(op, part)
         finally:
             if output is not None:
                 output = op.ui.popbuffer()
             if output:
                 outpart = op.reply.newpart('output', data=output,
                                            mandatory=False)
                 outpart.addparam('in-reply-to', str(part.id), mandatory=False)
+    # If exiting or interrupted, do not attempt to seek the stream in the
+    # finally block below. This makes abort faster.
+    except (SystemExit, KeyboardInterrupt):
+        hardabort = True
+        raise
     finally:
         # consume the part content to not corrupt the stream.
-        part.seek(0, 2)
+        if not hardabort:
+            part.seek(0, 2)
 
 
 def decodecaps(blob):
     """decode a bundle2 caps bytes blob into a dictionary
 
     The blob is a list of capabilities (one per line)
     Capabilities may have values using a line of the form::