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

Redis connections keep increasing when used with async_to_sync #125

Closed
tiltec opened this issue Aug 31, 2018 · 11 comments

Comments

@tiltec
Copy link

commented Aug 31, 2018

I noticed that our task runner (huey) was keeping over 1000 connections to Redis open. After a day of digging around, I noticed that it can also happen in a plain Django Channels environment.

Here's a simple repro using django management commands:

https://github.com/tiltec/channels-redis-sync-repro/

It shows that sending with async_to_sync opens a new redis connection per message and never closes it again.

This is a regression from channels_redis 2.2.1.

Related issues:

@philipbelesky

This comment has been minimized.

Copy link

commented Sep 1, 2018

We've noticed what appears to be a similar problem. Users generally run our app on a Heroku environment with a highly constrained redis resource (30 max connections) although we only use channel layers / async_to_sync on a small number of pages. Since the upgrade we've noticed a lot ERR max number of clients reached being reported.

@prokher

This comment has been minimized.

Copy link

commented Sep 3, 2018

JFYI. We saw a similar issue some time ago. It seemed that the reason is that async_to_sync constructs new event loop for each call. (Well, if it is called from the thread other than the one created by sync_to_async.)

@ruralscenery

This comment has been minimized.

Copy link

commented Sep 4, 2018

in function sync_to_async after following codes:

if hasattr(loop, "shutdown_asyncgens"):
    loop.run_until_complete(loop.shutdown_asyncgens()) 

append following line, would be closed connection properly:

loop.run_until_complete(get_channel_layer().close_pools())
@michael-k

This comment has been minimized.

Copy link
Contributor

commented Sep 7, 2018

The PR that introduced connection pooling is: #117

@jberci in the PR you mention

we (…) haven't seen any problems internally that could be attributed to this in a while

I wonder why you did not run into this. Do you never send messages from sync code?

@m3nu

This comment has been minimized.

Copy link

commented Sep 7, 2018

Ran into this as well. Adding the line as described by @ruralscenery fixed it. In asgiref/sync.py:50

@andrewgodwin

This comment has been minimized.

Copy link
Member

commented Sep 7, 2018

Unfortunately we can't add that line into asgiref as it's specific to the channel layer. This problem needs to be solved in the Redis layer, and if not I'll have to roll back the connection pooling PR and revert to a pre-pooling world (I don't have the time to trace down what's going on in there).

In the meantime, if this affects you, please use version 2.2.1.

@ruralscenery

This comment has been minimized.

Copy link

commented Sep 8, 2018

@michael-k when you use channels 2 to send the message out of django box, for example, using celery task to send message by the channel_layer.send function, every call of channel_layer.send will create a new connection and new asyncio event loop, the new event loop code in asgiref/sync async_to_sync function close the event loop but redis connection, so it leaves the redis connection still alive there.

@jberci

This comment has been minimized.

Copy link
Contributor

commented Sep 10, 2018

Hmm. Some of our code does use async_to_sync, but most of it is async. Additionally, we always have an underlying running event loop, so async_to_sync calls won't generate new ones.

I'll look into this as soon as possible.

@adihat

This comment has been minimized.

Copy link

commented Oct 15, 2018

@andrewgodwin using channels-redis v2.2.1 certainly does solve this issue. Infact it makes the experience a lot better.
I hit the async_to_sync() repetedly using a loop and it updated the UI smoothly. Any reason this experience isn't seen in 2.3?

@michael-k

This comment has been minimized.

Copy link
Contributor

commented Oct 15, 2018

@adihat Please read the comments above.

This is a bug in 2.3 and will most certainly be fixed in the next release (a PR with a bugfix has already been merged). If you want to help out with testing the fix, please see #130 (comment)

@onekiloparsec

This comment has been minimized.

Copy link

commented Jul 8, 2019

This fix has a drastic impact on the number of connections used in a django app deployed on heroku, (and using RedisCloud) thanks a lot!
Screenshot 2019-07-07 at 22 12 07

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
10 participants
You can’t perform that action at this time.