From 8011a4818eb9c70e1481fddb617875d108e54471 Mon Sep 17 00:00:00 2001 From: alvinttang Date: Sat, 25 Apr 2026 19:57:18 +0800 Subject: [PATCH] fix(client/grpc): use SyncGrpcClient._create_channel in sync paths `SyncGrpcClient.from_url` and `SyncGrpcClient.wait_until_server_ready` called `GrpcClient._create_channel`, but `GrpcClient` is a thin wrapper that does not define `_create_channel`. Only `SyncGrpcClient` and `AsyncGrpcClient` do. Every grpc-kind sync client construction crashed with `AttributeError: type object 'GrpcClient' has no attribute '_create_channel'` before any network call was attempted. Fix: route sync code paths through `SyncGrpcClient._create_channel`, which returns a sync `grpc.{insecure,secure}_channel` compatible with the surrounding `with` block. The async paths already use the correct `AsyncGrpcClient._create_channel`. Adds a unit-level regression test covering both call sites and asserting that `SyncGrpcClient._create_channel` returns a sync context manager. Fixes #4683 --- src/bentoml/_internal/client/grpc.py | 14 +++- .../client/test_grpc_sync_create_channel.py | 73 +++++++++++++++++++ 2 files changed, 85 insertions(+), 2 deletions(-) create mode 100644 tests/unit/_internal/client/test_grpc_sync_create_channel.py diff --git a/src/bentoml/_internal/client/grpc.py b/src/bentoml/_internal/client/grpc.py index 75b35072064..d6f9b9fea80 100644 --- a/src/bentoml/_internal/client/grpc.py +++ b/src/bentoml/_internal/client/grpc.py @@ -512,7 +512,11 @@ def wait_until_server_ready( ) -> None: protocol_version = kwargs.get("protocol_version", LATEST_PROTOCOL_VERSION) - with GrpcClient._create_channel( + # Use SyncGrpcClient._create_channel — the wrapper class GrpcClient does + # not define _create_channel, and the AsyncGrpcClient version returns an + # async channel that is incompatible with the sync ``with`` below. + # See bentoml/BentoML#4683. + with SyncGrpcClient._create_channel( f"{host}:{port}", ssl=kwargs.get("ssl", False), ssl_client_credentials=kwargs.get("ssl_client_credentials", None), @@ -725,7 +729,13 @@ def run(): raise BentoMLException("\n".join(exception_message)) pb, _ = import_generated_stubs(protocol_version) - with GrpcClient._create_channel( + # Use ``cls._create_channel`` — the wrapper class ``GrpcClient`` does + # not define ``_create_channel``, and the ``AsyncGrpcClient`` version + # returns an async channel that is incompatible with the sync ``with`` + # below. Using ``cls`` (rather than hardcoding ``SyncGrpcClient``) lets + # subclasses of ``SyncGrpcClient`` override the channel factory. + # See bentoml/BentoML#4683. + with cls._create_channel( server_url.replace(r"localhost", "0.0.0.0"), ssl=kwargs.get("ssl", False), ssl_client_credentials=kwargs.get("ssl_client_credentials", None), diff --git a/tests/unit/_internal/client/test_grpc_sync_create_channel.py b/tests/unit/_internal/client/test_grpc_sync_create_channel.py new file mode 100644 index 00000000000..cdd2af717f3 --- /dev/null +++ b/tests/unit/_internal/client/test_grpc_sync_create_channel.py @@ -0,0 +1,73 @@ +"""Regression test for #4263 / #4683: SyncGrpcClient mistakenly calls +``GrpcClient._create_channel`` (which doesn't exist), instead of its own +``SyncGrpcClient._create_channel``. + +Before the fix, both ``SyncGrpcClient.from_url`` and +``SyncGrpcClient.wait_until_server_ready`` raised +``AttributeError: type object 'GrpcClient' has no attribute '_create_channel'``. +""" + +from __future__ import annotations + +import pytest + +pytest.importorskip("grpc") + + +def test_sync_grpc_client_has_create_channel() -> None: + # The wrapper class ``GrpcClient`` does NOT define ``_create_channel`` — + # only ``SyncGrpcClient`` and ``AsyncGrpcClient`` do. Sync code paths + # MUST resolve ``_create_channel`` via ``SyncGrpcClient``. + from bentoml._internal.client.grpc import GrpcClient + from bentoml._internal.client.grpc import SyncGrpcClient + + assert not hasattr(GrpcClient, "_create_channel"), ( + "GrpcClient is a thin wrapper and intentionally has no " + "_create_channel. Sync paths must use SyncGrpcClient._create_channel." + ) + assert hasattr(SyncGrpcClient, "_create_channel") + + +def test_sync_grpc_client_create_channel_returns_sync_channel() -> None: + # ``SyncGrpcClient._create_channel`` must return a *sync* channel that + # supports ``with ... as channel``. Using the async ``aio.insecure_channel`` + # returns a channel that only supports ``async with``, breaking sync paths. + from bentoml._internal.client.grpc import SyncGrpcClient + + channel = SyncGrpcClient._create_channel("127.0.0.1:65530") + try: + assert hasattr(channel, "__enter__"), ( + "SyncGrpcClient._create_channel must return a sync context manager." + ) + finally: + # ``grpc.insecure_channel`` exposes ``close`` synchronously. + channel.close() + + +def test_sync_grpc_client_wait_until_server_ready_no_attribute_error() -> None: + # Regression for #4683: ``wait_until_server_ready`` was calling + # ``GrpcClient._create_channel`` (which doesn't exist) and crashed with + # AttributeError before it ever attempted a connection. + from bentoml._internal.client.grpc import SyncGrpcClient + + with pytest.raises(Exception) as excinfo: + SyncGrpcClient.wait_until_server_ready("127.0.0.1", 65531, timeout=1) + # Whatever happens, it must NOT be an AttributeError on _create_channel. + assert not ( + isinstance(excinfo.value, AttributeError) + and "_create_channel" in str(excinfo.value) + ), f"Regression: {excinfo.value!r}" + + +def test_sync_grpc_client_from_url_no_attribute_error(monkeypatch) -> None: + # Regression for #4683: ``SyncGrpcClient.from_url`` crashed with + # ``AttributeError: type object 'GrpcClient' has no attribute + # '_create_channel'`` because it called the wrong class's classmethod. + from bentoml._internal.client.grpc import SyncGrpcClient + + with pytest.raises(Exception) as excinfo: + SyncGrpcClient.from_url("127.0.0.1:65532") + assert not ( + isinstance(excinfo.value, AttributeError) + and "_create_channel" in str(excinfo.value) + ), f"Regression: {excinfo.value!r}"