Rails Ruby on Rails → 2.x

Comments and changes to this ticket

• 

  Pratik January 13th, 2009 @ 11:03 PM

  • Tag changed from “patch” to “activerecord, connection, patch, pool”
  • Assigned user set to “Pratik”
  • Title changed from “added using_connection method; like with_connection but sets it as default connection while inside the block ” to “Adding using_connection method”

• You flagged this item as spam.
  

  ronin-37814 (at lighthouseapp) January 14th, 2009 @ 06:29 AM

  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.

• You flagged this item as spam.
  

  ronin-37814 (at lighthouseapp) January 14th, 2009 @ 07:58 PM

  • Title changed from “Adding using_connection method” to “Adding cleanup_connection method”

  Realized all I really care about is cleaning up the connection at the end of the block, so here's a simpler version. It doesn't explicitly check out the connection. That will be done by AR on the first call to AR::B.connection.

  Test for nesting safety added.

  • cleanup_connection.diff 3.2 KB

• You flagged this item as spam.
  

  ronin-37814 (at lighthouseapp) January 14th, 2009 @ 10:03 PM

  added more real-world test case for nesting safety

  • cleanup_connection.diff 3.8 KB

• You flagged this item as spam.
  

  ronin-37814 (at lighthouseapp) January 14th, 2009 @ 10:05 PM

  renamed thread local variable to cleanup_connection

  • cleanup_connection.diff 3.8 KB

• You flagged this item as spam.
  

  hosiawak January 17th, 2009 @ 10:59 PM

  +1

• You flagged this item as spam.
  

  Michael Lang February 4th, 2009 @ 05:35 AM

  +1

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

• You flagged this item as spam.
  

  ronin-37814 (at lighthouseapp) February 17th, 2009 @ 09:31 PM

  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.

  • cleanup_connection.diff 4.8 KB

• You flagged this item as spam.
  

  ronin-37814 (at lighthouseapp) February 17th, 2009 @ 10:25 PM

  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.

• You flagged this item as spam.
  

  Nick Sieger February 17th, 2009 @ 10:48 PM

  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?

• You flagged this item as spam.
  

  ronin-37814 (at lighthouseapp) February 17th, 2009 @ 11:01 PM

  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.

• You flagged this item as spam.
  

  Nick Sieger February 18th, 2009 @ 01:48 AM

  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?

• You flagged this item as spam.
  

  Nick Sieger February 18th, 2009 @ 08:28 PM

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

  • 0001-Allow-AR-Base.connection-to-take-a-block-meaning-cl.patch 6.5 KB
• 

  Michael Koziarski February 18th, 2009 @ 08:52 PM

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

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

• You flagged this item as spam.
  

  ronin-37814 (at lighthouseapp) February 18th, 2009 @ 08:55 PM

  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

• 

  Dan Pickett February 19th, 2009 @ 02:36 PM

  +1

• You flagged this item as spam.
  

  Nick Sieger February 19th, 2009 @ 05:04 PM

  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.

• 

  Michael Koziarski February 19th, 2009 @ 07:01 PM

  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?

• You flagged this item as spam.
  

  ronin-37814 (at lighthouseapp) February 19th, 2009 @ 07:10 PM

  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.

• You flagged this item as spam.
  

  Jacob Burkhart February 26th, 2009 @ 04:01 PM

  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.

• You flagged this item as spam.
  

  ronin-37814 (at lighthouseapp) February 26th, 2009 @ 04:08 PM

  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.

• You flagged this item as spam.
  

  ronin-37814 (at lighthouseapp) March 11th, 2009 @ 11:06 PM

  • Title changed from “Adding cleanup_connection method” to “Modifying with_connection method”

  I've moved the functionality to with_connection per Koz's suggestion.

  If a connection exists with_connection will simply yield the connection. If a connection doesn't exist with_connection will call connection which will checkout a connection and set it as the current connection, yield it, and ensure it's checked in after the block finishes.

  This ensures that any nested with_connection calls won't check out multiple connections.

  ... And it doesn't increase the # of lines of the with_connection method :)

  • with_connection.diff 3.8 KB
• 

  Pratik March 12th, 2009 @ 12:34 AM

  • Assigned user changed from “Pratik” to “Michael Koziarski”

• You flagged this item as spam.
  

  ronin-37814 (at lighthouseapp) April 29th, 2009 @ 04:50 AM

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

• 

  Repository May 1st, 2009 @ 02:58 PM

  • State changed from “new” to “resolved”

  (from [5501b99a19a2a67a9a920fd3c7bff071a2ecf058]) Ensure ActiveRecord::Base.connection_pool.with_connection creates a new connection only when needed [#1752 state:resolved]

  Signed-off-by: Pratik Naik pratiknaik@gmail.com http://github.com/rails/rails/co...