-
Notifications
You must be signed in to change notification settings - Fork 101
feat(index): owns_client for explicit Redis client ownership handover #726
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
c7d1bbb
13c0047
8877e4a
5934834
4558ffe
c2d54c9
e42acf5
0ffaab1
29c37b1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -803,7 +803,12 @@ def from_dict(cls, schema_dict: dict[str, Any], **kwargs): | |
| return cls(schema=schema, **kwargs) | ||
|
|
||
| def disconnect(self): | ||
| """Disconnect from the Redis database.""" | ||
| """Close the Redis client if this index owns it. | ||
|
|
||
| Always invalidates the cached SQL schema. When the index does not own | ||
| the client (see ``owns_client``), the client is left open and the | ||
| index remains usable. | ||
| """ | ||
| raise NotImplementedError("This method should be implemented by subclasses.") | ||
|
|
||
| def key(self, id: str) -> str: | ||
|
|
@@ -871,11 +876,11 @@ def __init__( | |
| redis_url: str | None = None, | ||
| connection_kwargs: dict[str, Any] | None = None, | ||
| validate_on_load: bool = False, | ||
| owns_client: bool | None = None, | ||
| **kwargs, | ||
| ): | ||
| """Initialize the RedisVL search index with a schema, Redis client | ||
| (or URL string with other connection args), connection_args, and other | ||
| kwargs. | ||
| """Initialize the RedisVL search index with a schema and either a Redis | ||
| client or a URL string with other connection kwargs. | ||
|
|
||
| Args: | ||
| schema (IndexSchema): Index schema object. | ||
|
|
@@ -887,6 +892,12 @@ def __init__( | |
| args. | ||
| validate_on_load (bool, optional): Whether to validate data against schema | ||
| when loading. Defaults to False. | ||
| owns_client (Optional[bool], optional): Whether the index closes | ||
| the Redis client when the index is disconnected or garbage | ||
| collected. Defaults to None, meaning the index owns a client | ||
| only if it created one itself. Pass True to hand over a client | ||
| you created, or False to keep one the index would otherwise | ||
| close, in which case closing it becomes your responsibility. | ||
| """ | ||
| if "connection_args" in kwargs: | ||
| connection_kwargs = kwargs.pop("connection_args") | ||
|
|
@@ -906,7 +917,18 @@ def __init__( | |
| self._sql_executors: dict[str, Any] = {} | ||
|
|
||
| self._validated_client = kwargs.pop("_client_validated", False) | ||
| self._owns_redis_client = kwargs.pop("_owns_redis_client", redis_client is None) | ||
| if "_owns_redis_client" in kwargs: | ||
| # Underscore-prefixed kwargs are forwarded verbatim by | ||
| # _split_from_existing_kwargs, so this would otherwise be dropped | ||
| # in silence and leak the connection it used to control. | ||
| raise TypeError( | ||
| "_owns_redis_client is no longer accepted; use owns_client instead" | ||
| ) | ||
| # Must be assigned before _register_client_finalizer, which gates on | ||
| # this flag. | ||
| self._owns_redis_client = ( | ||
| redis_client is None if owns_client is None else bool(owns_client) | ||
| ) | ||
| self._client_finalizer = None | ||
| # Close the owned client when this index is garbage collected. When | ||
| # the client is created lazily, registration happens at creation time | ||
|
|
@@ -916,10 +938,15 @@ def __init__( | |
| _finalizer_close_client = staticmethod(_close_owned_sync_client) | ||
|
|
||
| def disconnect(self): | ||
| """Disconnect from the Redis database.""" | ||
| """Close the Redis client if this index owns it. | ||
|
|
||
| Always invalidates the cached SQL schema. When the index does not own | ||
| the client (see ``owns_client``), the client is left open and the | ||
| index remains usable. | ||
| """ | ||
| self.invalidate_sql_schema_cache() | ||
| if self._owns_redis_client is False: | ||
| logger.info("Index does not own client, not disconnecting") | ||
| if not self._owns_redis_client: | ||
| logger.info("Index does not own its client; leaving it open") | ||
| return | ||
| self._detach_client_finalizer() | ||
| if self.__redis_client: | ||
|
|
@@ -943,6 +970,9 @@ def from_existing( | |
| instantiated redis client. | ||
| redis_url (Optional[str]): The URL of the Redis server to | ||
| connect to. | ||
| owns_client (Optional[bool], optional): Whether the index closes | ||
| the client. Defaults to True when this method created the | ||
| client from `redis_url`, and False when you supplied one. | ||
|
|
||
| Raises: | ||
| ValueError: If redis_url or redis_client is not provided. | ||
|
|
@@ -978,7 +1008,7 @@ def from_existing( | |
| schema_dict = convert_index_info_to_schema(index_info) | ||
| schema = IndexSchema.from_dict(schema_dict) | ||
| if created_redis_client: | ||
| init_kwargs["_owns_redis_client"] = True | ||
| init_kwargs.setdefault("owns_client", True) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. None owns_client leaks factory clientLow Severity
Additional Locations (2)Reviewed by Cursor Bugbot for commit 29c37b1. Configure here. |
||
| return cls( | ||
| schema, | ||
| redis_client=redis_client, | ||
|
|
@@ -2256,6 +2286,7 @@ def __init__( | |
| redis_client: AsyncRedisClient | None = None, | ||
| connection_kwargs: dict[str, Any] | None = None, | ||
| validate_on_load: bool = False, | ||
| owns_client: bool | None = None, | ||
| **kwargs, | ||
| ): | ||
| """Initialize the RedisVL async search index with a schema. | ||
|
|
@@ -2270,6 +2301,12 @@ def __init__( | |
| args. | ||
| validate_on_load (bool, optional): Whether to validate data against schema | ||
| when loading. Defaults to False. | ||
| owns_client (Optional[bool], optional): Whether the index closes | ||
| the Redis client when the index is disconnected or garbage | ||
| collected. Defaults to None, meaning the index owns a client | ||
| only if it created one itself. Pass True to hand over a client | ||
| you created, or False to keep one the index would otherwise | ||
| close, in which case closing it becomes your responsibility. | ||
| """ | ||
| if "redis_kwargs" in kwargs: | ||
| connection_kwargs = kwargs.pop("redis_kwargs") | ||
|
|
@@ -2282,15 +2319,30 @@ def __init__( | |
| self._validate_on_load = validate_on_load | ||
| self._lib_name: str | None = kwargs.pop("lib_name", None) | ||
|
|
||
| # Store connection parameters | ||
| # Store connection parameters. Note the asymmetry with SearchIndex: | ||
| # there, _redis_client is a property that lazily creates the client, | ||
| # whereas here it is a plain attribute that stays None until | ||
| # _get_client() creates one. Read it through _get_client(), not | ||
| # directly. | ||
| self._redis_client = redis_client | ||
| self._redis_url = redis_url | ||
| self._connection_kwargs = connection_kwargs or {} | ||
| self._lock = asyncio.Lock() | ||
| self._sql_executors: dict[str, Any] = {} | ||
|
|
||
| self._validated_client = kwargs.pop("_client_validated", False) | ||
| self._owns_redis_client = kwargs.pop("_owns_redis_client", redis_client is None) | ||
| if "_owns_redis_client" in kwargs: | ||
| # Underscore-prefixed kwargs are forwarded verbatim by | ||
| # _split_from_existing_kwargs, so this would otherwise be dropped | ||
| # in silence and leak the connection it used to control. | ||
| raise TypeError( | ||
| "_owns_redis_client is no longer accepted; use owns_client instead" | ||
| ) | ||
| # Must be assigned before _register_client_finalizer, which gates on | ||
| # this flag. | ||
| self._owns_redis_client = ( | ||
| redis_client is None if owns_client is None else bool(owns_client) | ||
| ) | ||
| self._client_finalizer = None | ||
| # Close the owned client when this index is garbage collected. When | ||
| # the client is created lazily, registration happens at creation time | ||
|
|
@@ -2316,6 +2368,9 @@ async def from_existing( | |
| instantiated redis client. | ||
| redis_url (Optional[str]): The URL of the Redis server to | ||
| connect to. | ||
| owns_client (Optional[bool], optional): Whether the index closes | ||
| the client. Defaults to True when this method created the | ||
| client from `redis_url`, and False when you supplied one. | ||
| """ | ||
| if not redis_url and not redis_client: | ||
| raise ValueError( | ||
|
|
@@ -2356,7 +2411,7 @@ async def from_existing( | |
| schema_dict = convert_index_info_to_schema(index_info) | ||
| schema = IndexSchema.from_dict(schema_dict) | ||
| if created_redis_client: | ||
| init_kwargs["_owns_redis_client"] = True | ||
| init_kwargs.setdefault("owns_client", True) | ||
| return cls( | ||
| schema, | ||
| redis_client=redis_client, | ||
|
|
@@ -3509,16 +3564,27 @@ async def info(self, name: str | None = None) -> dict[str, Any]: | |
| return await self._info(index_name, client) | ||
|
|
||
| async def disconnect(self): | ||
| """Close the Redis client if this index owns it. | ||
|
|
||
| Always invalidates the cached SQL schema. When the index does not own | ||
| the client (see ``owns_client``), the client is left open and the | ||
| index remains usable. | ||
| """ | ||
| self.invalidate_sql_schema_cache() | ||
| if self._owns_redis_client is False: | ||
| if not self._owns_redis_client: | ||
| return | ||
| self._detach_client_finalizer() | ||
| if self._redis_client is not None: | ||
| await self._redis_client.aclose() | ||
| self._redis_client = None | ||
|
|
||
| def disconnect_sync(self): | ||
| if self._redis_client is None or self._owns_redis_client is False: | ||
| """Close an owned Redis client from synchronous code. | ||
|
|
||
| For callers outside an event loop, such as ``__del__`` or a shutdown | ||
| hook. Honours ``owns_client`` exactly as :meth:`disconnect` does. | ||
| """ | ||
| if self._redis_client is None or not self._owns_redis_client: | ||
| return | ||
| sync_wrapper(self.disconnect)() | ||
|
|
||
|
|
||


There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cache lock breaks cross-loop reconnect
Medium Severity
The new
_async_client_lockis created in__init__and reused for the life of the cache. On Python 3.10–3.13 anasyncio.Lockbinds to the loop that first acquires it, so afteradisconnect()clears_async_redis_client, a later lazy reconnect on a new loop (a secondasyncio.run(), a new pytest-asyncio loop, Jupyter) raisesRuntimeErrorinstead of opening a new client.Additional Locations (1)
redisvl/extensions/cache/base.py#L142-L144Reviewed by Cursor Bugbot for commit 29c37b1. Configure here.