Best practice to cleanup DB connection/resources

[Man Vuong]

Hi,

In one of my projects, I'm using RestCore in Rails application, then I encountered this error:

ActiveRecord::ConnectionTimeoutError: connection pool was full

After debugging, I figured out it was because of some code related to ActiveRecord in callbacks, something likes this:

client.get("/friends") do |response|

  friend = User.find_by(id: response['id'])

  # ...

end

DB connection leak!

I also noticed a very unusual high memory usage. At the beginning I thought it was because I'm using thread pool size 1000, too many threads? But after decreasing the pool size to 500, it's still. So I thought there was something wrong with the GC.

Finally I could reproduce the problem by doing steps like this interesting thread. Ruby does have problem with GC and threads!

I end up with this monkey patch:

module RestCore

  # Patch RestCore::Promise

  module PromiseExt

    protected

    def protected_yield(*args)

      super

    ensure

      collect_garbage

    end

    def collect_garbage

      # Cleanup DB connections.

      ActiveRecord::Base.clear_active_connections!

      # Force the Garbage collector to run,

      # otherwise, useless resources born inside threads may be never released.

      GC.start

    end

  end

  class Promise

    prepend PromiseExt

  end

end

The DB connection leak was fixed. Memory usage was decreased, but not much.

So, my question is that is it a correct way to do cleanup unused resources in `protected_yield` method? and is there any better way to achieve it?

Any suggestion is appreciated.

Thanks.

[Lin Jen-Shin (godfat)]

On Thu, Mar 12, 2015 at 1:22 PM, Man Vuong <thanh...@gmail.com> wrote:
> Hi,
>
> In one of my projects, I'm using RestCore in Rails application, then I
> encountered this error:
>
> ActiveRecord::ConnectionTimeoutError: connection pool was full
>
> After debugging, I figured out it was because of some code related to
> ActiveRecord in callbacks, something likes this:
>
> client.get("/friends") do |response|
> friend = User.find_by(id: response['id'])
> # ...
> end
>
> DB connection leak!

Yes, they never fix this, and you have to wrap your code with:

ActiveRecord::Base.connection_pool.with_connection do
...
end

Or the database connection would leak. So you'll have to do something like:

client.get("/friends") do |response|

ActiveRecord::Base.connection_pool.with_connection do

friend = User.find_by(id: response['id'])

end
end

This is quite tedious indeed, but this is how it works for years :(
Sequel has no problem with this, it could cleanup itself.
http://sequel.jeremyevans.net/

On the other hand, if you're using ActiveRecord outside Rails,
with a threaded server, you would need this Rack middleware:

use ActiveRecord::ConnectionAdapters::ConnectionManagement

It would do something like the `with_connection` for the application.
I have no idea why they never fix this. Maybe they just don't use threads.

> I also noticed a very unusual high memory usage. At the beginning I thought
> it was because I'm using thread pool size 1000, too many threads? But after
> decreasing the pool size to 500, it's still. So I thought there was
> something wrong with the GC.
>
> Finally I could reproduce the problem by doing steps like this interesting
> thread. Ruby does have problem with GC and threads!

Given the gist: https://gist.github.com/ryanlecompte/3281509
I guess if you use `with_connection`, the GC would work more as expected.
I believe ActiveRecord holds the connection all the time, and the records
hold the connection all the time, therefore all AR objects are not garbage.
With `with_connection`, maybe they could become garbage and the GC
could work properly.

I believe AR should do something like Sequel to cleanup itself.
Hope we could see this in the future.

> I end up with this monkey patch:
>
> module RestCore
> # Patch RestCore::Promise
> module PromiseExt
> protected
>
> def protected_yield(*args)
> super
> ensure
> collect_garbage
> end
>
> def collect_garbage
> # Cleanup DB connections.
> ActiveRecord::Base.clear_active_connections!
>
> # Force the Garbage collector to run,
> # otherwise, useless resources born inside threads may be never
> released.
> GC.start
> end
> end
>
> class Promise
> prepend PromiseExt
> end
> end
>
>
> The DB connection leak was fixed. Memory usage was decreased, but not much.

Does the DB connection leak if you remove GC.start?
I never really know what GC.start would affect...

> So, my question is that is it a correct way to do cleanup unused resources
> in `protected_yield` method? and is there any better way to achieve it?
>
> Any suggestion is appreciated.
>
> Thanks.

I guess this should work, and I agree that even if `with_connection` works
properly, it's quite tedious to do it in every single callback.

Maybe we could provide a callback wrapper, so that we could do something
like this:

client = Client.new(:callback_wrapper =>
ActiveRecord::Base.connection_pool.method(with_connection))

client.get("/friends") do |response|

# ... would work as if under `with_connection`
end

What do you think? My concern is that there are already some random stuffs,
like `error_callback`. I didn't think too much and added it because I
would like to
monitor all errors without fiddling around with ErrorHandler. I am a
bit worried that
all this kind of stuffs would grow out of control. I am still not sure
if `error_callback`
would be a good idea, it just... works for me, and that's it.

The name could be quite confusing as well...

Cheers,

[Man Vuong]

Looks like we cannot do much with AR connection, except managing it manually :D

Agree, Sequel is very lightweight :)

 

On the other hand, if you're using ActiveRecord outside Rails,
with a threaded server, you would need this Rack middleware:

    use ActiveRecord::ConnectionAdapters::ConnectionManagement

It would do something like the `with_connection` for the application.
I have no idea why they never fix this. Maybe they just don't use threads.

I think the Rails team really wants to fix it, but it's not an easy problem to fix. I'm not sure how it works in future version of Rails, but at least Rails 4.1.7 (the one I'm using) still has this shit lol

No, DB connection does not leak if I remove `GC.start`. I just tried to see if it could reduce memory. I have a unit test script to check the monkey patch:

class DummyObject


end


describe "garbage collection" do
  it "cleans up unused resources" do
    # Create some dummy object.
    obj = DummyObject.new


    2.times do
      client.get("/friends") do
        # More dummies...
        2.times { DummyObject.new }
      end
    end


    # Wait for all requests' finishing.
    client.wait


    expect(ObjectSpace.each_object(DummyObject).count).to eq(1)
  end
end

And it works as I expect, if I remove `GC.start`, this spec will fail because the number of DummyObject is larger than 1.

> So, my question is that is it a correct way to do cleanup unused resources
> in `protected_yield` method? and is there any better way to achieve it?
>
> Any suggestion is appreciated.
>
> Thanks.

I guess this should work, and I agree that even if `with_connection` works
properly, it's quite tedious to do it in every single callback.

Maybe we could provide a callback wrapper, so that we could do something
like this:

    client = Client.new(:callback_wrapper =>
      ActiveRecord::Base.connection_pool.method(with_connection))

I think using callback is so implicit, and not very convenient. How about a middleware likes `ActiveRecord::ConnectionAdapters::ConnectionManagement`, RestCore is Rack-style, so is it feasible to make a middleware like that?

[Lin Jen-Shin (godfat)]

On Fri, Mar 13, 2015 at 4:10 PM, Man Vuong <thanh...@gmail.com> wrote:
> On Thursday, March 12, 2015 at 1:07:57 PM UTC+7, Lin Jen-Shin (godfat)

>> On the other hand, if you're using ActiveRecord outside Rails,
>> with a threaded server, you would need this Rack middleware:
>>
>> use ActiveRecord::ConnectionAdapters::ConnectionManagement
>>
>> It would do something like the `with_connection` for the application.
>> I have no idea why they never fix this. Maybe they just don't use threads.
>>
> I think the Rails team really wants to fix it, but it's not an easy problem
> to fix. I'm not sure how it works in future version of Rails, but at least
> Rails 4.1.7 (the one I'm using) still has this shit lol

I am not sure if they really want to fix this, but I believe this is not
considered a bug (at least I believe a lot of people are aware of this),
so it might only be fixed as a new feature.

We're running 4.1.9 which also has this issue. I believe this is also true in
4.2+. Maybe we could expect this in 5+...

>> Does the DB connection leak if you remove GC.start?
>> I never really know what GC.start would affect...
>>
> No, DB connection does not leak if I remove `GC.start`. I just tried to see
> if it could reduce memory. I have a unit test script to check the monkey
> patch:
>
> class DummyObject
>
>
> end
>
>
> describe "garbage collection" do
> it "cleans up unused resources" do
> # Create some dummy object.
> obj = DummyObject.new
>
>
> 2.times do
> client.get("/friends") do
> # More dummies...
> 2.times { DummyObject.new }
> end
> end
>
>
> # Wait for all requests' finishing.
> client.wait
>
>
> expect(ObjectSpace.each_object(DummyObject).count).to eq(1)
> end
> end
>
>
> And it works as I expect, if I remove `GC.start`, this spec will fail
> because the number of DummyObject is larger than 1.

I am not sure why your application is using a lot of memory (well, so do
our application! :(), but this test cannot prove that GC.start would really
help though. GC would only start when the runtime system thinks that
they really need to release some memory (so that performance won't
be impacted unnecessarily), like memory pressure or, say if the CPU
is idling right now.

You could probably try putting a sleep somewhere to encourage GC to
run, this might make the test result a bit differently.

And if GC.start really helps, I might suggest to use a Rack middleware to
do this to avoid impacting performance too much.

> I think using callback is so implicit, and not very convenient. How about a
> middleware likes `ActiveRecord::ConnectionAdapters::ConnectionManagement`,
> RestCore is Rack-style, so is it feasible to make a middleware like that?

Good point, though this would only be needed in the case of callback,
and I should also change the error_callback to be a middleware.
I'll work on this when I got some chunk of free time.

Thanks!

(there would be a drawback though. If the client didn't use the middleware,
how do we enable this? I don't really want to put this in all the clients,
e.g. rest-firebase. Maybe we need a way to extend the existing clients
and edit the middleware stack! This might involve a lot of work :()