From 0e66abce464cb35c0bfab35a109911dc052d04b1 Mon Sep 17 00:00:00 2001 From: "Mathias L. Baumann" Date: Wed, 12 Aug 2026 14:04:11 +0200 Subject: [PATCH] Fix HTTP/2 keep-alive channel options `grpc.keepalive_time_ms` and `grpc.keepalive_timeout_ms` were computed with `timedelta.total_seconds() * 1000`, which yields a `float`. grpc-python silently ignores channel arguments whose value is neither `int` nor `str`, so keep-alive was never armed: no pings were sent and a client could only notice a dead stream through its own deadline or a teardown from the peer. Convert both durations to whole milliseconds via a small `_to_millis` helper. The regression test asserts the exact argument type, because value equality does not distinguish `60000` from `60000.0`. Signed-off-by: Mathias L. Baumann --- RELEASE_NOTES.md | 12 ++++++++-- src/frequenz/client/base/channel.py | 32 +++++++++++++++++-------- tests/test_channel.py | 36 +++++++++++++++++++++++++++-- 3 files changed, 66 insertions(+), 14 deletions(-) diff --git a/RELEASE_NOTES.md b/RELEASE_NOTES.md index 4bd60baf..62d4e139 100644 --- a/RELEASE_NOTES.md +++ b/RELEASE_NOTES.md @@ -2,7 +2,7 @@ ## Summary - +Fix HTTP/2 keep-alive, which was never actually enabled. ## Upgrading @@ -14,4 +14,12 @@ ## Bug Fixes - +- HTTP/2 keep-alive is now actually enabled. `grpc.keepalive_time_ms` and + `grpc.keepalive_timeout_ms` were computed with `timedelta.total_seconds() * 1000` + and therefore passed as `float`. gRPC silently ignores channel arguments that are + neither `int` nor `str`, so no keep-alive pings were ever sent and a client could + only detect a dead stream through its own deadline or a teardown sent by the peer. + + Note that keep-alive pings are answered by the nearest HTTP/2 peer. Where a proxy + terminates HTTP/2, successful pings prove that hop is alive, not the backend, so + a stream deadline is still needed to detect an orphaned upstream stream. diff --git a/src/frequenz/client/base/channel.py b/src/frequenz/client/base/channel.py index d00324c5..150b3fe0 100644 --- a/src/frequenz/client/base/channel.py +++ b/src/frequenz/client/base/channel.py @@ -162,23 +162,19 @@ def parse_grpc_uri( ("grpc.keepalive_permit_without_calls", 1), ( "grpc.keepalive_time_ms", - ( - ( - defaults.keep_alive.interval - if options.keep_alive_interval is None - else options.keep_alive_interval - ).total_seconds() - * 1000 + _to_millis( + defaults.keep_alive.interval + if options.keep_alive_interval is None + else options.keep_alive_interval ), ), ( "grpc.keepalive_timeout_ms", - ( + _to_millis( defaults.keep_alive.timeout if options.keep_alive_timeout is None else options.keep_alive_timeout - ).total_seconds() - * 1000, + ), ), ] if keep_alive @@ -212,6 +208,22 @@ def parse_grpc_uri( return insecure_channel(target, channel_options, interceptors=interceptors) +def _to_millis(duration: timedelta) -> int: + """Convert a duration to whole milliseconds. + + gRPC channel arguments must be `int` or `str`. grpc-python silently ignores + arguments of any other type, so passing a `float` disables the option instead + of raising an error. + + Args: + duration: The duration to convert. + + Returns: + The duration in whole milliseconds. + """ + return int(duration.total_seconds() * 1000) + + def _to_bool(value: str) -> bool: value = value.lower() if value in ("true", "on", "1"): diff --git a/tests/test_channel.py b/tests/test_channel.py index 019556d4..7495fbcb 100644 --- a/tests/test_channel.py +++ b/tests/test_channel.py @@ -280,11 +280,11 @@ def test_parse_uri_ok( # pylint: disable=too-many-locals ("grpc.keepalive_permit_without_calls", 1), ( "grpc.keepalive_time_ms", - (expected_keep_alive_interval.total_seconds() * 1000), + int(expected_keep_alive_interval.total_seconds() * 1000), ), ( "grpc.keepalive_timeout_ms", - expected_keep_alive_timeout.total_seconds() * 1000, + int(expected_keep_alive_timeout.total_seconds() * 1000), ), ] if expected_keep_alive.enabled @@ -326,6 +326,38 @@ def test_parse_uri_ok( # pylint: disable=too-many-locals ) +@pytest.mark.parametrize( + "uri", + [ + "grpc://api.example.com:443", + "grpc://api.example.com:443?keep_alive_interval_s=12.5", + "grpc://api.example.com:443?keep_alive_timeout_s=0.75", + ], +) +def test_keep_alive_option_types(uri: str) -> None: + """Test that keep-alive channel arguments are passed as `int`, not `float`. + + gRPC silently ignores channel arguments whose value is neither `int` nor `str`, + so a `float` here disables keep-alive without any error. Comparing values only + would not catch this, because `60000 == 60000.0`. + """ + with mock.patch( + "frequenz.client.base.channel.insecure_channel", + return_value=mock.MagicMock(name="mock_channel", spec=Channel), + ) as insecure_channel_mock: + parse_grpc_uri(uri, defaults=ChannelOptions(ssl=SslOptions(enabled=False))) + + channel_options = insecure_channel_mock.call_args.args[1] + assert channel_options is not None + keep_alive_args = dict(channel_options) + for name in ("grpc.keepalive_time_ms", "grpc.keepalive_timeout_ms"): + value = keep_alive_args[name] + assert type(value) is int, ( # pylint: disable=unidiomatic-typecheck + f"channel argument {name!r} has value {value!r} of type " + f"{type(value).__name__}; gRPC only accepts int or str" + ) + + @pytest.mark.parametrize("value", ["true", "on", "1", "TrUe", "On", "ON", "TRUE"]) def test_to_bool_true(value: str) -> None: """Test conversion of valid boolean values to True."""