Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle HTTP/1.1 half-closed connections gracefully. #641

Merged
merged 13 commits into from
Jul 7, 2023
20 changes: 16 additions & 4 deletions httpcore/_async/http11.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
ConnectionNotAvailable,
LocalProtocolError,
RemoteProtocolError,
WriteError,
map_exceptions,
)
from .._models import Origin, Request, Response
Expand Down Expand Up @@ -84,10 +85,21 @@ async def handle_async_request(self, request: Request) -> Response:

try:
kwargs = {"request": request}
async with Trace("send_request_headers", logger, request, kwargs) as trace:
await self._send_request_headers(**kwargs)
async with Trace("send_request_body", logger, request, kwargs) as trace:
await self._send_request_body(**kwargs)
try:
async with Trace(
"send_request_headers", logger, request, kwargs
) as trace:
await self._send_request_headers(**kwargs)
async with Trace("send_request_body", logger, request, kwargs) as trace:
await self._send_request_body(**kwargs)
except WriteError:
# If we get a write error while we're writing the request,
# then we supress this error and move on to attempting to
# read the response. Servers can sometimes close the request
# pre-emptively and then respond with a well formed HTTP
# error response.
pass

async with Trace(
"receive_response_headers", logger, request, kwargs
) as trace:
Expand Down
20 changes: 16 additions & 4 deletions httpcore/_sync/http11.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
ConnectionNotAvailable,
LocalProtocolError,
RemoteProtocolError,
WriteError,
map_exceptions,
)
from .._models import Origin, Request, Response
Expand Down Expand Up @@ -84,10 +85,21 @@ def handle_request(self, request: Request) -> Response:

try:
kwargs = {"request": request}
with Trace("send_request_headers", logger, request, kwargs) as trace:
self._send_request_headers(**kwargs)
with Trace("send_request_body", logger, request, kwargs) as trace:
self._send_request_body(**kwargs)
try:
with Trace(
"send_request_headers", logger, request, kwargs
) as trace:
self._send_request_headers(**kwargs)
with Trace("send_request_body", logger, request, kwargs) as trace:
self._send_request_body(**kwargs)
except WriteError:
# If we get a write error while we're writing the request,
Copy link
Member

@cdeler cdeler Jul 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if server just closes the request, but don't answer (I saw it in some proxies)? Read will fail, so we will have partially-closed connection?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The connection is closed and the client raises an exception...

httpcore.RemoteProtocolError("Server disconnected without sending a response.")

I've added a test case to demonstrate this.

# then we supress this error and move on to attempting to
# read the response. Servers can sometimes close the request
# pre-emptively and then respond with a well formed HTTP
# error response.
pass

with Trace(
"receive_response_headers", logger, request, kwargs
) as trace:
Expand Down
54 changes: 54 additions & 0 deletions tests/_async/test_connection.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,12 @@
SOCKET_OPTION,
AsyncHTTPConnection,
AsyncMockBackend,
AsyncMockStream,
AsyncNetworkStream,
ConnectError,
ConnectionNotAvailable,
Origin,
WriteError,
)


Expand Down Expand Up @@ -83,6 +85,58 @@ async def test_concurrent_requests_not_available_on_http11_connections():
await conn.request("GET", "https://example.com/")


@pytest.mark.filterwarnings("ignore::pytest.PytestUnraisableExceptionWarning")
@pytest.mark.anyio
async def test_write_error_but_response_sent():
"""
Attempting to issue a request against an already active HTTP/1.1 connection
will raise a `ConnectionNotAvailable` exception.
"""

class ErrorOnRequestTooLargeStream(AsyncMockStream):
def __init__(self, buffer: typing.List[bytes], http2: bool = False) -> None:
super().__init__(buffer, http2)
self.count = 0

async def write(
self, buffer: bytes, timeout: typing.Optional[float] = None
) -> None:
self.count += len(buffer)

if self.count > 1_000_000:
raise WriteError()

class ErrorOnRequestTooLarge(AsyncMockBackend):
async def connect_tcp(
self,
host: str,
port: int,
timeout: typing.Optional[float] = None,
local_address: typing.Optional[str] = None,
socket_options: typing.Optional[typing.Iterable[SOCKET_OPTION]] = None,
) -> AsyncMockStream:
return ErrorOnRequestTooLargeStream(list(self._buffer), http2=self._http2)

origin = Origin(b"https", b"example.com", 443)
network_backend = ErrorOnRequestTooLarge(
[
b"HTTP/1.1 413 Payload Too Large\r\n",
b"Content-Type: plain/text\r\n",
b"Content-Length: 37\r\n",
b"\r\n",
b"Request body exceeded 1,000,000 bytes",
]
)

async with AsyncHTTPConnection(
origin=origin, network_backend=network_backend, keepalive_expiry=5.0
) as conn:
content = b"x" * 10_000_000
response = await conn.request("POST", "https://example.com/", content=content)
assert response.status == 413
assert response.content == b"Request body exceeded 1,000,000 bytes"


@pytest.mark.anyio
async def test_http2_connection():
origin = Origin(b"https", b"example.com", 443)
Expand Down
54 changes: 54 additions & 0 deletions tests/_sync/test_connection.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,12 @@
SOCKET_OPTION,
HTTPConnection,
MockBackend,
MockStream,
NetworkStream,
ConnectError,
ConnectionNotAvailable,
Origin,
WriteError,
)


Expand Down Expand Up @@ -83,6 +85,58 @@ def test_concurrent_requests_not_available_on_http11_connections():
conn.request("GET", "https://example.com/")


@pytest.mark.filterwarnings("ignore::pytest.PytestUnraisableExceptionWarning")

def test_write_error_but_response_sent():
"""
Attempting to issue a request against an already active HTTP/1.1 connection
will raise a `ConnectionNotAvailable` exception.
"""

class ErrorOnRequestTooLargeStream(MockStream):
def __init__(self, buffer: typing.List[bytes], http2: bool = False) -> None:
super().__init__(buffer, http2)
self.count = 0

def write(
self, buffer: bytes, timeout: typing.Optional[float] = None
) -> None:
self.count += len(buffer)

if self.count > 1_000_000:
raise WriteError()

class ErrorOnRequestTooLarge(MockBackend):
def connect_tcp(
self,
host: str,
port: int,
timeout: typing.Optional[float] = None,
local_address: typing.Optional[str] = None,
socket_options: typing.Optional[typing.Iterable[SOCKET_OPTION]] = None,
) -> MockStream:
return ErrorOnRequestTooLargeStream(list(self._buffer), http2=self._http2)

origin = Origin(b"https", b"example.com", 443)
network_backend = ErrorOnRequestTooLarge(
[
b"HTTP/1.1 413 Payload Too Large\r\n",
b"Content-Type: plain/text\r\n",
b"Content-Length: 37\r\n",
b"\r\n",
b"Request body exceeded 1,000,000 bytes",
]
)

with HTTPConnection(
origin=origin, network_backend=network_backend, keepalive_expiry=5.0
) as conn:
content = b"x" * 10_000_000
response = conn.request("POST", "https://example.com/", content=content)
assert response.status == 413
assert response.content == b"Request body exceeded 1,000,000 bytes"



def test_http2_connection():
origin = Origin(b"https", b"example.com", 443)
Expand Down