Connections are not garbage collected

robertgrimm commented 9 years ago

I noticed the Redis class makes no attempt to automatically disconnect/quit its client connection when the Redis object is garbage collected and instead appears to force the application to manage that. Is this by design? While applications can of course keep track of this themselves and call Redis#quit when appropriate, it appears that the Redis class can be modified to do that automatically so applications don't have to worry about that and thus not leak Redis connections.

For example:

class Redis
  def initialize(options={})
     # existing initialize
     # not sure if finalize should be called with @original_client or just client.
     ObjectSpace.define_finalizer(self, self.class.finalize(@original_client))
  end

  def self.finalize(client)
    proc { client.disconnect }
  end

  # rest of the class definition
end

This can be tested by doing something like

redis = Redis.new
# Would be nice if the gem natively supported "client setname"...
redis.client.call(["client", "setname", "test"])
# Run "redis-cli client list | grep test" or something similar to see the connection
redis = nil
GC.start
# Run "redis-cli client list | grep test" again and see the connection is no longer there

Another option would be add something like this to the Client class instead. Thoughts?

fotinakis commented 9 years ago

May or not be related, but I'm currently seeing a lot of Ruby segfaults like this:

ruby/2.2.0/gems/redis-3.2.1/lib/redis/connection/ruby.rb:49: [BUG] object allocation during garbage collection phase

fotinakis commented 9 years ago

I definitely agree with this issue — I've found that the redis-rb library can easily consume the limited number of connections I'm paying for on a third-party redis service, and so I have to do a lot of nasty application and system configuration to aggressively cleanup the connections.

BBCMarkMcDonnell commented 8 years ago

Wow, I'm surprised this hasn't gotten more traction/feedback from the Redis gem authors?

I've been load testing a collection of microservices hosted on AWS and discovered that our connections weren't ever dropping down once opened. I've only just started looking into why, and this issue popped up first on Google :-(

I'll have to take a look at @hale's commit/fix and fork my own version.

badboy commented 8 years ago

Hey @BBCMarkMcDonnell. We maitain redis-rb in our free time and may not face the same problems as heavy users. Looks like this issue just fell through. @hale's fix was never submitted as a PR, so we didn't really see that either. I now pulled it in (#603) and will wait for the CI to report back.

Thanks for bringing this up again.

Integralist commented 8 years ago

Thanks @badboy for the feedback, I wasn't aware of the situation. My apologies. I'm grateful you're looking back into this. Much appreciated!

islemaster commented 8 years ago

Hi there! Has there been any progress on this? It looks like #603 has been abandoned since April.

Our Rails application is using redis-rb on multiple frontend nodes to direct traffic to a Redis cluster on AWS. We upgraded redis-rb from v3.1.0 to v3.3.1 earlier this week. Since then, our open connection counts have been rising:

screenshot from 2016-09-02 18-09-37

I'm not totally sure the redis-rb upgrade is the cause, but the timing is right. We never added manual disconnect calls in our application - the close method isn't documented in the readme, and most of the examples don't contain close/disconnect calls either. And everything seemed okay at 3.1.0.

I'll be debugging on our end and will post anything I learn here. Thanks!

fotinakis commented 8 years ago

I have also manually added a bunch of redis.disconnect! calls around our application to mitigate this.

erkki commented 8 years ago

I thought I was seeing similar issues and starting to look into it, could not reproduce the original report. @robertgrimm (or anyone else) can you still reproduce this on latest master?

islemaster commented 8 years ago

Update on our particular situation: I haven't found or fixed the cause of our frontends leaving connections open, but I did find a workaround for our particular case: Using the timeout parameter to make the Redis nodes drop idle clients.

This blog post from last September got me started. Once I was sure we could change the timeout on our nodes without any restart or service interruption we changed it from the default 0 (never expire) to 3600 (one hour), more than enough for our application.

The effect was dramatic, confirming my suspicion that most of these connections were stale.

screenshot from 2016-09-09 17-18-56

That dropoff at the end isn't just an incomplete data point - it's the moment we set timeout: 3600.

I was surprised that there's no expiration by default (on AWS anyway), but it sounds like that's the desired behavior for many applications - especially those using a small connection pool of long-lived connections, where it's very rare that connections are not cleaned up properly. In our case, I'm sure we never need a connection to last more than an hour so this will do for now.

willnet commented 4 years ago

I processed following script with redis-rb(4.2.2) and ruby 2.7.2 to see how GC works. GC seems to disconnect a connection.

Did someone fix it?

require 'redis'

redis = Redis.new
redis.get('foo')
sleep 10 # A connection exists
redis = nil
GC.start
sleep # There are no connection. GC seems to disconnect it.