http: drop custom http client logic

Eight and a half years ago, as my starter bug on code.google.com, I
investigated a mysterious "broken pipe" error from seemingly random
clients[0]. That investigation revealed a tragic story: the Python
standard library's httplib was (and remains) barely functional. During
large POSTs, if a server responds early with an error (even a
permission denied error!) the client only notices that the server
closed the connection and everything breaks. Such server behavior is
implicitly legal under RFC 2616 (the latest HTTP RFC as of when I was
last working on this), and my understanding is that later RFCs have
made it explicitly legal to respond early with any status code outside
the 2xx range.

I embarked, probably foolishly, on a journey to write a new http
library with better overall behavior. The http library appears to work
well in most cases, but it can get confused in the presence of
proxies, and it depends on select(2) which limits its utility if a lot
of file descriptors are open. I haven't touched the http library in
almost two years, and in the interim the Python community has
discovered a better way[1] of writing network code. In theory some day
urllib3 will have its own home-grown http library built on h11[2], or
we could do that. Either way, it's time to declare our current
confusingly-named "http2" client logic and move on. I do hope to
revisit this some day: it's still garbage that we can't even respond
with a 401 or 403 without reading the entire POST body from the
client, but the goalposts on writing a new http client library have
moved substantially. We're almost certainly better off just switching
to requests and eventually picking up their http fixes than trying to
live with something that realistically only we'll ever use. Another
approach would be to write an adapter so that Mercurial can use pycurl
if it's installed. Neither of those approaches seem like they should
be investigated prior to a release of Mercurial that works on Python
3: that's where the mindshare is going to be for any improvements to
the state of the http client art.

0: http://web.archive.org/web/20130501031801/http://code.google.com/p/support/issues/detail?id=2716
1: http://sans-io.readthedocs.io/
2: https://github.com/njsmith/h11

Differential Revision: https://phab.mercurial-scm.org/D2444

mercurial/configitems.py

@@ -1026,9 +1026,6 b" coreconfigitem('ui', 'formatted',"
 coreconfigitem('ui', 'graphnodetemplate',
     default=None,
 )
-coreconfigitem('ui', 'http2debuglevel',
-    default=None,
-)
 coreconfigitem('ui', 'interactive',
     default=None,
 )
@@ -1127,9 +1124,6 b" coreconfigitem('ui', 'traceback',"
 coreconfigitem('ui', 'tweakdefaults',
     default=False,
 )
-coreconfigitem('ui', 'usehttp2',
-    default=False,
-)
 coreconfigitem('ui', 'username',
     alias=[('ui', 'user')]
 )

mercurial/httpconnection.py

@@ -10,15 +10,10 b''
 
 from __future__ import absolute_import
 
-import logging
 import os
-import socket
 
 from .i18n import _
 from . import (
-    httpclient,
-    sslutil,
-    urllibcompat,
     util,
 )
 
@@ -110,190 +105,3 b' def readauthforuri(ui, uri, user):'
             if user and not bestuser:
                 auth['username'] = user
     return bestauth
-
-# Mercurial (at least until we can remove the old codepath) requires
-# that the http response object be sufficiently file-like, so we
-# provide a close() method here.
-class HTTPResponse(httpclient.HTTPResponse):
-    def close(self):
-        pass
-
-class HTTPConnection(httpclient.HTTPConnection):
-    response_class = HTTPResponse
-    def request(self, method, uri, body=None, headers=None):
-        if headers is None:
-            headers = {}
-        if isinstance(body, httpsendfile):
-            body.seek(0)
-        httpclient.HTTPConnection.request(self, method, uri, body=body,
-                                          headers=headers)
-
-
-_configuredlogging = False
-LOGFMT = '%(levelname)s:%(name)s:%(lineno)d:%(message)s'
-# Subclass BOTH of these because otherwise urllib2 "helpfully"
-# reinserts them since it notices we don't include any subclasses of
-# them.
-class http2handler(urlreq.httphandler, urlreq.httpshandler):
-    def __init__(self, ui, pwmgr):
-        global _configuredlogging
-        urlreq.abstracthttphandler.__init__(self)
-        self.ui = ui
-        self.pwmgr = pwmgr
-        self._connections = {}
-        # developer config: ui.http2debuglevel
-        loglevel = ui.config('ui', 'http2debuglevel')
-        if loglevel and not _configuredlogging:
-            _configuredlogging = True
-            logger = logging.getLogger('mercurial.httpclient')
-            logger.setLevel(getattr(logging, loglevel.upper()))
-            handler = logging.StreamHandler()
-            handler.setFormatter(logging.Formatter(LOGFMT))
-            logger.addHandler(handler)
-
-    def close_all(self):
-        """Close and remove all connection objects being kept for reuse."""
-        for openconns in self._connections.values():
-            for conn in openconns:
-                conn.close()
-        self._connections = {}
-
-    # shamelessly borrowed from urllib2.AbstractHTTPHandler
-    def do_open(self, http_class, req, use_ssl):
-        """Return an addinfourl object for the request, using http_class.
-
-        http_class must implement the HTTPConnection API from httplib.
-        The addinfourl return value is a file-like object.  It also
-        has methods and attributes including:
-            - info(): return a mimetools.Message object for the headers
-            - geturl(): return the original request URL
-            - code: HTTP status code
-        """
-        # If using a proxy, the host returned by get_host() is
-        # actually the proxy. On Python 2.6.1, the real destination
-        # hostname is encoded in the URI in the urllib2 request
-        # object. On Python 2.6.5, it's stored in the _tunnel_host
-        # attribute which has no accessor.
-        tunhost = getattr(req, '_tunnel_host', None)
-        host = urllibcompat.gethost(req)
-        if tunhost:
-            proxyhost = host
-            host = tunhost
-        elif req.has_proxy():
-            proxyhost = urllibcompat.gethost(req)
-            host = urllibcompat.getselector(
-                req).split('://', 1)[1].split('/', 1)[0]
-        else:
-            proxyhost = None
-
-        if proxyhost:
-            if ':' in proxyhost:
-                # Note: this means we'll explode if we try and use an
-                # IPv6 http proxy. This isn't a regression, so we
-                # won't worry about it for now.
-                proxyhost, proxyport = proxyhost.rsplit(':', 1)
-            else:
-                proxyport = 3128 # squid default
-            proxy = (proxyhost, proxyport)
-        else:
-            proxy = None
-
-        if not host:
-            raise urlerr.urlerror('no host given')
-
-        connkey = use_ssl, host, proxy
-        allconns = self._connections.get(connkey, [])
-        conns = [c for c in allconns if not c.busy()]
-        if conns:
-            h = conns[0]
-        else:
-            if allconns:
-                self.ui.debug('all connections for %s busy, making a new '
-                              'one\n' % host)
-            timeout = None
-            if req.timeout is not socket._GLOBAL_DEFAULT_TIMEOUT:
-                timeout = req.timeout
-            h = http_class(host, timeout=timeout, proxy_hostport=proxy)
-            self._connections.setdefault(connkey, []).append(h)
-
-        headers = dict(req.headers)
-        headers.update(req.unredirected_hdrs)
-        headers = dict(
-            (name.title(), val) for name, val in headers.items())
-        try:
-            path = urllibcompat.getselector(req)
-            if '://' in path:
-                path = path.split('://', 1)[1].split('/', 1)[1]
-            if path[0] != '/':
-                path = '/' + path
-            h.request(req.get_method(), path, req.data, headers)
-            r = h.getresponse()
-        except socket.error as err: # XXX what error?
-            raise urlerr.urlerror(err)
-
-        # Pick apart the HTTPResponse object to get the addinfourl
-        # object initialized properly.
-        r.recv = r.read
-
-        resp = urlreq.addinfourl(r, r.headers, urllibcompat.getfullurl(req))
-        resp.code = r.status
-        resp.msg = r.reason
-        return resp
-
-    # httplib always uses the given host/port as the socket connect
-    # target, and then allows full URIs in the request path, which it
-    # then observes and treats as a signal to do proxying instead.
-    def http_open(self, req):
-        if urllibcompat.getfullurl(req).startswith('https'):
-            return self.https_open(req)
-        def makehttpcon(*args, **kwargs):
-            k2 = dict(kwargs)
-            k2[r'use_ssl'] = False
-            return HTTPConnection(*args, **k2)
-        return self.do_open(makehttpcon, req, False)
-
-    def https_open(self, req):
-        # urllibcompat.getfullurl(req) does not contain credentials and we may
-        # need them to match the certificates.
-        url = urllibcompat.getfullurl(req)
-        user, password = self.pwmgr.find_stored_password(url)
-        res = readauthforuri(self.ui, url, user)
-        if res:
-            group, auth = res
-            self.auth = auth
-            self.ui.debug("using auth.%s.* for authentication\n" % group)
-        else:
-            self.auth = None
-        return self.do_open(self._makesslconnection, req, True)
-
-    def _makesslconnection(self, host, port=443, *args, **kwargs):
-        keyfile = None
-        certfile = None
-
-        if args: # key_file
-            keyfile = args.pop(0)
-        if args: # cert_file
-            certfile = args.pop(0)
-
-        # if the user has specified different key/cert files in
-        # hgrc, we prefer these
-        if self.auth and 'key' in self.auth and 'cert' in self.auth:
-            keyfile = self.auth['key']
-            certfile = self.auth['cert']
-
-        # let host port take precedence
-        if ':' in host and '[' not in host or ']:' in host:
-            host, port = host.rsplit(':', 1)
-            port = int(port)
-            if '[' in host:
-                host = host[1:-1]
-
-        kwargs[r'keyfile'] = keyfile
-        kwargs[r'certfile'] = certfile
-
-        con = HTTPConnection(host, port, use_ssl=True,
-                             ssl_wrap_socket=sslutil.wrapsocket,
-                             ssl_validator=sslutil.validatesocket,
-                             ui=self.ui,
-                             **kwargs)
-        return con

mercurial/httppeer.py

@@ -291,9 +291,6 b' class httppeer(wireproto.wirepeer):'
             size = data.length
         elif data is not None:
             size = len(data)
-        if size and self.ui.configbool('ui', 'usehttp2'):
-            headers[r'Expect'] = r'100-Continue'
-            headers[r'X-HgHttp2'] = r'1'
         if data is not None and r'Content-Type' not in headers:
             headers[r'Content-Type'] = r'application/mercurial-0.1'
 

mercurial/url.py

@@ -470,17 +470,9 b' def opener(ui, authinfo=None, useragent='
     construct an opener suitable for urllib2
     authinfo will be added to the password manager
     '''
-    # experimental config: ui.usehttp2
-    if ui.configbool('ui', 'usehttp2'):
-        handlers = [
-            httpconnectionmod.http2handler(
-                ui,
-                passwordmgr(ui, ui.httppasswordmgrdb))
-        ]
-    else:
-        handlers = [httphandler()]
-        if has_https:
-            handlers.append(httpshandler(ui))
+    handlers = [httphandler()]
+    if has_https:
+        handlers.append(httpshandler(ui))
 
     handlers.append(proxyhandler(ui))
 

setup.py

@@ -806,7 +806,6 b" packages = ['mercurial',"
             'mercurial.cext',
             'mercurial.cffi',
             'mercurial.hgweb',
-            'mercurial.httpclient',
             'mercurial.pure',
             'mercurial.thirdparty',
             'mercurial.thirdparty.attr',

tests/test-check-code.t

@@ -13,8 +13,6 b' New errors are not allowed. Warnings are'
   > -X mercurial/thirdparty \
   > | sed 's-\\-/-g' | "$check_code" --warnings --per-file=0 - || false
   Skipping i18n/polib.py it has no-che?k-code (glob)
-  Skipping mercurial/httpclient/__init__.py it has no-che?k-code (glob)
-  Skipping mercurial/httpclient/_readers.py it has no-che?k-code (glob)
   Skipping mercurial/statprof.py it has no-che?k-code (glob)
   Skipping tests/badserverext.py it has no-che?k-code (glob)
 

tests/test-commandserver.t

@@ -211,7 +211,6 b' check that local configs for the cached '
   ui.slash=True
   ui.interactive=False
   ui.mergemarkers=detailed
-  ui.usehttp2=true (?)
   ui.foo=bar
   ui.nontty=true
   web.address=localhost
@@ -221,7 +220,6 b' check that local configs for the cached '
   ui.slash=True
   ui.interactive=False
   ui.mergemarkers=detailed
-  ui.usehttp2=true (?)
   ui.nontty=true
 
   $ rm -R foo