Skip to content
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

Refactor async client connection-handling #280

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

abrookins
Copy link
Collaborator

@abrookins abrookins commented Feb 13, 2025

Refactor interfaces related to the Redis client in AsyncRedisIndex (and possibly others).

Constraints

  • We want to initialize the Redis connection lazily, not at object instantiation or module import.
  • Python doesn't have async properties, so properties are limited to sync function calls
    • Therefore, we don't want properties to be part of the interface
  • For functions we "remove," we'll leave them in place with a deprecation warning
    • Remaining backwards-compatible until the next major release

The Design Today

  • Can create a client and set it using index.connect()
    • Pass in a redis_url
    • Calls index.set_client()
  • Can pass in a client and set it with index.set_client()
    • Pass in a client instance
  • Can't pass in a client instance with __init__()
    • Or anything URL, etc.
  • Can create a client with index.from_existing()
    • Pass it a redis_url
    • Calls index.set_client()
  • Can't use index as an async context manager
    • Requires explicit resource handling, atexit handler
    • But this is broken
  • The setup_async_redis() decorator creates a function wrapper that calls validate_async_redis() with every set_client() call
  • The RedisConnectionFactory.connect() returns incompatible types (sync, async Redis)
    • Sync vs. async depends on a parameter

The Design After Refactor

  • Can use as an async context manager
    • Either disconnect the index manually with disconnect() or use it as a context manager
    • async with Index()... will disconnect() after you exit the context block
  • Lazily instantiate client with self._get_client() method ("private")
  • Remove set_client() public interface if possible
    • Lazily instantiate client
  • Remove connect() public interface if possible
    • Lazily instantiate client
  • Leave from_existing()
  • Leave client() property, now just returns ._redis_client and can be None
  • Call validate_async_redis() when setting a new client instance for the first time
    • But make this an internal check rather than attached to public interface
  • Allow redis_url, redis_kwargs, or redis_client in __init__()

Examples Post-Refactor

#1: New instance with redis URL
index = AsyncRedisIndex(redis_url="...")
#2: New instance with existing client
index = AsyncRedisIndex(redis_client=client)
#3: New instance with `from_existing`
index = AsyncRedisIndex.from_existing(..., redis_url="...")

#4 Passing both a client and a URL is isallowed
try:
    index = AsyncRedisIndex(redis_client=client, redis_url="...")
except:
    pass
else:
    raise RuntimeError("Should have raised!")

# The client is lazily connected here, and we send telemetry.
await index.something_using_redis()
await index.something_else_using_redis()
# Close the client.
await index.close()

async with AsyncRedisIndex(redis_url="...") as index:
    # Same story: client is lazily created now
    await index.something_using_redis()
    await index.something_else_using_redis()
    # Client is automatically closed

# __enter__ is reentrant
async with index:
    # Lazily opens a new client connection.
    await index.a_third_thing_using_redis()
    # Client is automatically closed

@abrookins abrookins changed the title Fix/raae 595 async client tweaks Refactor async client connection-handling Feb 13, 2025
@abrookins abrookins marked this pull request as ready for review February 20, 2025 05:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant