Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/httpx2/httpx2/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
"AsyncByteStream",
"AsyncClient",
"AsyncHTTPTransport",
"AsyncHTTPunkTransport",
"Auth",
"BaseTransport",
"BasicAuth",
Expand Down
2 changes: 2 additions & 0 deletions src/httpx2/httpx2/_transports/__init__.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
from .asgi import ASGITransport
from .base import AsyncBaseTransport, BaseTransport
from .default import AsyncHTTPTransport, HTTPTransport
from .httpunk import AsyncHTTPunkTransport
from .mock import MockTransport
from .wsgi import WSGITransport

Expand All @@ -11,5 +12,6 @@
"AsyncHTTPTransport",
"HTTPTransport",
"MockTransport",
"AsyncHTTPunkTransport",
"WSGITransport",
]
190 changes: 190 additions & 0 deletions src/httpx2/httpx2/_transports/httpunk.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,190 @@
from __future__ import annotations

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Record the commit under the engineer's authorship

The reviewed commit records Codex <codex@openai.com> as its Author rather than the engineer, contrary to the repository requirement that coding-agent work retain engineer-only authorship; recreate the commit with the engineer as its sole author.

AGENTS.md reference: AGENTS.md:L3-L3

Useful? React with 👍 / 👎.


import asyncio
import contextlib
import ssl
import typing
from collections.abc import AsyncIterator
from dataclasses import dataclass, field

from .._exceptions import UnsupportedProtocol
from .._models import Request, Response
from .._types import AsyncByteStream
from .base import AsyncBaseTransport

__all__ = ["AsyncHTTPunkTransport"]


@dataclass
class _HTTPunkConnection:
protocol: typing.Any
connection: typing.Any

async def aclose(self) -> None:
await self.protocol.aclose()


@dataclass
class _HTTPunkOriginPool:
max_connections: int
idle: asyncio.Queue[_HTTPunkConnection] = field(default_factory=asyncio.Queue)
lock: asyncio.Lock = field(default_factory=asyncio.Lock)
count: int = 0
all_connections: list[_HTTPunkConnection] = field(default_factory=list)


class _HTTPunkStream(AsyncByteStream):
def __init__(
self,
response: typing.Any,
transport: AsyncHTTPunkTransport,
origin: tuple[str, str, int],
connection: _HTTPunkConnection,
) -> None:
self._response = response
self._transport = transport
self._origin = origin
self._connection = connection
self._closed = False

async def __aiter__(self) -> AsyncIterator[bytes]:
try:
async for chunk in self._response.aiter_bytes():
yield chunk
finally:
await self.aclose()

async def aclose(self) -> None:
if self._closed:
return
self._closed = True
with contextlib.suppress(Exception):
await self._response.aclose()
await self._transport._release(self._origin, self._connection)


class AsyncHTTPunkTransport(AsyncBaseTransport):
"""An experimental async HTTP/1.1 transport backed by httpunk."""

def __init__(
self,
*,
max_connections: int = 100,
verify: bool | ssl.SSLContext = True,
) -> None:
self._max_connections = max_connections
self._verify = verify
self._pools: dict[tuple[str, str, int], _HTTPunkOriginPool] = {}
self._closed = False

async def handle_async_request(self, request: Request) -> Response:
if self._closed:
raise RuntimeError("Cannot send a request after transport is closed.")

scheme = request.url.scheme
if scheme not in ("http", "https"):
raise UnsupportedProtocol(f"Request URL has an unsupported protocol '{scheme}://'.")

host = request.url.host
port = request.url.port or (443 if scheme == "https" else 80)
origin = (scheme, host, port)
connection = await self._acquire(origin)

target = request.url.raw_path.decode("ascii") or "/"
body = await request.aread()

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Release the connection when reading the request body fails

When an asynchronous upload stream raises or the task is cancelled during request.aread(), the connection has already been acquired but this await occurs outside the cleanup block. That connection remains counted and is never returned to the idle queue, so a small pool can block all subsequent requests. Read the body before acquiring a connection or include this operation in cleanup that releases the slot.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P1: request.aread() is called after acquiring the connection but before the try/except block that calls _discard. If the body read raises (e.g. stream error or cancellation), the acquired connection is never released or discarded, permanently leaking a pool slot. Move aread() before _acquire, or include it inside the try block.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/httpx2/httpx2/_transports/httpunk.py, line 94:

<comment>`request.aread()` is called after acquiring the connection but before the `try/except` block that calls `_discard`. If the body read raises (e.g. stream error or cancellation), the acquired connection is never released or discarded, permanently leaking a pool slot. Move `aread()` before `_acquire`, or include it inside the try block.</comment>

<file context>
@@ -0,0 +1,190 @@
+        connection = await self._acquire(origin)
+
+        target = request.url.raw_path.decode("ascii") or "/"
+        body = await request.aread()
+
+        try:
</file context>


try:
response = await connection.connection.request(
request.method,
target,
headers=request.headers.multi_items(),
body=body or None,
)
except BaseException:
await self._discard(origin, connection)
raise

return Response(
response.status,
headers=list(response.headers.items()),
stream=_HTTPunkStream(response, self, origin, connection),
extensions={"http_version": b"HTTP/1.1"},
)

async def _acquire(self, origin: tuple[str, str, int]) -> _HTTPunkConnection:
pool = self._pools.get(origin)
if pool is None:
pool = self._pools[origin] = _HTTPunkOriginPool(max_connections=self._max_connections)

try:
return pool.idle.get_nowait()
except asyncio.QueueEmpty:
pass

async with pool.lock:
if pool.count < pool.max_connections:
pool.count += 1
connection = await self._connect(origin)
Comment on lines +126 to +127

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Roll back capacity when connection setup fails

With max_connections=1, or once every slot has encountered a transient DNS, TCP, or TLS failure, _connect raises after pool.count is incremented and the count is never restored. The next request then sees the pool at capacity and waits forever on an empty idle queue. Roll back the reserved slot on every setup failure or cancellation.

Useful? React with 👍 / 👎.

pool.all_connections.append(connection)
return connection
Comment on lines +127 to +129

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P1: If _connect raises (e.g. DNS resolution failure, TCP timeout, TLS error), pool.count has already been incremented but is never decremented. With max_connections=1 this permanently exhausts the pool, causing all subsequent requests to hang indefinitely on pool.idle.get(). Wrap the connect call in a try/except that decrements pool.count on failure.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/httpx2/httpx2/_transports/httpunk.py, line 127:

<comment>If `_connect` raises (e.g. DNS resolution failure, TCP timeout, TLS error), `pool.count` has already been incremented but is never decremented. With `max_connections=1` this permanently exhausts the pool, causing all subsequent requests to hang indefinitely on `pool.idle.get()`. Wrap the connect call in a try/except that decrements `pool.count` on failure.</comment>

<file context>
@@ -0,0 +1,190 @@
+        async with pool.lock:
+            if pool.count < pool.max_connections:
+                pool.count += 1
+                connection = await self._connect(origin)
+                pool.all_connections.append(connection)
+                return connection
</file context>
Suggested change
connection = await self._connect(origin)
pool.all_connections.append(connection)
return connection
try:
connection = await self._connect(origin)
except BaseException:
pool.count -= 1
raise
pool.all_connections.append(connection)
return connection


return await pool.idle.get()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2: When the connection pool is exhausted (all max_connections in use), pool.idle.get() blocks indefinitely without any timeout. Unlike the default transport which supports pool timeouts via the "timeout" extension, this transport offers no back-pressure or timeout mechanism, so callers can hang forever waiting for a connection to become available. Consider adding an asyncio.wait_for around the get() call, or reading a timeout from request.extensions.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/httpx2/httpx2/_transports/httpunk.py, line 131:

<comment>When the connection pool is exhausted (all `max_connections` in use), `pool.idle.get()` blocks indefinitely without any timeout. Unlike the default transport which supports pool timeouts via the `"timeout"` extension, this transport offers no back-pressure or timeout mechanism, so callers can hang forever waiting for a connection to become available. Consider adding an `asyncio.wait_for` around the `get()` call, or reading a timeout from `request.extensions`.</comment>

<file context>
@@ -0,0 +1,190 @@
+                pool.all_connections.append(connection)
+                return connection
+
+        return await pool.idle.get()
+
+    async def _connect(self, origin: tuple[str, str, int]) -> _HTTPunkConnection:
</file context>


async def _connect(self, origin: tuple[str, str, int]) -> _HTTPunkConnection:
try:
import httpunk.asyncio
except ImportError as exc: # pragma: no cover
msg = "Using 'AsyncHTTPunkTransport' requires installing the 'httpunk' package."
raise RuntimeError(msg) from exc

scheme, host, port = origin
ssl_context = self._ssl_context(scheme)
authority = f"{host}:{port}"

loop = asyncio.get_running_loop()
_transport, protocol = await loop.create_connection(
lambda: httpunk.asyncio.H1ClientProtocol(authority=authority),
host,
port,
ssl=ssl_context,
Comment on lines +145 to +149

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Map socket setup failures to ConnectError

When DNS resolution, TCP connection, or TLS negotiation fails, loop.create_connection raises raw exceptions such as OSError or ssl.SSLError. Because this path performs no exception translation, callers using the documented except httpx2.RequestError hierarchy will not catch these transport failures. Convert setup failures to httpx2.ConnectError or the appropriate transport exception.

Useful? React with 👍 / 👎.

)
Comment on lines +145 to +150

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P1: loop.create_connection raises raw OSError or ssl.SSLError on DNS/TCP/TLS failures. These are not mapped to httpx2.ConnectError, so callers using except httpx2.RequestError (or except httpx2.ConnectError) won't catch transport setup failures. Wrap this call and translate connection-related exceptions to ConnectError.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/httpx2/httpx2/_transports/httpunk.py, line 145:

<comment>`loop.create_connection` raises raw `OSError` or `ssl.SSLError` on DNS/TCP/TLS failures. These are not mapped to `httpx2.ConnectError`, so callers using `except httpx2.RequestError` (or `except httpx2.ConnectError`) won't catch transport setup failures. Wrap this call and translate connection-related exceptions to `ConnectError`.</comment>

<file context>
@@ -0,0 +1,190 @@
+        authority = f"{host}:{port}"
+
+        loop = asyncio.get_running_loop()
+        _transport, protocol = await loop.create_connection(
+            lambda: httpunk.asyncio.H1ClientProtocol(authority=authority),
+            host,
</file context>
Suggested change
_transport, protocol = await loop.create_connection(
lambda: httpunk.asyncio.H1ClientProtocol(authority=authority),
host,
port,
ssl=ssl_context,
)
try:
_transport, protocol = await loop.create_connection(
lambda: httpunk.asyncio.H1ClientProtocol(authority=authority),
host,
port,
ssl=ssl_context,
)
except (OSError, ssl.SSLError) as exc:
from .._exceptions import ConnectError
msg = f"Failed to connect to {host}:{port}"
raise ConnectError(msg) from exc

connection = await protocol.ready()
return _HTTPunkConnection(protocol=protocol, connection=connection)

def _ssl_context(self, scheme: str) -> ssl.SSLContext | None:
if scheme != "https":
return None
if isinstance(self._verify, ssl.SSLContext):
ssl_context = self._verify
elif self._verify:
ssl_context = ssl.create_default_context()
else:
ssl_context = ssl._create_unverified_context()
ssl_context.set_alpn_protocols(["http/1.1"])
return ssl_context

async def _release(self, origin: tuple[str, str, int], connection: _HTTPunkConnection) -> None:
pool = self._pools.get(origin)
if pool is None or self._closed:
await connection.aclose()
return
pool.idle.put_nowait(connection)

async def _discard(self, origin: tuple[str, str, int], connection: _HTTPunkConnection) -> None:
with contextlib.suppress(Exception):
await connection.aclose()

pool = self._pools.get(origin)
if pool is None:
return
with contextlib.suppress(ValueError):
pool.all_connections.remove(connection)
pool.count -= 1

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Wake queued acquires after discarding a connection

When the pool is saturated and another request is already blocked in pool.idle.get(), an error on the active request calls _discard and only decrements the count. The queued request is never notified that capacity is now available, so it can hang indefinitely even though pool.count < pool.max_connections; use a condition/semaphore or otherwise wake the waiter so it can create a replacement connection.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P1: After decrementing pool.count, tasks blocked on await pool.idle.get() in _acquire are never woken. When the pool is saturated and a connection error triggers _discard, waiters hang indefinitely despite capacity becoming available. A sentinel value should be placed on the queue, or a semaphore/condition should be used to notify waiters that they can create a new connection.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/httpx2/httpx2/_transports/httpunk.py, line 182:

<comment>After decrementing `pool.count`, tasks blocked on `await pool.idle.get()` in `_acquire` are never woken. When the pool is saturated and a connection error triggers `_discard`, waiters hang indefinitely despite capacity becoming available. A sentinel value should be placed on the queue, or a semaphore/condition should be used to notify waiters that they can create a new connection.</comment>

<file context>
@@ -0,0 +1,190 @@
+            return
+        with contextlib.suppress(ValueError):
+            pool.all_connections.remove(connection)
+        pool.count -= 1
+
+    async def aclose(self) -> None:
</file context>


async def aclose(self) -> None:
self._closed = True
connections = [connection for pool in self._pools.values() for connection in pool.all_connections]
self._pools.clear()
for connection in connections:
with contextlib.suppress(Exception):
await connection.aclose()
3 changes: 3 additions & 0 deletions src/httpx2/pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,9 @@ cli = [
http2 = [
"h2>=3,<5",
]
httpunk = [
"httpunk>=0.1.2",
]
socks = [
"socksio==1.*",
]
Expand Down
45 changes: 45 additions & 0 deletions tests/httpx2/test_httpunk_transport.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
from __future__ import annotations

import asyncio

import pytest

import httpx2

pytest.importorskip("httpunk")

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Install httpunk in the CI test environment

Checked .github/workflows/main.yml: each test job runs scripts/install, which executes uv sync --frozen, but the root dev group requests the httpx2 extras without httpunk. Consequently this module-level skip fires on every CI Python version, none of these tests run, and the imported httpunk.py remains largely uncovered before scripts/coverage enforces 100% coverage. Include the extra in the development/CI environment rather than silently skipping the new transport tests.

Useful? React with 👍 / 👎.



def test_async_httpunk_transport(server: object) -> None:
async def run() -> httpx2.Response:
async with httpx2.AsyncClient(transport=httpx2.AsyncHTTPunkTransport()) as client:
return await client.get(str(server.url)) # type: ignore[attr-defined]

response = asyncio.run(run())

assert response.status_code == 200
assert response.text == "Hello, world!"
assert response.http_version == "HTTP/1.1"


def test_async_httpunk_transport_request_body(server: object) -> None:
async def run() -> httpx2.Response:
async with httpx2.AsyncClient(transport=httpx2.AsyncHTTPunkTransport()) as client:
return await client.post(str(server.url.copy_with(path="/echo_body")), content=b"hello") # type: ignore[attr-defined]

response = asyncio.run(run())

assert response.status_code == 200
assert response.content == b"hello"


def test_async_httpunk_transport_connection_reuse(server: object) -> None:
async def run() -> None:
transport = httpx2.AsyncHTTPunkTransport()
async with httpx2.AsyncClient(transport=transport) as client:
response = await client.get(str(server.url)) # type: ignore[attr-defined]
assert response.status_code == 200
response = await client.get(str(server.url)) # type: ignore[attr-defined]
assert response.status_code == 200
assert sum(pool.count for pool in transport._pools.values()) == 1

asyncio.run(run())
Loading
Loading