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.