launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Diff comments:

> 
> === modified file 'lib/lp/services/timeout.py'
> --- lib/lp/services/timeout.py	2014-08-29 01:41:14 +0000
> +++ lib/lp/services/timeout.py	2016-03-22 16:38:32 +0000
> @@ -145,47 +158,112 @@
>          return call_with_timeout
>  
>  
> -class CleanableHTTPHandler(urllib2.HTTPHandler):
> -    """Subclass of `urllib2.HTTPHandler` that can be cleaned-up."""
> -
> -    def http_open(self, req):
> -        """See `urllib2.HTTPHandler`."""
> -        def connection_factory(*args, **kwargs):
> -            """Save the created connection so that we can clean it up."""
> -            self.__conn = httplib.HTTPConnection(*args, **kwargs)
> -            return self.__conn
> -        return self.do_open(connection_factory, req)
> -
> -    def reset_connection(self):
> -        """Reset the underlying HTTP connection."""
> -        try:
> -            self.__conn.sock.shutdown(socket.SHUT_RDWR)
> -        except AttributeError:
> -            # It's possible that the other thread closed the socket
> -            # beforehand.
> -            pass
> -        self.__conn.close()
> +class CleanableConnectionPoolMixin:
> +    """Enhance urllib3's connection pools to support forced socket cleanup."""
> +
> +    def __init__(self, *args, **kwargs):
> +        super(CleanableConnectionPoolMixin, self).__init__(*args, **kwargs)
> +        self._all_connections = []
> +        self._all_connections_mutex = Lock()
> +
> +    def _new_conn(self):
> +        self._all_connections_mutex.acquire()
> +        try:
> +            if self._all_connections is None:
> +                raise ClosedPoolError(self, "Pool is closed.")
> +            conn = super(CleanableConnectionPoolMixin, self)._new_conn()
> +            self._all_connections.append(conn)
> +            return conn
> +        finally:
> +            self._all_connections_mutex.release()
> +
> +    def close(self):
> +        self._all_connections_mutex.acquire()
> +        try:
> +            if self._all_connections is None:
> +                return
> +            for conn in self._all_connections:
> +                sock = getattr(conn, "sock", None)
> +                if sock is not None:
> +                    sock.shutdown(socket.SHUT_RDWR)
> +                    sock.close()
> +                    conn.sock = None
> +            self._all_connections = None
> +        finally:
> +            self._all_connections_mutex.release()
> +        super(CleanableConnectionPoolMixin, self).close()

I don't think we can, because the superclass only closes the socket, it doesn't call shutdown on it first, which we were rather deliberately trying to do in the previous implementation and is the whole purpose of all this palaver.  This was the only way I could find to preserve that behaviour.

> +
> +
> +class CleanableHTTPConnectionPool(
> +    CleanableConnectionPoolMixin, HTTPConnectionPool):
> +    pass
> +
> +
> +class CleanableHTTPSConnectionPool(
> +    CleanableConnectionPoolMixin, HTTPSConnectionPool):
> +    pass
> +
> +
> +cleanable_pool_classes_by_scheme = {
> +    "http": CleanableHTTPConnectionPool,
> +    "https": CleanableHTTPSConnectionPool,
> +    }
> +
> +
> +class CleanablePoolManager(PoolManager):
> +    """A version of urllib3's PoolManager supporting forced socket cleanup."""
> +
> +    # XXX cjwatson 2015-03-11: Reimplements PoolManager._new_pool; check
> +    # this when upgrading requests.
> +    def _new_pool(self, scheme, host, port):
> +        if scheme not in cleanable_pool_classes_by_scheme:
> +            raise ValueError("Unhandled scheme: %s" % scheme)
> +        pool_cls = cleanable_pool_classes_by_scheme[scheme]
> +        if scheme == 'http':
> +            kwargs = self.connection_pool_kw.copy()
> +            for kw in ('key_file', 'cert_file', 'cert_reqs', 'ca_certs',
> +                       'ssl_version'):

I won't tell you, then.

> +                kwargs.pop(kw, None)
> +
> +        return pool_cls(host, port, **kwargs)
> +
> +
> +class CleanableHTTPAdapter(HTTPAdapter):
> +    """Enhance HTTPAdapter to use CleanablePoolManager."""
> +
> +    # XXX cjwatson 2015-03-11: Reimplements HTTPAdapter.init_poolmanager;
> +    # check this when upgrading requests.
> +    def init_poolmanager(self, connections, maxsize, block=DEFAULT_POOLBLOCK):
> +        # save these values for pickling
> +        self._pool_connections = connections
> +        self._pool_maxsize = maxsize
> +        self._pool_block = block
> +
> +        self.poolmanager = CleanablePoolManager(
> +            num_pools=connections, maxsize=maxsize, block=block)
>  
>  
>  class URLFetcher:
>      """Object fetching remote URLs with a time out."""
>  
>      @with_timeout(cleanup='cleanup')
> -    def fetch(self, url, data=None):
> +    def fetch(self, url, **request_kwargs):
>          """Fetch the URL using a custom HTTP handler supporting timeout."""
> -        assert url.startswith('http://'), "only http is supported."
> -        self.handler = CleanableHTTPHandler()
> -        opener = urllib2.build_opener(self.handler)
> -        return opener.open(url, data).read()
> +        request_kwargs.setdefault("method", "GET")
> +        self.session = Session()
> +        # Mount our custom adapters.
> +        self.session.mount("https://";, CleanableHTTPAdapter())
> +        self.session.mount("http://";, CleanableHTTPAdapter())
> +        return self.session.request(url=url, **request_kwargs).content
>  
>      def cleanup(self):
>          """Reset the connection when the operation timed out."""
> -        self.handler.reset_connection()
> -
> -
> -def urlfetch(url, data=None):
> -    """Wrapper for `urllib2.urlopen()` that times out."""
> -    return URLFetcher().fetch(url, data)
> +        self.session.close()
> +
> +
> +def urlfetch(url, **request_kwargs):
> +    """Wrapper for `requests.get()` that times out."""
> +    return URLFetcher().fetch(url, **request_kwargs)
>  
>  
>  class TransportWithTimeout(Transport):


-- 
https://code.launchpad.net/~cjwatson/launchpad/timeout-with-requests/+merge/252570
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.