From bc05f55059d940c77f791487dcf462674c011440 Mon Sep 17 00:00:00 2001 From: "TAHRI Ahmed R." Date: Thu, 17 Oct 2024 10:09:18 +0200 Subject: [PATCH] :bug: fix handling aggressive ACKs watcher in some QUIC server implementation leading to a ProtocolError (#161) --- CHANGES.rst | 6 ++++++ src/urllib3/_version.py | 2 +- src/urllib3/backend/_async/hface.py | 15 +++++++++++++++ src/urllib3/backend/hface.py | 15 +++++++++++++++ src/urllib3/exceptions.py | 4 ++++ 5 files changed, 41 insertions(+), 1 deletion(-) diff --git a/CHANGES.rst b/CHANGES.rst index b58a7706c6..ecdc0c064d 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -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) ===================== diff --git a/src/urllib3/_version.py b/src/urllib3/_version.py index 9d2b08ebb4..4202d2efea 100644 --- a/src/urllib3/_version.py +++ b/src/urllib3/_version.py @@ -1,4 +1,4 @@ # This file is protected via CODEOWNERS from __future__ import annotations -__version__ = "2.10.905" +__version__ = "2.10.906" diff --git a/src/urllib3/backend/_async/hface.py b/src/urllib3/backend/_async/hface.py index 77eac83985..ee313e5c14 100644 --- a/src/urllib3/backend/_async/hface.py +++ b/src/urllib3/backend/_async/hface.py @@ -43,6 +43,7 @@ IncompleteRead, InvalidHeader, MustDowngradeError, + MustRedialError, ProtocolError, ResponseNotReady, SSLError, @@ -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 diff --git a/src/urllib3/backend/hface.py b/src/urllib3/backend/hface.py index 144a0fd9f8..64d96cb43e 100644 --- a/src/urllib3/backend/hface.py +++ b/src/urllib3/backend/hface.py @@ -47,6 +47,7 @@ IncompleteRead, InvalidHeader, MustDowngradeError, + MustRedialError, ProtocolError, ResponseNotReady, SSLError, @@ -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 diff --git a/src/urllib3/exceptions.py b/src/urllib3/exceptions.py index ecd2c0c502..63b0ffcbd7 100644 --- a/src/urllib3/exceptions.py +++ b/src/urllib3/exceptions.py @@ -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."""