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.

Sign up for GitHub

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/2 half-closed connections gracefully. #777

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Handle HTTP/2 half-closed connections gracefully. #777

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6801e17
Add failing tests
karpetrosyan Aug 9, 2023
2f35282
Suppress StreamClosedError
karpetrosyan Aug 9, 2023
515c4b1
changelog
karpetrosyan Aug 9, 2023
a34b60d
Fix test on python 311
karpetrosyan Aug 9, 2023
642d883
Merge branch 'master' into handle-http-413-cases-for-h2
karpetrosyan Sep 23, 2023
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 CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).

## Unreleased

- Handle HTTP/2 half-closed connections gracefully. (#777)
- Change the type of `Extensions` from `Mapping[Str, Any]` to `MutableMapping[Str, Any]`. (#762)
- Handle HTTP/1.1 half-closed connections gracefully. (#641)
- Drop Python 3.7 support. (#727)
Expand Down
14 changes: 10 additions & 4 deletions httpcore/_async/http2.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,10 +138,16 @@ async def handle_async_request(self, request: Request) -> Response:

try:
kwargs = {"request": request, "stream_id": stream_id}
async with Trace("send_request_headers", logger, request, kwargs):
await self._send_request_headers(request=request, stream_id=stream_id)
async with Trace("send_request_body", logger, request, kwargs):
await self._send_request_body(request=request, stream_id=stream_id)
try:
async with Trace("send_request_headers", logger, request, kwargs):
await self._send_request_headers(
request=request, stream_id=stream_id
)
async with Trace("send_request_body", logger, request, kwargs):
await self._send_request_body(request=request, stream_id=stream_id)
except h2.exceptions.StreamClosedError:
pass

async with Trace(
"receive_response_headers", logger, request, kwargs
) as trace:
Expand Down
14 changes: 10 additions & 4 deletions httpcore/_sync/http2.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,10 +138,16 @@ def handle_request(self, request: Request) -> Response:

try:
kwargs = {"request": request, "stream_id": stream_id}
with Trace("send_request_headers", logger, request, kwargs):
self._send_request_headers(request=request, stream_id=stream_id)
with Trace("send_request_body", logger, request, kwargs):
self._send_request_body(request=request, stream_id=stream_id)
try:
with Trace("send_request_headers", logger, request, kwargs):
self._send_request_headers(
request=request, stream_id=stream_id
)
with Trace("send_request_body", logger, request, kwargs):
self._send_request_body(request=request, stream_id=stream_id)
except h2.exceptions.StreamClosedError:
pass

with Trace(
"receive_response_headers", logger, request, kwargs
) as trace:
Expand Down
88 changes: 88 additions & 0 deletions tests/_async/test_connection.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,56 @@ async def connect_tcp(
assert response.content == b"Request body exceeded 1,000,000 bytes"


@pytest.mark.filterwarnings("ignore::pytest.PytestUnraisableExceptionWarning")
@pytest.mark.anyio
async def test_write_error_with_response_sent_http2():
"""
If a server half-closes the connection while the client is sending
the request, it may still send a response. In this case the client
should successfully read and return the response.

See also the `test_write_error_without_response_sent_http2` test above.
"""

origin = Origin(b"https", b"example.com", 443)
network_backend = AsyncMockBackend(
[
hyperframe.frame.SettingsFrame().serialize(),
hyperframe.frame.WindowUpdateFrame(
stream_id=0, window_increment=2147418112
).serialize()
+ hyperframe.frame.WindowUpdateFrame(
stream_id=1, window_increment=2147418112
).serialize()
+ hyperframe.frame.HeadersFrame(
stream_id=1,
data=hpack.Encoder().encode(
[
(b":status", b"413"),
(b"content-type", b"plain/text"),
]
),
flags=["END_HEADERS"],
).serialize()
+ hyperframe.frame.DataFrame(
stream_id=1,
data=b"Request body exceeded 1,000,000 bytes",
flags=["END_STREAM"],
).serialize()
+ hyperframe.frame.RstStreamFrame(stream_id=1, error_code=0).serialize(),
Comment on lines +159 to +180
Copy link
Member

Choose a reason for hiding this comment

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

Interesting. Is it intentional/neccessary-for-the-test that the data for these frames is concatenated?

Copy link
Member Author

Choose a reason for hiding this comment

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

I ran into some issues without frame concatenation.

Copy link
Member Author

@karpetrosyan karpetrosyan Sep 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Without cancatenation, the client has enough time to send the entire request and receives 413 with no stream errors.

],
http2=True,
)

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
@pytest.mark.filterwarnings("ignore::pytest.PytestUnraisableExceptionWarning")
async def test_write_error_without_response_sent():
Expand Down Expand Up @@ -184,9 +234,47 @@ async def connect_tcp(
content = b"x" * 10_000_000
with pytest.raises(RemoteProtocolError) as exc_info:
await conn.request("POST", "https://example.com/", content=content)

assert str(exc_info.value) == "Server disconnected without sending a response."


@pytest.mark.anyio
@pytest.mark.filterwarnings("ignore::pytest.PytestUnraisableExceptionWarning")
async def test_write_error_without_response_sent_http2():
"""
If a server fully closes the connection while the client is sending
the request, then client should raise an error.

See also the `test_write_error_with_response_sent_http2` test above.
"""

origin = Origin(b"https", b"example.com", 443)
network_backend = AsyncMockBackend(
[
hyperframe.frame.SettingsFrame().serialize(),
hyperframe.frame.WindowUpdateFrame(
stream_id=0, window_increment=2147418112
).serialize()
+ hyperframe.frame.WindowUpdateFrame(
stream_id=1, window_increment=2147418112
).serialize()
+ hyperframe.frame.RstStreamFrame(stream_id=1, error_code=0).serialize(),
],
http2=True,
)

async with AsyncHTTPConnection(
origin=origin, network_backend=network_backend, keepalive_expiry=5.0
) as conn:
content = b"x" * 10_000_000
with pytest.raises(RemoteProtocolError) as exc_info:
await conn.request("POST", "https://example.com/", content=content)
assert str(exc_info.value) in [
"<StreamReset stream_id:1, error_code:ErrorCodes.NO_ERROR, remote_reset:True>",
"<StreamReset stream_id:1, error_code:0, remote_reset:True>",
]


@pytest.mark.anyio
@pytest.mark.filterwarnings("ignore::pytest.PytestUnraisableExceptionWarning")
async def test_http2_connection():
Expand Down
88 changes: 88 additions & 0 deletions tests/_sync/test_connection.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,56 @@ def connect_tcp(
assert response.content == b"Request body exceeded 1,000,000 bytes"


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

def test_write_error_with_response_sent_http2():
"""
If a server half-closes the connection while the client is sending
the request, it may still send a response. In this case the client
should successfully read and return the response.

See also the `test_write_error_without_response_sent_http2` test above.
"""

origin = Origin(b"https", b"example.com", 443)
network_backend = MockBackend(
[
hyperframe.frame.SettingsFrame().serialize(),
hyperframe.frame.WindowUpdateFrame(
stream_id=0, window_increment=2147418112
).serialize()
+ hyperframe.frame.WindowUpdateFrame(
stream_id=1, window_increment=2147418112
).serialize()
+ hyperframe.frame.HeadersFrame(
stream_id=1,
data=hpack.Encoder().encode(
[
(b":status", b"413"),
(b"content-type", b"plain/text"),
]
),
flags=["END_HEADERS"],
).serialize()
+ hyperframe.frame.DataFrame(
stream_id=1,
data=b"Request body exceeded 1,000,000 bytes",
flags=["END_STREAM"],
).serialize()
+ hyperframe.frame.RstStreamFrame(stream_id=1, error_code=0).serialize(),
],
http2=True,
)

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"



@pytest.mark.filterwarnings("ignore::pytest.PytestUnraisableExceptionWarning")
def test_write_error_without_response_sent():
Expand Down Expand Up @@ -184,10 +234,48 @@ def connect_tcp(
content = b"x" * 10_000_000
with pytest.raises(RemoteProtocolError) as exc_info:
conn.request("POST", "https://example.com/", content=content)

assert str(exc_info.value) == "Server disconnected without sending a response."



@pytest.mark.filterwarnings("ignore::pytest.PytestUnraisableExceptionWarning")
def test_write_error_without_response_sent_http2():
"""
If a server fully closes the connection while the client is sending
the request, then client should raise an error.

See also the `test_write_error_with_response_sent_http2` test above.
"""

origin = Origin(b"https", b"example.com", 443)
network_backend = MockBackend(
[
hyperframe.frame.SettingsFrame().serialize(),
hyperframe.frame.WindowUpdateFrame(
stream_id=0, window_increment=2147418112
).serialize()
+ hyperframe.frame.WindowUpdateFrame(
stream_id=1, window_increment=2147418112
).serialize()
+ hyperframe.frame.RstStreamFrame(stream_id=1, error_code=0).serialize(),
],
http2=True,
)

with HTTPConnection(
origin=origin, network_backend=network_backend, keepalive_expiry=5.0
) as conn:
content = b"x" * 10_000_000
with pytest.raises(RemoteProtocolError) as exc_info:
conn.request("POST", "https://example.com/", content=content)
assert str(exc_info.value) in [
"<StreamReset stream_id:1, error_code:ErrorCodes.NO_ERROR, remote_reset:True>",
"<StreamReset stream_id:1, error_code:0, remote_reset:True>",
]



@pytest.mark.filterwarnings("ignore::pytest.PytestUnraisableExceptionWarning")
def test_http2_connection():
origin = Origin(b"https", b"example.com", 443)
Expand Down