Rails not properly handling Oracle db connections/sesions in dev mode

We are running edge rails with oracle. After a few hundred requests all available sesions are used up. It seems prior connections are being left open. When this happens no one using the installation of Oracle can create a new session until you kill your mongrel/webrick server.

Patch #6928 addresses this problem, and i applied it to my vendor rails and it worked.

I think, if possible, this should go into 1.2....it really is a major pain, and for people like me who are introducing rails into enterprisey situations it doesn't help the effort...

cheers,tom

tdfowler@pacbell.net wrote:

We are running edge rails with oracle. After a few hundred requests all available sesions are used up. It seems prior connections are being left open. When this happens no one using the installation of Oracle can create a new session until you kill your mongrel/webrick server.

I'm not seeing this behavior. Reconnecting to Oracle on every request would suck regardless, even in dev mode.

Anyone else seeing this behavior?

Michael,

I have done some further investigating on this issue. I can reproduce this problem without fail. The root problem I believe is in clear_reloadable_connections! where it deletes the connection name from active_connections.

Patch 6928 stops sessions from building up, however, you then have the needless overhead of setting up a new oracle connection/session on each request.

I believe clear_reloadable_connections! should be as follows:

# Clears the cache which maps classes def clear_reloadable_connections!   @@active_connections.each do |name, conn|    if conn.supports_reloading?       conn.disconnect!       @@active_connections.delete(name)    end end end

Here is my complete analysis/trace of this through the code:

After each request dispatcher.rb calls reset_after_dispatch

if you are running in development (Dependencies.mechanism == :load) then reset_application! is called, which in turn calls:

"ActiveRecord::Base.clear_reloadable_connections! if defined?(ActiveRecord)"

  (connection_specification.rb:) # Clears the cache which maps classes 1: def clear_reloadable_connections! 2: @@active_connections.each do |name, conn| 3: conn.disconnect! if conn.supports_reloading? 4: @@active_connections.delete(name) 5: end 6: end

notice line 4 above deletes the connection name from active_connections.

Now when the next request comes in eventually ActiveRecord::Base.connection gets called. Which in turn ends up calling retrieve_connection:

1: def self.retrieve_connection #:nodoc: 2: # Name is nil if establish_connection hasn't been called for 3: # some class along the inheritance chain up to AR::Base yet. 4: if name = active_connection_name 5: if conn = active_connections[name] 6: # Verify the connection. 7: conn.verify!(@@verification_timeout) 8: elsif spec = @@defined_connections[name] 9: # Activate this connection specification. 10: klass = name.constantize 11: klass.connection = spec 12: conn = active_connections[name] 13: end 14: end 15: 16: conn or raise ConnectionNotEstablished 17: end

In this method you end up executing lines 9-12. The call to connection= on line 11 takes you to connection_specification.rb:

    # Set the connection for the class. 1: def self.connection=(spec) #:nodoc: 2: if spec.kind_of?(ActiveRecord::ConnectionAdapters::AbstractAdapter) 3: active_connections[name] = spec 4: elsif spec.kind_of?(ConnectionSpecification) 5: config = spec.config.reverse_merge(:allow_concurrency => @@allow_concurrency) 7: self.connection = self.send(spec.adapter_method, config) 8: elsif spec.nil? 9: raise ConnectionNotEstablished 10: else 11: establish_connection spec 12: end 13: end

where lines 5,6 get executed. line 7 calls oracle_connection in oracle_adapter.rb.

At this point you have 2 sessions open and will get an additional one on each request.

tdfowler@pacbell.net wrote:

I believe clear_reloadable_connections! should be as follows:

# Clears the cache which maps classes def clear_reloadable_connections!   @@active_connections.each do |name, conn|    if conn.supports_reloading?       conn.disconnect!       @@active_connections.delete(name)    end end end

Agreed. I don't fully understand the intent of the reloadable bit, from the comments on the initial checkin it seems related to a sqlite issue.

Can someone from core comment?

And agree that this seems like a blocker on 1.2.

Thanks for the quick reply Michael.

For what's worth, the above results were @work (RHEL 4, Oracle 10g)

Curious, I also tested under mysql 5 - with rails on one server, db on another. Each request sets up a new connection (monitored with mysqladmin processlist) Interesting thing is that it leaves 3 connections open, on the 4th request the 3 connections are cleared and new one is established. The sequence repeats. (the app had one controller and one model)

I also tested this out at home on windoze, with Oracle XE and saw the same issue.

To see the sessions building up on oracle I look at v$session table.

Thanks Again,

Tom

<resending w/ a more enticing subject for core folks> tdfowler@pacbell.net wrote:

I believe clear_reloadable_connections! should be as follows:

# Clears the cache which maps classes def clear_reloadable_connections!   @@active_connections.each do |name, conn|    if conn.supports_reloading?       conn.disconnect!       @@active_connections.delete(name)    end end end

Agreed. I don't fully understand the intent of the reloadable bit, from the comments on the initial checkin it seems related to a sqlite issue. I don't understand why the current code would _not_ disconnect the connection (if it doesn't support reloading), but would still delete it from the cache.

Can someone from core comment?

And agree that this seems like a blocker on 1.2.

<bump> (and w/ more comments below)

Michael A. Schoen wrote:

<resending w/ a more enticing subject for core folks>

I believe clear_reloadable_connections! should be as follows:

# Clears the cache which maps classes def clear_reloadable_connections!   @@active_connections.each do |name, conn|    if conn.supports_reloading?       conn.disconnect!       @@active_connections.delete(name)    end end end

Agreed. I don't fully understand the intent of the reloadable bit, from the comments on the initial checkin it seems related to a sqlite issue. I don't understand why the current code would _not_ disconnect the connection (if it doesn't support reloading), but would still delete it from the cache.

Can someone from core comment?

And agree that this seems like a blocker on 1.2.

Looking at this history of this, seems this was added to deal w/ a sqlite problem, in that sqlite requires a new connection in order to see schema changes. Presuming that's the only db that requires a new connection on every request, I'd suggest the following:

1) change #supports_reloading? to #requires_reloading? (or somesuch), make it clear that it's an issue of "requiring" a new connection (for sqlite only).

2) make the change above, such that both the cache is cleared and the connection dropped ONLY for sqlite.

1) change #supports_reloading? to #requires_reloading? (or somesuch), make it clear that it's an issue of "requiring" a new connection (for sqlite only).

2) make the change above, such that both the cache is cleared and the connection dropped ONLY for sqlite.

That should already be the case. The abstract implementation is:

      # Returns true if its safe to reload the connection between requests for development mode.       # This is not the case for Ruby/MySQL and it's not necessary for any adapters except SQLite.       def supports_reloading?         false       end

And only SQLite overwrites it.

Agree that it's a blocker. Please do investigate, if you have the time. Otherwise I'll do when I get around to it.

DHH wrote:

1) change #supports_reloading? to #requires_reloading? (or somesuch), make it clear that it's an issue of "requiring" a new connection (for sqlite only).

2) make the change above, such that both the cache is cleared and the connection dropped ONLY for sqlite.

That should already be the case. The abstract implementation is:

     # Returns true if its safe to reload the connection between requests for development mode.      # This is not the case for Ruby/MySQL and it's not necessary for any adapters except SQLite.      def supports_reloading?        false      end

And only SQLite overwrites it.

Agree that it's a blocker. Please do investigate, if you have the time. Otherwise I'll do when I get around to it.

The problem is that clear_reloadable_connections! is currently defined as:

       # Clears the cache which maps classes        def clear_reloadable_connections!          @@active_connections.each do |name, conn|            conn.disconnect! if conn.supports_reloading?            @@active_connections.delete(name)          end        end

So it only disconnects for sqlite, BUT removes the connection from the cache always.

I can provide a patch, just want to make sure I understood what was intended here.

So it only disconnects for sqlite, BUT removes the connection from the cache always.

I can provide a patch, just want to make sure I understood what was intended here.

You're right. That was a bug.

Could ya'all try now after http://dev.rubyonrails.org/changeset/5955 has been made?

I believe this is the last issue before 1.2 leaves the station.

DHH wrote:

> So it only disconnects for sqlite, BUT removes the connection from the > cache always. > > I can provide a patch, just want to make sure I understood what was > intended here.

You're right. That was a bug.

Could ya'all try now after http://dev.rubyonrails.org/changeset/5955 has been made?

I believe this is the last issue before 1.2 leaves the station.

I just updated, and it works for me.

tom

DHH wrote:

I believe this is the last issue before 1.2 leaves the station.

I've still got 2 Oracle-related tickets open, the first one in particular is a long-standing and silly bug (not possible to insert the string "null" into the db). Both are patched and ready-to-go...

  http://dev.rubyonrails.org/ticket/6997

  http://dev.rubyonrails.org/ticket/6780

Would it be possible to get this applied as well?

Thanks.

Both are patched and ready-to-go...

        http://dev.rubyonrails.org/ticket/6997

        http://dev.rubyonrails.org/ticket/6780

Would it be possible to get this applied as well?

done and done.