Skip to content

Refactor async client connection-handling #280

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

Merged
merged 26 commits into from
Feb 20, 2025

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
Copy link
Collaborator

@tylerhutcherson tylerhutcherson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huge effort; great work! Left a few comments, but once addressed merge away

@tylerhutcherson
Copy link
Collaborator

Ahh one more thing, we need to make sure to update the README

@abrookins
Copy link
Collaborator Author

Thanks, @tylerhutcherson! I saw a typo in a comment, too. I'll get those changes in and send it back to you this morning.

@tylerhutcherson tylerhutcherson merged commit 286315a into main Feb 20, 2025
36 checks passed
@tylerhutcherson tylerhutcherson deleted the fix/RAAE-595-async-client-tweaks branch February 20, 2025 21:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants