Repository: encode/httpx

Issue:
URL.join behaviour is incorrect.
Our `URL.join` behavior currently isn't as expected. It should be in line with rfc3986#section-5.2, and Python's existing `urljoin`.

It ain't...

```python
>>> httpx.URL('http://www.example.com/some/directory/abc.html').join('../../foo.html')
URL('http://www.example.com/some/directory/foo.html')
```

Discussion:
tomchristie: Missing a final commit that fixes `.is_absolute` to `.is_absolute_url`.

I'm assuming GitHub's just lagging here, since it appears pushed to me.

Patch:
diff --git a/httpx/__version__.py b/httpx/__version__.py
index c5079f7..b2e30d9 100644
--- a/httpx/__version__.py
+++ b/httpx/__version__.py
@@ -1,3 +1,3 @@
 __title__ = "httpx"
 __description__ = "A next generation HTTP client, for Python 3."
-__version__ = "0.6.7"
+__version__ = "0.6.8"
diff --git a/httpx/concurrency.py b/httpx/concurrency.py
index b8246b5..2ee45a8 100644
--- a/httpx/concurrency.py
+++ b/httpx/concurrency.py
@@ -108,6 +108,9 @@ class Reader(BaseReader):
 
         return data
 
+    def is_connection_dropped(self) -> bool:
+        return self.stream_reader.at_eof()
+
 
 class Writer(BaseWriter):
     def __init__(self, stream_writer: asyncio.StreamWriter, timeout: TimeoutConfig):
diff --git a/httpx/dispatch/connection.py b/httpx/dispatch/connection.py
index b1400af..4a303f2 100644
--- a/httpx/dispatch/connection.py
+++ b/httpx/dispatch/connection.py
@@ -102,3 +102,10 @@ class HTTPConnection(AsyncDispatcher):
         else:
             assert self.h11_connection is not None
             return self.h11_connection.is_closed
+
+    def is_connection_dropped(self) -> bool:
+        if self.h2_connection is not None:
+            return self.h2_connection.is_connection_dropped()
+        else:
+            assert self.h11_connection is not None
+            return self.h11_connection.is_connection_dropped()
diff --git a/httpx/dispatch/connection_pool.py b/httpx/dispatch/connection_pool.py
index 0b827c1..b6c0457 100644
--- a/httpx/dispatch/connection_pool.py
+++ b/httpx/dispatch/connection_pool.py
@@ -9,7 +9,6 @@ from ..config import (
     TimeoutTypes,
     VerifyTypes,
 )
-from ..exceptions import NotConnected
 from ..interfaces import AsyncDispatcher, ConcurrencyBackend
 from ..models import AsyncRequest, AsyncResponse, Origin
 from .connection import HTTPConnection
@@ -108,35 +107,25 @@ class ConnectionPool(AsyncDispatcher):
         cert: CertTypes = None,
         timeout: TimeoutTypes = None,
     ) -> AsyncResponse:
-        allow_connection_reuse = True
-        connection = None
-        while connection is None:
-            connection = await self.acquire_connection(
-                origin=request.url.origin, allow_connection_reuse=allow_connection_reuse
+        connection = await self.acquire_connection(origin=request.url.origin)
+        try:
+            response = await connection.send(
+                request, verify=verify, cert=cert, timeout=timeout
             )
-            try:
-                response = await connection.send(
-                    request, verify=verify, cert=cert, timeout=timeout
-                )
-            except BaseException as exc:
-                self.active_connections.remove(connection)
-                self.max_connections.release()
-                if isinstance(exc, NotConnected) and allow_connection_reuse:
-                    connection = None
-                    allow_connection_reuse = False
-                else:
-                    raise exc
+        except BaseException as exc:
+            self.active_connections.remove(connection)
+            self.max_connections.release()
+            raise exc
 
         return response
 
-    async def acquire_connection(
-        self, origin: Origin, allow_connection_reuse: bool = True
-    ) -> HTTPConnection:
-        connection = None
-        if allow_connection_reuse:
-            connection = self.active_connections.pop_by_origin(origin, http2_only=True)
-            if connection is None:
-                connection = self.keepalive_connections.pop_by_origin(origin)
+    async def acquire_connection(self, origin: Origin) -> HTTPConnection:
+        connection = self.active_connections.pop_by_origin(origin, http2_only=True)
+        if connection is None:
+            connection = self.keepalive_connections.pop_by_origin(origin)
+
+        if connection is not None and connection.is_connection_dropped():
+            connection = None
 
         if connection is None:
             await self.max_connections.acquire()
diff --git a/httpx/dispatch/http11.py b/httpx/dispatch/http11.py
index c239535..dd0e24a 100644
--- a/httpx/dispatch/http11.py
+++ b/httpx/dispatch/http11.py
@@ -4,7 +4,6 @@ import h11
 
 from ..concurrency import TimeoutFlag
 from ..config import TimeoutConfig, TimeoutTypes
-from ..exceptions import NotConnected
 from ..interfaces import BaseReader, BaseWriter, ConcurrencyBackend
 from ..models import AsyncRequest, AsyncResponse
 
@@ -46,12 +45,7 @@ class HTTP11Connection:
     ) -> AsyncResponse:
         timeout = None if timeout is None else TimeoutConfig(timeout)
 
-        try:
-            await self._send_request(request, timeout)
-        except ConnectionResetError:  # pragma: nocover
-            # We're currently testing this case in HTTP/2.
-            # Really we should test it here too, but this'll do in the meantime.
-            raise NotConnected() from None
+        await self._send_request(request, timeout)
 
         task, args = self._send_request_data, [request.stream(), timeout]
         async with self.backend.background_manager(task, args=args):
@@ -188,3 +182,6 @@ class HTTP11Connection:
     @property
     def is_closed(self) -> bool:
         return self.h11_state.our_state in (h11.CLOSED, h11.ERROR)
+
+    def is_connection_dropped(self) -> bool:
+        return self.reader.is_connection_dropped()
diff --git a/httpx/dispatch/http2.py b/httpx/dispatch/http2.py
index 35d487a..331f82d 100644
--- a/httpx/dispatch/http2.py
+++ b/httpx/dispatch/http2.py
@@ -6,7 +6,6 @@ import h2.events
 
 from ..concurrency import TimeoutFlag
 from ..config import TimeoutConfig, TimeoutTypes
-from ..exceptions import NotConnected
 from ..interfaces import BaseReader, BaseWriter, ConcurrencyBackend
 from ..models import AsyncRequest, AsyncResponse
 
@@ -39,10 +38,7 @@ class HTTP2Connection:
         if not self.initialized:
             self.initiate_connection()
 
-        try:
-            stream_id = await self.send_headers(request, timeout)
-        except ConnectionResetError:
-            raise NotConnected() from None
+        stream_id = await self.send_headers(request, timeout)
 
         self.events[stream_id] = []
         self.timeout_flags[stream_id] = TimeoutFlag()
@@ -176,3 +172,6 @@ class HTTP2Connection:
     @property
     def is_closed(self) -> bool:
         return False
+
+    def is_connection_dropped(self) -> bool:
+        return self.reader.is_connection_dropped()
diff --git a/httpx/exceptions.py b/httpx/exceptions.py
index 3305e2d..19af3e6 100644
--- a/httpx/exceptions.py
+++ b/httpx/exceptions.py
@@ -34,13 +34,6 @@ class PoolTimeout(Timeout):
 # HTTP exceptions...
 
 
-class NotConnected(Exception):
-    """
-    A connection was lost at the point of starting a request,
-    prior to any writes succeeding.
-    """
-
-
 class HttpError(Exception):
     """
     An HTTP error occurred.
diff --git a/httpx/interfaces.py b/httpx/interfaces.py
index 02d11ce..f058ede 100644
--- a/httpx/interfaces.py
+++ b/httpx/interfaces.py
@@ -130,6 +130,9 @@ class BaseReader:
     ) -> bytes:
         raise NotImplementedError()  # pragma: no cover
 
+    def is_connection_dropped(self) -> bool:
+        raise NotImplementedError()  # pragma: no cover
+
 
 class BaseWriter:
     """
diff --git a/httpx/models.py b/httpx/models.py
index 9cbdc25..04754af 100644
--- a/httpx/models.py
+++ b/httpx/models.py
@@ -98,7 +98,8 @@ class URL:
                 self.components = self.components.copy_with(authority=idna_authority)
 
         # Normalize scheme and domain name.
-        self.components = self.components.normalize()
+        if self.is_absolute_url:
+            self.components = self.components.normalize()
 
         # Add any query parameters.
         if params:
@@ -170,7 +171,7 @@ class URL:
         Return `True` for absolute URLs such as 'http://example.com/path',
         and `False` for relative URLs such as '/path'.
         """
-        # We don't use rfc3986's `is_absolute` because it treats
+        # We don't use `.is_absolute` from `rfc3986` because it treats
         # URLs with a fragment portion as not absolute.
         # What we actually care about is if the URL provides
         # a scheme and hostname to which connections should be made.


Test patch:
diff --git a/tests/dispatch/test_connection_pools.py b/tests/dispatch/test_connection_pools.py
index 5ef884e..14fd40c 100644
--- a/tests/dispatch/test_connection_pools.py
+++ b/tests/dispatch/test_connection_pools.py
@@ -131,3 +131,39 @@ async def test_premature_response_close(server):
         await response.close()
         assert len(http.active_connections) == 0
         assert len(http.keepalive_connections) == 0
+
+
+@pytest.mark.asyncio
+async def test_keepalive_connection_closed_by_server_is_reestablished(server):
+    """
+    Upon keep-alive connection closed by remote a new connection should be reestablished.
+    """
+    async with httpx.ConnectionPool() as http:
+        response = await http.request("GET", "http://127.0.0.1:8000/")
+        await response.read()
+
+        await server.shutdown()  # shutdown the server to close the keep-alive connection
+        await server.startup()
+
+        response = await http.request("GET", "http://127.0.0.1:8000/")
+        await response.read()
+        assert len(http.active_connections) == 0
+        assert len(http.keepalive_connections) == 1
+
+
+@pytest.mark.asyncio
+async def test_keepalive_http2_connection_closed_by_server_is_reestablished(server):
+    """
+    Upon keep-alive connection closed by remote a new connection should be reestablished.
+    """
+    async with httpx.ConnectionPool() as http:
+        response = await http.request("GET", "http://127.0.0.1:8000/")
+        await response.read()
+
+        await server.shutdown()  # shutdown the server to close the keep-alive connection
+        await server.startup()
+
+        response = await http.request("GET", "http://127.0.0.1:8000/")
+        await response.read()
+        assert len(http.active_connections) == 0
+        assert len(http.keepalive_connections) == 1
diff --git a/tests/dispatch/test_http2.py b/tests/dispatch/test_http2.py
index b194524..99caab6 100644
--- a/tests/dispatch/test_http2.py
+++ b/tests/dispatch/test_http2.py
@@ -68,7 +68,7 @@ def test_http2_reconnect():
 
     with Client(backend=backend) as client:
         response_1 = client.get("http://example.org/1")
-        backend.server.raise_disconnect = True
+        backend.server.close_connection = True
         response_2 = client.get("http://example.org/2")
 
     assert response_1.status_code == 200
diff --git a/tests/dispatch/utils.py b/tests/dispatch/utils.py
index a1e02a4..8a62554 100644
--- a/tests/dispatch/utils.py
+++ b/tests/dispatch/utils.py
@@ -43,7 +43,7 @@ class MockHTTP2Server(BaseReader, BaseWriter):
         self.app = app
         self.buffer = b""
         self.requests = {}
-        self.raise_disconnect = False
+        self.close_connection = False
 
     # BaseReader interface
 
@@ -55,9 +55,6 @@ class MockHTTP2Server(BaseReader, BaseWriter):
     # BaseWriter interface
 
     def write_no_block(self, data: bytes) -> None:
-        if self.raise_disconnect:
-            self.raise_disconnect = False
-            raise ConnectionResetError()
         events = self.conn.receive_data(data)
         self.buffer += self.conn.data_to_send()
         for event in events:
@@ -74,6 +71,9 @@ class MockHTTP2Server(BaseReader, BaseWriter):
     async def close(self) -> None:
         pass
 
+    def is_connection_dropped(self) -> bool:
+        return self.close_connection
+
     # Server implementation
 
     def request_received(self, headers, stream_id):
diff --git a/tests/models/test_url.py b/tests/models/test_url.py
index 70089e0..01489ac 100644
--- a/tests/models/test_url.py
+++ b/tests/models/test_url.py
@@ -1,4 +1,6 @@
 from httpx import URL
+from httpx.exceptions import InvalidURL
+import pytest
 
 
 def test_idna_url():
@@ -31,7 +33,7 @@ def test_url_eq_str():
     assert str(url) == url
 
 
-def test_url__params():
+def test_url_params():
     url = URL("https://example.org:123/path/to/somewhere", params={"a": "123"})
     assert str(url) == "https://example.org:123/path/to/somewhere?a=123"
 
@@ -39,6 +41,75 @@ def test_url__params():
     assert str(url) == "https://example.org:123/path/to/somewhere?a=123"
 
 
+def test_url_join():
+    """
+    Some basic URL joining tests.
+    """
+    url = URL("https://example.org:123/path/to/somewhere")
+    assert url.join('/somewhere-else') == "https://example.org:123/somewhere-else"
+    assert url.join('somewhere-else') == "https://example.org:123/path/to/somewhere-else"
+    assert url.join('../somewhere-else') == "https://example.org:123/path/somewhere-else"
+    assert url.join('../../somewhere-else') == "https://example.org:123/somewhere-else"
+
+
+def test_url_join_rfc3986():
+    """
+    URL joining tests, as-per reference examples in RFC 3986.
+
+    https://tools.ietf.org/html/rfc3986#section-5.4
+    """
+
+    url = URL("http://example.com/b/c/d;p?q")
+
+    with pytest.raises(InvalidURL):
+        assert url.join("g:h") == "g:h"
+
+    assert url.join("g") == "http://example.com/b/c/g"
+    assert url.join("./g") == "http://example.com/b/c/g"
+    assert url.join("g/") == "http://example.com/b/c/g/"
+    assert url.join("/g") == "http://example.com/g"
+    assert url.join("//g") == "http://g"
+    assert url.join("?y") == "http://example.com/b/c/d;p?y"
+    assert url.join("g?y") == "http://example.com/b/c/g?y"
+    assert url.join("#s") == "http://example.com/b/c/d;p?q#s"
+    assert url.join("g#s") == "http://example.com/b/c/g#s"
+    assert url.join("g?y#s") == "http://example.com/b/c/g?y#s"
+    assert url.join(";x") == "http://example.com/b/c/;x"
+    assert url.join("g;x") == "http://example.com/b/c/g;x"
+    assert url.join("g;x?y#s") == "http://example.com/b/c/g;x?y#s"
+    assert url.join("") == "http://example.com/b/c/d;p?q"
+    assert url.join(".") == "http://example.com/b/c/"
+    assert url.join("./") == "http://example.com/b/c/"
+    assert url.join("..") == "http://example.com/b/"
+    assert url.join("../") == "http://example.com/b/"
+    assert url.join("../g") == "http://example.com/b/g"
+    assert url.join("../..") == "http://example.com/"
+    assert url.join("../../") == "http://example.com/"
+    assert url.join("../../g") == "http://example.com/g"
+
+    assert url.join("../../../g") == "http://example.com/g"
+    assert url.join("../../../../g") == "http://example.com/g"
+
+    assert url.join("/./g") == "http://example.com/g"
+    assert url.join("/../g") == "http://example.com/g"
+    assert url.join("g.") == "http://example.com/b/c/g."
+    assert url.join(".g") == "http://example.com/b/c/.g"
+    assert url.join("g..") == "http://example.com/b/c/g.."
+    assert url.join("..g") == "http://example.com/b/c/..g"
+
+    assert url.join("./../g") == "http://example.com/b/g"
+    assert url.join("./g/.") == "http://example.com/b/c/g/"
+    assert url.join("g/./h") == "http://example.com/b/c/g/h"
+    assert url.join("g/../h") == "http://example.com/b/c/h"
+    assert url.join("g;x=1/./y") == "http://example.com/b/c/g;x=1/y"
+    assert url.join("g;x=1/../y") == "http://example.com/b/c/y"
+
+    assert url.join("g?y/./x") == "http://example.com/b/c/g?y/./x"
+    assert url.join("g?y/../x") == "http://example.com/b/c/g?y/../x"
+    assert url.join("g#s/./x") == "http://example.com/b/c/g#s/./x"
+    assert url.join("g#s/../x") == "http://example.com/b/c/g#s/../x"
+
+
 def test_url_set():
     urls = (
         URL("http://example.org:123/path/to/somewhere"),
diff --git a/tests/test_multipart.py b/tests/test_multipart.py
index 9a8f12c..354bcce 100644
--- a/tests/test_multipart.py
+++ b/tests/test_multipart.py
@@ -10,11 +10,11 @@ from httpx import (
     CertTypes,
     Client,
     Dispatcher,
-    multipart,
     Request,
     Response,
     TimeoutTypes,
     VerifyTypes,
+    multipart,
 )