Necessary issues to resolve

[Andrew-Chen-Wang]

EDIT I am in the process of moving aioredis to redis-py at RedisLabs. Apologies for the wait.

---

Several issues will be resolved by #1156 which will probably be included in 2.1.0. Issues to be resolved with potential fixes:

• Warning caused by Redis.__del__() #1115
  • Just delete the __del__ magic method and get everyone to manually disconnect.
  • Fixed in Remove __del__ from Redis (Fixes #1115) #1227
• Pubsub: health check races with get_message to read from the socket #1217
  • The fix should be included once the parallel PR in redis-py is merged
  • Fixed in Fix health check after unsubscribe #1207
• [2.0] Should Lock default to using contextvars? #1040
  • Potentially can be resolved via the solution used at aredis
• aioredis 1.3.1 vs 2.0.0 performance #1208
  • Looking for a fix by @m-novikov. Any help at [RFC] Use callback based asyncio.Protocol for communication #1216 would be super appreciated!
• ConnectionResetError: [Errno 104] Connection reset by peer #778 (comment)
  • Fixed by ConnectionResetError: [Errno 104] Connection reset by peer #778 (comment)
  • Fixed by Redis update 2021-10-08 #1156 Retry class and get_connection: catch OSError too redis/redis-py#1832

Wishlist (not necessary to get into 2.0.1):

• Sentinel client ConnectionError "Too many connections" #1173
• RuntimeError: coroutine ignored GeneratorExit #1137
  • Resolved by using Hiredis
  • We should put something in the docs saying "INSTALL HIREDIS (RECOMMENDED)"
• Redis.close() doesn't close connection pool created in __init__ #1103
  • I'd like to know if this is still an issue.
  • To be fixed in Add auto_close_connection_pool and close_connection_pool in Redis.close() #1256 . The issue is that redis-py has auto cleanup code in __del__ but aioredis doesn't have the implicit capability since there is no async del method.

[Andrew-Chen-Wang]

Current plan for the port of #1156 as that is probably never going to merge. If anyone would like to help, please ping me in this thread:

• New winter semester is starting, so I'll be fairly busy.
• We should first reorganize our code to be similar in structure to redis-py. The issue now with Redis update 2021-10-08 #1156 is that it's not inline with current master. Incremental changes i.e. porting code from redis-py can be done much faster if we first do the reorganization
• Incremental PRs. We don't need to do a PR for each PR done in redis-py, but the port is humongous, so putting a few features in at a time reduces the burden.

Edit 2022-01-03: should be up to date with redis-py 4.0.1 now. Hoping to push it out in 2.1.0

[Andrew-Chen-Wang]

I am moving aioredis to redis-py now.

[zhukovgreen]

@Andrew-Chen-Wang we are using aioredis in https://github.com/cr0hn/aiohttp-cache/. What do we have to do? I am sorry, but it is not clear to me. Is it going to be a new pypi package with the new interface? So far at redis-py I can't see any asyncio support...

[Andrew-Chen-Wang]

Apologies, I shouldn't have changed the title so early. Yes, redis py will include an asyncio module that has the same interface as redis-py, just under redis.asyncio. We haven't actually merged the asyncio PR yet, but it will be in redis-py soon.

Edit: just for reference, if I didn't move this to redis-py, I would have to implement all of this alone: https://github.com/redis/redis/releases/tag/7.0-rc1

[kylebebak]

@Andrew-Chen-Wang

Yes, redis py will include an asyncio module that has the same interface as redis-py, just under redis.asyncio. We haven't actually merged the asyncio PR yet, but it will be in redis-py soon.

It looks like this is the PR you're referring to: redis/redis-py#1899

I don't see any issues where support for aynscio is mentioned in redis-py, except for this issue where you ask if Redis Labs would be willing to support aioredis: redis/redis-py#1813 (comment)

Have redis-py mantainers told you they're willing to add async support to their library? If they have, could you provide a link to that response from the redis-py maintainers? If they haven't, I think it's over-optimistic to tell people that the asyncio PR "will be in redis-py soon"

The PR you opened, redis/redis-py#1899, seems like it copies most of the code from this repo into the other repo. That doesn't address the issues you listed above that ought to be resolved in aioredis-py. Just to pick one that seems very serious: #1208. Our team hasn't upgraded from 1.3.1 because of this issue

I think it would be more productive to focus on improving the code in this repo, not on moving it all into another repo and hoping that this will cause professional maintainers to appear

@seandstewart @abrookins

[seandstewart]

Hey folks -

First, I want to say that I'm sorry for the hiatus. @Andrew-Chen-Wang thank you for continuing to provide support even as you work through your classes. I appreciate your effort to find a safe home for this code, as the risk with OSS which has no funding is that the maintainers may simply stop showing up. We've seen that a few times now with this library.

When Redis Labs volunteered to take over maintenance of this library, I was ecstatic. Unfortunately, that hasn't played out in reality, so it's natural to try and get robust asyncio support somewhere. This is an important library for the async community.

I've gone through the open PRs and merged a few that looked ready for release. I'm also going to spend a few hours today looking into #1208 and the related PR #1216. I hope to resolve this performance issue and any remaining open bugs in the next week and get a release out.

@Andrew-Chen-Wang, if redis/redis-py#1899 stalls or doesn't pan out, I think something we can look at to reduce maintenance burden could be to vendorize redis-py as an internal dependency using a git submodule. That would allow us to ingest upstream changes without having to do full re-implementations. When I ported to redis-py's implementation, the API was extremely stable and hadn't changed much in years, but it seems we can potentially expect a higher velocity of changes so trying to keep up may quickly become unreasonable.

[kylebebak]

@Andrew-Chen-Wang

I see redis/redis-py#1899 was merged, that's truly impressive!

@seandstewart , what do you think this means for the future of this repo?

For example, would you consider archiving this repo and doing async dev work in redis-py instead? Would you move issues such as the ones in this thread into redis-py so they can be tracked and resolved there?

[Andrew-Chen-Wang]

@kylebebak As of right now, there are actually some things that haven't been merged into redis py yet, and some issues in redis py that needs to be implemented (like the PubSub conn retry issue). So for now, this repo will continue to be under maintenance mode while everyone begins migrating to redis-py. In terms of current issues, I'm not sure GitHub allows us to transfer them between organizations.

After a complete migration to redis py, I'm thinking that, here, we can revert master to be at head of major version 1. The original code here was very interesting and deserves more attention. I definitely don't have the time to look after it, but it would be great to transfer maintainership again!

Finally, redis-py is under Redis, yes that Redis, maintenance with committers from AWS helping out too. Based on my previous conversations with Sean, neither of us really have the time to help out that much with async dev, but we're all looking into resolving 1) the lock issue and 2) the performance issue. But for the most part, though i can't really speak for Sean, I definitely won't have time to continue unless I'm requested for assistance again.

And sorry for a lack of response. I wasnt sure if Redis wanted to officially talk about the PR, so I kept quiet 😅 We were already talking about this migration for awhile before the PR.

[seandstewart]

@kylebebak, @Andrew-Chen-Wang

Seconding what Andrew said and adding that Ive been working diligently on identifying and resolving the performance issues with our async client. I made a breakthrough the other day and should have an update soon.

@Andrew-Chen-Wang, @chayim - I approached this performance issue from the ground up using Sans-IO principles in the hope that we can open the door to a shared model for our client that is not completely bound to a particular IO model.

[Andrew-Chen-Wang]

@seandstewart that sounds awesom3! Thanks a ton and awesome work! One concern I have, though I have no experience with Sans-Io, is that, for backwards compatibility, there tends to always be a split between sync and async classes. From my experience with Django and elasticsearch-py, my only concern would be how abstract are you making these two different IO classes or are you combining them into one? And if the latter, will users be confused by them? (ie when to await). Looking forward to the PR!

[kylebebak]

Agreed, that's great news. I think the performance issues are the most important ones to resolve. They're hard and they require low-level knowledge of this code base.

Even though Redis is single-threaded, it's super fast, and a client that performs well under load and high concurrency can really take advantage of that speed. I'm guessing performance is one of the main reasons someone chooses this Redis client over a blocking client.

Anyways, thanks for working on this @seandstewart !

[seandstewart]

Hey @Andrew-Chen-Wang @kylebebak

I have the initial sans-io implementation here:

https://github.com/seandstewart/redis-py-sansio

Most importantly, this approach allows us to optimize each IO implementation, so with a simple benchmark I'm seeing similar performance to aioreds v1.3.

[HassanAbouelela]

One thing that could possibly be added to that list is working on migration guides. The v1 -> v2 migration guide is missing at least a few changes, and the v2 -> redis-py mentions that it's "almost the exact same", but doesn't elaborate beyond that (if it's almost the exact same, what should we look to change in our codebases? If it is the same, perhaps the readme could be changed).

A few of the changes I've encountered which are not documented are:

• brpoplpush and rpoplpush no longer take sourcekey and destkey, but src and dst. This is an issue for people using keyword args
• hmset_dict has been removed. The alternatives are easy to find, but the guide should probably mention the removal
• iscan was renamed to scan_iter
• Exceptions are almost completely bare of any documentation. This is not as important as the other two if people are going to be migrating away from this library anyway

I could open a separate issue for this is it's more helpful.