Patchwork [4,of,6] httppeer: wrap HTTPResponse.read() globally

login
register
mail settings
Submitter Gregory Szorc
Date April 14, 2017, 7:44 a.m.
Message ID <b8a66f70caadbe53bf2e.1492155848@ubuntu-vm-main>
Download mbox | patch
Permalink /patch/20197/
State Accepted
Headers show

Comments

Gregory Szorc - April 14, 2017, 7:44 a.m.
# HG changeset patch
# User Gregory Szorc <gregory.szorc@gmail.com>
# Date 1492155236 25200
#      Fri Apr 14 00:33:56 2017 -0700
# Node ID b8a66f70caadbe53bf2ea43b0be1d1d8acba94ad
# Parent  80bd24abcf67d5dc8b5f5bf83c796e1f71fc5bd9
httppeer: wrap HTTPResponse.read() globally

There were a handful of places in the code where HTTPResponse.read()
was called with no explicit error handling or with inconsistent
error handling. In order to eliminate this class of bug, we globally
swap out HTTPResponse.read() with a unified error handler.

I initially attempted to fix all call sites. However, after
going down that rabbit hole, I figured it was best to just change
read() to do what we want. This appears to be a worthwhile
change, as the tests demonstrate many of our uncaught exceptions
go away.

To better represent this class of failure, we introduce a new
error type. The main benefit over IOError is it can hold a hint.
I'm receptive to tweaking its name or inheritance.
Yuya Nishihara - April 16, 2017, 3:34 a.m.
On Fri, 14 Apr 2017 00:44:08 -0700, Gregory Szorc wrote:
> # HG changeset patch
> # User Gregory Szorc <gregory.szorc@gmail.com>
> # Date 1492155236 25200
> #      Fri Apr 14 00:33:56 2017 -0700
> # Node ID b8a66f70caadbe53bf2ea43b0be1d1d8acba94ad
> # Parent  80bd24abcf67d5dc8b5f5bf83c796e1f71fc5bd9
> httppeer: wrap HTTPResponse.read() globally
> 
> There were a handful of places in the code where HTTPResponse.read()
> was called with no explicit error handling or with inconsistent
> error handling. In order to eliminate this class of bug, we globally
> swap out HTTPResponse.read() with a unified error handler.
> 
> I initially attempted to fix all call sites. However, after
> going down that rabbit hole, I figured it was best to just change
> read() to do what we want. This appears to be a worthwhile
> change, as the tests demonstrate many of our uncaught exceptions
> go away.

After reading the whole series, I agree. Queued but for the last one, thanks.

> To better represent this class of failure, we introduce a new
> error type. The main benefit over IOError is it can hold a hint.
> I'm receptive to tweaking its name or inheritance.

How about WireTransportError? RichIOError seems a bit confusing since it
isn't (and shouldn't be) an IOError.
Gregory Szorc - April 16, 2017, 6:07 p.m.
On Sat, Apr 15, 2017 at 8:34 PM, Yuya Nishihara <yuya@tcha.org> wrote:

> On Fri, 14 Apr 2017 00:44:08 -0700, Gregory Szorc wrote:
> > # HG changeset patch
> > # User Gregory Szorc <gregory.szorc@gmail.com>
> > # Date 1492155236 25200
> > #      Fri Apr 14 00:33:56 2017 -0700
> > # Node ID b8a66f70caadbe53bf2ea43b0be1d1d8acba94ad
> > # Parent  80bd24abcf67d5dc8b5f5bf83c796e1f71fc5bd9
> > httppeer: wrap HTTPResponse.read() globally
> >
> > There were a handful of places in the code where HTTPResponse.read()
> > was called with no explicit error handling or with inconsistent
> > error handling. In order to eliminate this class of bug, we globally
> > swap out HTTPResponse.read() with a unified error handler.
> >
> > I initially attempted to fix all call sites. However, after
> > going down that rabbit hole, I figured it was best to just change
> > read() to do what we want. This appears to be a worthwhile
> > change, as the tests demonstrate many of our uncaught exceptions
> > go away.
>
> After reading the whole series, I agree. Queued but for the last one,
> thanks.
>
> > To better represent this class of failure, we introduce a new
> > error type. The main benefit over IOError is it can hold a hint.
> > I'm receptive to tweaking its name or inheritance.
>
> How about WireTransportError? RichIOError seems a bit confusing since it
> isn't (and shouldn't be) an IOError.
>

Yeah, I think renaming it to "WireTransportError" or similar is a good
idea. I'll try to send that patch later today...

Patch

diff --git a/mercurial/error.py b/mercurial/error.py
--- a/mercurial/error.py
+++ b/mercurial/error.py
@@ -246,3 +246,6 @@  class UnsupportedBundleSpecification(Exc
 
 class CorruptedState(Exception):
     """error raised when a command is not able to read its state from file"""
+
+class RichIOError(Abort):
+    """An IOError that can also have a hint attached."""
diff --git a/mercurial/httppeer.py b/mercurial/httppeer.py
--- a/mercurial/httppeer.py
+++ b/mercurial/httppeer.py
@@ -75,6 +75,41 @@  def encodevalueinheaders(value, header, 
 
     return result
 
+def _wraphttpresponse(resp):
+    """Wrap an HTTPResponse with common error handlers.
+
+    This ensures that any I/O from any consumer raises the appropriate
+    error and messaging.
+    """
+    origread = resp.read
+
+    class readerproxy(resp.__class__):
+        def read(self, size=None):
+            try:
+                return origread(size)
+            except httplib.IncompleteRead as e:
+                # e.expected is an integer if length known or None otherwise.
+                if e.expected:
+                    msg = _('HTTP request error (incomplete response; '
+                            'expected %d bytes got %d)') % (e.expected,
+                                                           len(e.partial))
+                else:
+                    msg = _('HTTP request error (incomplete response)')
+
+                raise error.RichIOError(
+                    msg,
+                    hint=_('this may be an intermittent network failure; '
+                           'if the error persists, consider contacting the '
+                           'network or server operator'))
+            except httplib.HTTPException as e:
+                raise error.RichIOError(
+                    _('HTTP request error (%s)') % e,
+                    hint=_('this may be an intermittent failure; '
+                           'if the error persists, consider contacting the '
+                           'network or server operator'))
+
+    resp.__class__ = readerproxy
+
 class httppeer(wireproto.wirepeer):
     def __init__(self, ui, path):
         self.path = path
@@ -223,6 +258,10 @@  class httppeer(wireproto.wirepeer):
             self.ui.debug('http error while sending %s command\n' % cmd)
             self.ui.traceback()
             raise IOError(None, inst)
+
+        # Insert error handlers for common I/O failures.
+        _wraphttpresponse(resp)
+
         # record the url we got redirected to
         resp_url = resp.geturl()
         if resp_url.endswith(qs):
diff --git a/tests/test-http-bad-server.t b/tests/test-http-bad-server.t
--- a/tests/test-http-bad-server.t
+++ b/tests/test-http-bad-server.t
@@ -267,10 +267,10 @@  Server sends an incomplete capabilities 
   $ hg --config badserver.closeaftersendbytes=180 serve -p $HGPORT -d --pid-file=hg.pid -E error.log
   $ cat hg.pid > $DAEMON_PIDS
 
-TODO client spews a stack due to uncaught httplib.IncompleteRead
-
-  $ hg clone http://localhost:$HGPORT/ clone 2> /dev/null
-  [1]
+  $ hg clone http://localhost:$HGPORT/ clone
+  abort: HTTP request error (incomplete response; expected 385 bytes got 20)
+  (this may be an intermittent network failure; if the error persists, consider contacting the network or server operator)
+  [255]
 
   $ killdaemons.py $DAEMON_PIDS
 
@@ -461,11 +461,11 @@  Server sends empty HTTP body for getbund
   $ hg --config badserver.closeaftersendbytes=933 serve -p $HGPORT -d --pid-file=hg.pid -E error.log
   $ cat hg.pid > $DAEMON_PIDS
 
-TODO client spews a stack due to uncaught httplib.IncompleteRead
-
-  $ hg clone http://localhost:$HGPORT/ clone 2> /dev/null
+  $ hg clone http://localhost:$HGPORT/ clone
   requesting all changes
-  [1]
+  abort: HTTP request error (incomplete response)
+  (this may be an intermittent network failure; if the error persists, consider contacting the network or server operator)
+  [255]
 
   $ killdaemons.py $DAEMON_PIDS
 
@@ -525,11 +525,11 @@  Server sends partial compression string
   $ hg --config badserver.closeaftersendbytes=945 serve -p $HGPORT -d --pid-file=hg.pid -E error.log
   $ cat hg.pid > $DAEMON_PIDS
 
-TODO client spews a stack due to uncaught httplib.IncompleteRead
-
-  $ hg clone http://localhost:$HGPORT/ clone 2> /dev/null
+  $ hg clone http://localhost:$HGPORT/ clone
   requesting all changes
-  [1]
+  abort: HTTP request error (incomplete response; expected 1 bytes got 3)
+  (this may be an intermittent network failure; if the error persists, consider contacting the network or server operator)
+  [255]
 
   $ killdaemons.py $DAEMON_PIDS
 
@@ -593,7 +593,8 @@  Server sends partial bundle2 header magi
 
   $ hg clone http://localhost:$HGPORT/ clone
   requesting all changes
-  abort: connection ended unexpectedly
+  abort: HTTP request error (incomplete response; expected 1 bytes got 3)
+  (this may be an intermittent network failure; if the error persists, consider contacting the network or server operator)
   [255]
 
   $ killdaemons.py $DAEMON_PIDS
@@ -616,7 +617,8 @@  Server sends incomplete bundle2 stream p
 
   $ hg clone http://localhost:$HGPORT/ clone
   requesting all changes
-  abort: connection ended unexpectedly
+  abort: HTTP request error (incomplete response; expected 1 bytes got 3)
+  (this may be an intermittent network failure; if the error persists, consider contacting the network or server operator)
   [255]
 
   $ killdaemons.py $DAEMON_PIDS
@@ -640,7 +642,8 @@  Servers stops after bundle2 stream param
 
   $ hg clone http://localhost:$HGPORT/ clone
   requesting all changes
-  abort: connection ended unexpectedly
+  abort: HTTP request error (incomplete response)
+  (this may be an intermittent network failure; if the error persists, consider contacting the network or server operator)
   [255]
 
   $ killdaemons.py $DAEMON_PIDS
@@ -664,7 +667,8 @@  Server stops sending after bundle2 part 
 
   $ hg clone http://localhost:$HGPORT/ clone
   requesting all changes
-  abort: connection ended unexpectedly
+  abort: HTTP request error (incomplete response)
+  (this may be an intermittent network failure; if the error persists, consider contacting the network or server operator)
   [255]
 
   $ killdaemons.py $DAEMON_PIDS
@@ -785,7 +789,8 @@  Server stops sending after 0 length payl
   added 1 changesets with 1 changes to 1 files
   transaction abort!
   rollback completed
-  abort: connection ended unexpectedly
+  abort: HTTP request error (incomplete response)
+  (this may be an intermittent network failure; if the error persists, consider contacting the network or server operator)
   [255]
 
   $ killdaemons.py $DAEMON_PIDS