Make connection_validator extension handle case where Database#valid_…

…connection? raises an exception

This shouldn't happen, but if it did, it resulted in a
corruption of the connection pool, with the connection allocated
to the current thread and never being checked back in.

Fix previous CHANGELOG entry while here.

CHANGELOG

@@ -1,6 +1,8 @@
 === master
 
-* Support :avoid_connection_compare_by_identity Database option to work around bugs in other libraries or the ruby implementation (jeremyevans) 
+* Make connection_validator extension handle case where Database#valid_connection? raises an exception (jeremyevans)
+
+* Support :compare_connections_by_identity Database option to work around bugs in other libraries or the ruby implementation (jeremyevans) 
 
 * Make mysql2 adapter handle invalid statement handles when closing prepared statements (jeremyevans)
 

lib/sequel/extensions/connection_validator.rb

@@ -105,18 +105,23 @@ def acquire(*a)
       1.times do
         if (conn = super) &&
            (timer = sync{@connection_timestamps.delete(conn)}) &&
-           Sequel.elapsed_seconds_since(timer) > @connection_validation_timeout &&
-           !db.valid_connection?(conn)
+           Sequel.elapsed_seconds_since(timer) > @connection_validation_timeout
 
-          case pool_type
-          when :sharded_threaded, :sharded_timed_queue
-            sync{@allocated[a.last].delete(Sequel.current)}
-          else
-            sync{@allocated.delete(Sequel.current)}
-          end
+          begin
+            valid = db.valid_connection?(conn)
+          ensure
+            unless valid
+              case pool_type
+              when :sharded_threaded, :sharded_timed_queue
+                sync{@allocated[a.last].delete(Sequel.current)}
+              else
+                sync{@allocated.delete(Sequel.current)}
+              end
 
-          disconnect_connection(conn)
-          redo
+              disconnect_connection(conn)
+              redo
+            end
+          end
         end
       end
 

spec/extensions/connection_validator_spec.rb

@@ -10,6 +10,7 @@ def disconnect_connection(conn)
       end
       def valid_connection?(conn)
         super
+        raise if conn.valid == :exception
         conn.valid
       end
       def connect(server)
@@ -54,6 +55,16 @@ def connect(server)
     c2.wont_be_same_as(c1)
   end
 
+  it "should assume that exceptions raised during valid_connection mean the connection is not valid" do
+    c1 = @db.synchronize{|c| c}
+    @db.sqls.must_equal []
+    c1.valid = :exception
+    @db.pool.connection_validation_timeout = -1
+    c2 = @db.synchronize{|c| c}
+    @db.sqls.must_equal ['SELECT NULL', 'disconnect']
+    c2.wont_be_same_as(c1)
+  end
+
   it "should handle Database#disconnect calls while the connection is checked out" do
     @db.synchronize{|c| @db.disconnect}
   end