Skip to content

Commit

Permalink
🐛 fix handling aggressive ACKs watcher in some QUIC server implementa…
Browse files Browse the repository at this point in the history
…tion leading to a ProtocolError (#161)
  • Loading branch information
Ousret authored Oct 17, 2024
1 parent 606670e commit bc05f55
Show file tree
Hide file tree
Showing 5 changed files with 41 additions and 1 deletion.
6 changes: 6 additions & 0 deletions CHANGES.rst
Original file line number Diff line number Diff line change
@@ -1,3 +1,9 @@
2.10.906 (2024-10-17)
=====================

- Fixed handling aggressive ACKs watcher in some QUIC server implementation leading to a ``ProtocolError``.
We're actively working toward a solution that will avoid to recycle the QUIC connection.

2.10.905 (2024-10-15)
=====================

Expand Down
2 changes: 1 addition & 1 deletion src/urllib3/_version.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
# This file is protected via CODEOWNERS
from __future__ import annotations

__version__ = "2.10.905"
__version__ = "2.10.906"
15 changes: 15 additions & 0 deletions src/urllib3/backend/_async/hface.py
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@
IncompleteRead,
InvalidHeader,
MustDowngradeError,
MustRedialError,
ProtocolError,
ResponseNotReady,
SSLError,
Expand Down Expand Up @@ -793,6 +794,20 @@ async def __exchange_until(
stream_related_event: bool = hasattr(event, "stream_id")

if not stream_related_event and isinstance(event, ConnectionTerminated):
# some server implementation may be aggressive toward "idle" session
# this is especially true when using QUIC.
# For example, google/quiche expect regular ACKs, otherwise will
# deduct that network conn is dead.
# todo: we will need some kind of background watch constrained by TrafficPolice
# see: https://github.com/google/quiche/commit/c4bb0723f0a03e135bc9328b59a39382761f3de6
# https://github.com/google/quiche/blob/92b45f743288ea2f43ae8cdc4a783ef252e41d93/quiche/quic/core/quic_connection.cc#L6322
if event.error_code == 0:
self._protocol = None
await self.close()
raise MustRedialError(
f"Remote peer just closed our connection, probably for not answering to unsolicited packet. ({event.message})"
)

if (
event.error_code == 400
and event.message
Expand Down
15 changes: 15 additions & 0 deletions src/urllib3/backend/hface.py
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@
IncompleteRead,
InvalidHeader,
MustDowngradeError,
MustRedialError,
ProtocolError,
ResponseNotReady,
SSLError,
Expand Down Expand Up @@ -844,6 +845,20 @@ def __exchange_until(
stream_related_event: bool = hasattr(event, "stream_id")

if not stream_related_event and isinstance(event, ConnectionTerminated):
# some server implementation may be aggressive toward "idle" session
# this is especially true when using QUIC.
# For example, google/quiche expect regular ACKs, otherwise will
# deduct that network conn is dead.
# todo: we will need some kind of background watch constrained by TrafficPolice
# see: https://github.com/google/quiche/commit/c4bb0723f0a03e135bc9328b59a39382761f3de6
# https://github.com/google/quiche/blob/92b45f743288ea2f43ae8cdc4a783ef252e41d93/quiche/quic/core/quic_connection.cc#L6322
if event.error_code == 0:
self._protocol = None
self.close()
raise MustRedialError(
f"Remote peer just closed our connection, probably for not answering to unsolicited packet. ({event.message})"
)

if (
event.error_code == 400
and event.message
Expand Down
4 changes: 4 additions & 0 deletions src/urllib3/exceptions.py
Original file line number Diff line number Diff line change
Expand Up @@ -351,3 +351,7 @@ class RecoverableError(HTTPError):

class MustDowngradeError(RecoverableError):
"""An error occurred with a protocol and can be circumvented using an older protocol."""


class MustRedialError(RecoverableError):
"""The remote peer closed the connection without error, but expected it to be still open."""

0 comments on commit bc05f55

Please sign in to comment.