From 226aea9169cf26d3ef7c6bd7f23d318fb7069332 Mon Sep 17 00:00:00 2001 From: Stainless Bot <107565488+stainless-bot@users.noreply.github.com> Date: Tue, 12 Dec 2023 14:52:59 +0000 Subject: [PATCH] refactor(client): simplify cleanup (#297) This removes Client.__del__, but users are not expected to call this directly. --- pyproject.toml | 2 +- src/lithic/_base_client.py | 26 ++++++++++++++++++++------ src/lithic/_client.py | 30 ++++-------------------------- tests/test_client.py | 23 ++--------------------- 4 files changed, 27 insertions(+), 54 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index 059ec7cd..ab1d7fa6 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -81,7 +81,7 @@ typecheck = { chain = [ ]} "typecheck:pyright" = "pyright" "typecheck:verify-types" = "pyright --verifytypes lithic --ignoreexternal" -"typecheck:mypy" = "mypy --enable-incomplete-feature=Unpack ." +"typecheck:mypy" = "mypy ." [build-system] requires = ["hatchling"] diff --git a/src/lithic/_base_client.py b/src/lithic/_base_client.py index bbbb8a54..04a20bfd 100644 --- a/src/lithic/_base_client.py +++ b/src/lithic/_base_client.py @@ -5,6 +5,7 @@ import time import uuid import email +import asyncio import inspect import logging import platform @@ -672,9 +673,16 @@ def _idempotency_key(self) -> str: return f"stainless-python-retry-{uuid.uuid4()}" +class SyncHttpxClientWrapper(httpx.Client): + def __del__(self) -> None: + try: + self.close() + except Exception: + pass + + class SyncAPIClient(BaseClient[httpx.Client, Stream[Any]]): _client: httpx.Client - _has_custom_http_client: bool _default_stream_cls: type[Stream[Any]] | None = None def __init__( @@ -747,7 +755,7 @@ def __init__( custom_headers=custom_headers, _strict_response_validation=_strict_response_validation, ) - self._client = http_client or httpx.Client( + self._client = http_client or SyncHttpxClientWrapper( base_url=base_url, # cast to a valid type because mypy doesn't understand our type narrowing timeout=cast(Timeout, timeout), @@ -755,7 +763,6 @@ def __init__( transport=transport, limits=limits, ) - self._has_custom_http_client = bool(http_client) def is_closed(self) -> bool: return self._client.is_closed @@ -1135,9 +1142,17 @@ def get_api_list( return self._request_api_list(model, page, opts) +class AsyncHttpxClientWrapper(httpx.AsyncClient): + def __del__(self) -> None: + try: + # TODO(someday): support non asyncio runtimes here + asyncio.get_running_loop().create_task(self.aclose()) + except Exception: + pass + + class AsyncAPIClient(BaseClient[httpx.AsyncClient, AsyncStream[Any]]): _client: httpx.AsyncClient - _has_custom_http_client: bool _default_stream_cls: type[AsyncStream[Any]] | None = None def __init__( @@ -1210,7 +1225,7 @@ def __init__( custom_headers=custom_headers, _strict_response_validation=_strict_response_validation, ) - self._client = http_client or httpx.AsyncClient( + self._client = http_client or AsyncHttpxClientWrapper( base_url=base_url, # cast to a valid type because mypy doesn't understand our type narrowing timeout=cast(Timeout, timeout), @@ -1218,7 +1233,6 @@ def __init__( transport=transport, limits=limits, ) - self._has_custom_http_client = bool(http_client) def is_closed(self) -> bool: return self._client.is_closed diff --git a/src/lithic/_client.py b/src/lithic/_client.py index dfa0a78d..b3b3ad08 100644 --- a/src/lithic/_client.py +++ b/src/lithic/_client.py @@ -3,7 +3,6 @@ from __future__ import annotations import os -import asyncio from typing import Any, Dict, Union, Mapping, cast from typing_extensions import Self, Literal, override @@ -36,6 +35,8 @@ DEFAULT_MAX_RETRIES, SyncAPIClient, AsyncAPIClient, + SyncHttpxClientWrapper, + AsyncHttpxClientWrapper, make_request_options, ) @@ -265,7 +266,7 @@ def copy( if http_client is not None: raise ValueError("The 'http_client' argument is mutually exclusive with 'connection_pool_limits'") - if self._has_custom_http_client: + if not isinstance(self._client, SyncHttpxClientWrapper): raise ValueError( "A custom HTTP client has been set and is mutually exclusive with the 'connection_pool_limits' argument" ) @@ -297,16 +298,6 @@ def copy( # client.with_options(timeout=10).foo.create(...) with_options = copy - def __del__(self) -> None: - if not hasattr(self, "_has_custom_http_client") or not hasattr(self, "close"): - # this can happen if the '__init__' method raised an error - return - - if self._has_custom_http_client: - return - - self.close() - def api_status( self, *, @@ -567,7 +558,7 @@ def copy( if http_client is not None: raise ValueError("The 'http_client' argument is mutually exclusive with 'connection_pool_limits'") - if self._has_custom_http_client: + if not isinstance(self._client, AsyncHttpxClientWrapper): raise ValueError( "A custom HTTP client has been set and is mutually exclusive with the 'connection_pool_limits' argument" ) @@ -599,19 +590,6 @@ def copy( # client.with_options(timeout=10).foo.create(...) with_options = copy - def __del__(self) -> None: - if not hasattr(self, "_has_custom_http_client") or not hasattr(self, "close"): - # this can happen if the '__init__' method raised an error - return - - if self._has_custom_http_client: - return - - try: - asyncio.get_running_loop().create_task(self.close()) - except Exception: - pass - async def api_status( self, *, diff --git a/tests/test_client.py b/tests/test_client.py index 14edfe05..20e5b1a9 100644 --- a/tests/test_client.py +++ b/tests/test_client.py @@ -720,14 +720,6 @@ def test_proxies_option_mutually_exclusive_with_http_client(self) -> None: http_client=http_client, ) - def test_client_del(self) -> None: - client = Lithic(base_url=base_url, api_key=api_key, _strict_response_validation=True) - assert not client.is_closed() - - client.__del__() - - assert client.is_closed() - def test_copied_client_does_not_close_http(self) -> None: client = Lithic(base_url=base_url, api_key=api_key, _strict_response_validation=True) assert not client.is_closed() @@ -735,9 +727,8 @@ def test_copied_client_does_not_close_http(self) -> None: copied = client.copy() assert copied is not client - copied.__del__() + del copied - assert not copied.is_closed() assert not client.is_closed() def test_client_context_manager(self) -> None: @@ -1554,15 +1545,6 @@ async def test_proxies_option_mutually_exclusive_with_http_client(self) -> None: http_client=http_client, ) - async def test_client_del(self) -> None: - client = AsyncLithic(base_url=base_url, api_key=api_key, _strict_response_validation=True) - assert not client.is_closed() - - client.__del__() - - await asyncio.sleep(0.2) - assert client.is_closed() - async def test_copied_client_does_not_close_http(self) -> None: client = AsyncLithic(base_url=base_url, api_key=api_key, _strict_response_validation=True) assert not client.is_closed() @@ -1570,10 +1552,9 @@ async def test_copied_client_does_not_close_http(self) -> None: copied = client.copy() assert copied is not client - copied.__del__() + del copied await asyncio.sleep(0.2) - assert not copied.is_closed() assert not client.is_closed() async def test_client_context_manager(self) -> None: