This is an automated email from the ASF dual-hosted git repository.
yuqi1129 pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/gravitino.git
The following commit(s) were added to refs/heads/main by this push:
new b63f010793 [#13736] fix(python-client): make close idempotent (#13741)
b63f010793 is described below
commit b63f010793bbf8e37a3be40915c89e4cbc7f8dc7
Author: Thunderkeg <[email protected]>
AuthorDate: Sat Oct 10 17:37:52 2026 +0800
[#13736] fix(python-client): make close idempotent (#13741)
### What changes were proposed in this pull request?
- Remove the unsupported HTTP `CLOSE` request from `HTTPClient.close()`.
- Make close local and idempotent after success: call the authentication
data provider without an HTTP request, skip cleanup on repeated
successful calls, and propagate failures so callers can retry.
- Add regression tests for `HTTPClient`, the public `GravitinoClient`,
and `OAuth2TokenProvider` delegation.
### Why are the changes needed?
The bug is that `HTTPClient.close()` sends an unsupported `CLOSE /`
request before closing the authentication provider. The request can fail
when the server does not implement `CLOSE` or is unavailable, preventing
local authentication cleanup.
Fix: #13736
### Does this PR introduce any user-facing change?
Yes. Python client `close()` no longer sends an HTTP request and is safe
to call repeatedly after successful cleanup. The public method signature
is unchanged.
### How was this patch tested?
- From `clients/client-python/tests/unittests`, `PYTHONPATH=../..
coverage run --branch -m unittest` passed 1,185 tests.
- From the same directory, `PYTHONPATH=../.. python -m unittest -v
auth.test_oauth2_token_provider.TestOAuth2TokenProvider.test_close_delegates_without_request`
passed.
- Ruff and Black checks passed on the four changed Python files.
- A local Docker regression against a `2.0.0-SNAPSHOT` server verified
successful `GET /api/version` responses, no `CLOSE` requests, and one
provider cleanup when the server was available and after it stopped.
- Full OAuth service integration is not covered.
---------
Signed-off-by: jiangxt2 <[email protected]>
---
.../client-python/gravitino/utils/http_client.py | 7 ++-
.../unittests/auth/test_oauth2_token_provider.py | 23 +++++++++
.../tests/unittests/test_gravitino_client.py | 17 +++++++
.../tests/unittests/test_http_client_close.py | 58 ++++++++++++++++++++++
4 files changed, 104 insertions(+), 1 deletion(-)
diff --git a/clients/client-python/gravitino/utils/http_client.py
b/clients/client-python/gravitino/utils/http_client.py
index 0aaa0e3940..c7fece2bd5 100644
--- a/clients/client-python/gravitino/utils/http_client.py
+++ b/clients/client-python/gravitino/utils/http_client.py
@@ -97,6 +97,7 @@ class HTTPClient:
self.timeout = gravitino_client_config.get_client_request_timeout()
self.is_debug = is_debug
self.auth_data_provider = auth_data_provider
+ self._closed = False
def _build_url(self, endpoint=None, params=None):
url = self.host
@@ -244,10 +245,14 @@ class HTTPClient:
)
def close(self):
- self._request("close", "/")
+ if self._closed:
+ return
+
if self.auth_data_provider is not None:
self.auth_data_provider.close()
+ self._closed = True
+
def unpack(path: str):
def decorator(func):
diff --git
a/clients/client-python/tests/unittests/auth/test_oauth2_token_provider.py
b/clients/client-python/tests/unittests/auth/test_oauth2_token_provider.py
index d7c3928d05..ddf998c011 100644
--- a/clients/client-python/tests/unittests/auth/test_oauth2_token_provider.py
+++ b/clients/client-python/tests/unittests/auth/test_oauth2_token_provider.py
@@ -20,17 +20,40 @@ from unittest.mock import patch
from gravitino.auth.auth_constants import AuthConstants
from gravitino.auth.default_oauth2_token_provider import
DefaultOAuth2TokenProvider
+from gravitino.auth.oauth2_token_provider import OAuth2TokenProvider
from gravitino.exceptions.base import (
BadRequestException,
IllegalArgumentException,
UnauthorizedException,
)
+from gravitino.utils.http_client import HTTPClient
from tests.unittests.auth import mock_base
OAUTH_PORT = 1082
+class _TestOAuth2TokenProvider(OAuth2TokenProvider):
+ def _get_access_token(self):
+ return None
+
+ def validate(self):
+ pass
+
+
class TestOAuth2TokenProvider(unittest.TestCase):
+ @patch("gravitino.utils.http_client.build_opener")
+ def test_close_delegates_without_request(self, build_opener):
+ provider = _TestOAuth2TokenProvider("http://localhost";)
+
+ with patch.object(
+ HTTPClient, "close", autospec=True, side_effect=HTTPClient.close
+ ) as client_close:
+ provider.close()
+ provider.close()
+
+ self.assertEqual(2, client_close.call_count)
+ build_opener.assert_not_called()
+
def test_provider_init_exception(self):
with self.assertRaises(IllegalArgumentException):
_ = DefaultOAuth2TokenProvider(uri="test")
diff --git a/clients/client-python/tests/unittests/test_gravitino_client.py
b/clients/client-python/tests/unittests/test_gravitino_client.py
index 80ebda8c16..9b3aad87da 100644
--- a/clients/client-python/tests/unittests/test_gravitino_client.py
+++ b/clients/client-python/tests/unittests/test_gravitino_client.py
@@ -15,6 +15,7 @@
# specific language governing permissions and limitations
# under the License.
import unittest
+from unittest.mock import Mock, patch
from gravitino import GravitinoAdminClient, GravitinoClient
from gravitino.client.gravitino_client_config import GravitinoClientConfig
@@ -25,6 +26,22 @@ from tests.unittests import mock_base
@mock_base.mock_data
class TestMetalake(unittest.TestCase):
# pylint: disable=W0212
+ def test_gravitino_client_close_delegates_without_request(self,
*mock_methods):
+ auth_data_provider = Mock()
+ gravitino_client = GravitinoClient(
+ uri="http://localhost:8090";,
+ metalake_name="test",
+ check_version=False,
+ auth_data_provider=auth_data_provider,
+ )
+
+ with patch("gravitino.utils.http_client.build_opener") as build_opener:
+ gravitino_client.close()
+ gravitino_client.close()
+
+ auth_data_provider.close.assert_called_once_with()
+ build_opener.assert_not_called()
+
def test_gravitino_client_headers(self, *mock_methods):
expected_headers = {
"k1": "v1",
diff --git a/clients/client-python/tests/unittests/test_http_client_close.py
b/clients/client-python/tests/unittests/test_http_client_close.py
new file mode 100644
index 0000000000..9f1252409e
--- /dev/null
+++ b/clients/client-python/tests/unittests/test_http_client_close.py
@@ -0,0 +1,58 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+import unittest
+from unittest.mock import Mock, patch
+
+from gravitino.utils.http_client import HTTPClient
+
+
+class TestHTTPClientClose(unittest.TestCase):
+ @patch("gravitino.utils.http_client.build_opener")
+ def test_close_without_auth_provider_is_idempotent(self, build_opener):
+ client = HTTPClient("http://localhost";)
+
+ client.close()
+ client.close()
+
+ build_opener.assert_not_called()
+
+ @patch("gravitino.utils.http_client.build_opener")
+ def test_close_closes_auth_provider_once(self, build_opener):
+ auth_data_provider = Mock()
+ client = HTTPClient("http://localhost";,
auth_data_provider=auth_data_provider)
+
+ client.close()
+ client.close()
+
+ auth_data_provider.close.assert_called_once_with()
+ build_opener.assert_not_called()
+
+ @patch("gravitino.utils.http_client.build_opener")
+ def test_close_retries_after_auth_provider_failure(self, build_opener):
+ auth_data_provider = Mock()
+ auth_data_provider.close.side_effect = [RuntimeError("cleanup
failed"), None]
+ client = HTTPClient("http://localhost";,
auth_data_provider=auth_data_provider)
+
+ with self.assertRaisesRegex(RuntimeError, "cleanup failed"):
+ client.close()
+
+ client.close()
+ client.close()
+
+ self.assertEqual(2, auth_data_provider.close.call_count)
+ build_opener.assert_not_called()
