Modifying with_connection method

example of why nesting protection is very helpful

def x
  using_connection do
    User.all.each {|u| y(u) }
  end
end

def y(u)
  using_connection do
    u.something = rand
    u.save!
  end
end

def z
  y(@user)
end

Basically it allows you to only wrap code which is immediately doing an AR update without having to worry about nesting issues.

added more real-world test case for nesting safety

renamed thread local variable to cleanup_connection

+1

+1

This patch lets me use ActiveRecord in Ramaze. A most timely patch.

Moved cleanup_connection method to ConnectionHandler. Using clear_active_connections! instead of release_connection so we release connections from all connection pools in use by the current thread.

As requested by lifo

Real World Usecase:

You have 100 threads which stay alive and running throughout the entire life of your process. These threads are intermittently doing AR calls. Now assume you can't or don't want to increase your connection pool size to 100. Or assume instead of 100 its some unknown number of threads which might be higher than you can set your connection pool size to.

Without this patch each thread will hold onto a single connection even though it's not making use of it most of the time. With this patch you can wrap all your AR access in a cleanup_connection block and the thread will release the connection whenever its finished with it. This will allow you to get away with a small connection pool even with lots of threads using AR.

I can appreciate the desire for this patch, but what about just piggy-backing off the existing connection method instead? Consider this patch:

http://gist.github.com/66038

It's just a proof-of-concept, may need some cleaning up and adding tests/merging w/ coderrr's patch.

What do you think?

Hey nick

The only issue with that is that if you have multiple connections (in different AR models) you'd need to do a connection call for each of them. If you forgot to do that these connections would remain open in the thread.

User.connection { Project.connection { ... } }

Not that big deal. Other than that I don't really have any objection to piggybacking.

If you're satisfied with that pattern, we could push the logic up to the AR::Base class method to do the cleanup and it could do so with clear_active_connections! like you have in your patch.

Any other thoughts about the overloading of AR::Base.connection?

Here's a patch w/ coderrr's tests and my approach with passing the block down through AR::Base.connection.

Don't we already have a with_connection method on the connection pool?

Perhaps we just make that available from AR::Base more simply?

with_connection isn't good enough. It only releases the connection which it checks out. And the connection it checks out will be different from AR::B#connection

+1

Koz: with_connection doesn't associate the connection with the thread, so that regular code like Model.find would still pull another connection. That's yet another option to make with_connection stash the connection away in the thread if it isn't there.

Just seems to me like unless there's a reason that with_connection shouldn't do that, then it'd be cleaner than messing with .connection

What's the behaviour of this new method if a connection is already checked out, does it check out another and replace all the thread locals?

Well there are two versions...

In the original version (cleanup_connection method), if a connection is already checked out it won't check out a new one. But will release it when the block completes.

In the latest version it wouldn't check out a new connection either but also wouldn't release the connection afterwards.

I don't have a problem with putting the functionality in with_connection. I agree changing connection method is a heavier change, even though without a block it would function as before. I really don't care where this functionality ends up, but at this point if I had to vote, it seems that modifying the connection method would be cleanest and point people in the right direction in terms of checkins/checkout.

I've noticed that ActiveRecord::Base.connection_pool.with_connection doesn't override the connection for all your models...

If you plan to

ActiveRecord::Base.connection_pool.with_connection do |conn|

u = User.new
u.save!

end

then your user object is not saved with the connection you opened with_connection. Instead you have to do:

User.connection_pool.with_connection do |conn|

u = User.new
u.save!

end

Is this by design? Why?

If we change is_connection to:

  def with_connection
    conn = checkout
    old_conn = @reserved_connections[current_connection_id]
    @reserved_connections[current_connection_id] = conn
    yield conn
  ensure
    checkin conn
    @reserved_connections[current_connection_id] = old_conn
  end

Then it works as I had expected it should.

Yes, the issue you pointed out is exactly why I created this ticket.

We need more than what your implementation of with_connection provides. If you have nested calls to with_connection you'll check out a new connection for each one where you really want to be using the same connection throughout all of the calls.

So what's the deal? Do you have any response Koz?