Merge pull request rails#53672 from matthewd/last-activity

Author: Matthew Draper

activerecord/CHANGELOG.md

@@ -1,3 +1,13 @@
+*   Remember when a database connection has recently been verified (for
+    two seconds, by default), to avoid repeated reverifications during a
+    single request.
+
+    This should recreate a similar rate of verification as in Rails 7.1,
+    where connections are leased for the duration of a request, and thus
+    only verified once.
+
+    *Matthew Draper*
+
 *   Allow to reset cache counters for multiple records.
 
     ```

activerecord/lib/active_record/connection_adapters/abstract_adapter.rb

@@ -168,6 +168,7 @@ def initialize(config_or_deprecated_connection, deprecated_logger = nil, depreca
         @default_timezone = self.class.validate_default_timezone(@config[:default_timezone])
 
         @raw_connection_dirty = false
+        @last_activity = nil
         @verified = false
       end
 
@@ -217,6 +218,10 @@ def connection_retries
         (@config[:connection_retries] || 1).to_i
       end
 
+      def verify_timeout
+        (@config[:verify_timeout] || 2).to_i
+      end
+
       def retry_deadline
         if @config[:retry_deadline]
           @config[:retry_deadline].to_f
@@ -343,6 +348,13 @@ def seconds_idle # :nodoc:
         Process.clock_gettime(Process::CLOCK_MONOTONIC) - @idle_since
       end
 
+      # Seconds since this connection last communicated with the server
+      def seconds_since_last_activity # :nodoc:
+        if @raw_connection && @last_activity
+          Process.clock_gettime(Process::CLOCK_MONOTONIC) - @last_activity
+        end
+      end
+
       def unprepared_statement
         cache = prepared_statements_disabled_cache.add?(object_id) if @prepared_statements
         yield
@@ -670,6 +682,7 @@ def reconnect!(restore_transactions: false)
 
           enable_lazy_transactions!
           @raw_connection_dirty = false
+          @last_activity = Process.clock_gettime(Process::CLOCK_MONOTONIC)
           @verified = true
 
           reset_transaction(restore: restore_transactions) do
@@ -689,6 +702,7 @@ def reconnect!(restore_transactions: false)
             end
           end
 
+          @last_activity = nil
           @verified = false
 
           raise translated_exception
@@ -763,6 +777,7 @@ def verify!
               @raw_connection = @unconfigured_connection
               @unconfigured_connection = nil
               configure_connection
+              @last_activity = Process.clock_gettime(Process::CLOCK_MONOTONIC)
               @verified = true
               return
             end
@@ -992,6 +1007,9 @@ def with_raw_connection(allow_retry: false, materialize_transactions: true)
             if @verified
               # Cool, we're confident the connection's ready to use. (Note this might have
               # become true during the above #materialize_transactions.)
+            elsif (last_activity = seconds_since_last_activity) && last_activity < verify_timeout
+              # We haven't actually verified the connection since we acquired it, but it
+              # has been used very recently. We're going to assume it's still okay.
             elsif reconnectable
               if allow_retry
                 # Not sure about the connection yet, but if anything goes wrong we can
@@ -1033,6 +1051,7 @@ def with_raw_connection(allow_retry: false, materialize_transactions: true)
                 # Barring a known-retryable error inside the query (regardless of
                 # whether we were in a _position_ to retry it), we should infer that
                 # there's likely a real problem with the connection.
+                @last_activity = nil
                 @verified = false
               end
 
@@ -1047,6 +1066,7 @@ def with_raw_connection(allow_retry: false, materialize_transactions: true)
         # `with_raw_connection` block only when the block is guaranteed to
         # exercise the raw connection.
         def verified!
+          @last_activity = Process.clock_gettime(Process::CLOCK_MONOTONIC)
           @verified = true
         end
 

activerecord/lib/active_record/connection_adapters/mysql2_adapter.rb

@@ -106,7 +106,14 @@ def connected?
       end
 
       def active?
-        connected? && @lock.synchronize { @raw_connection&.ping } || false
+        if connected?
+          @lock.synchronize do
+            if @raw_connection&.ping
+              verified!
+              true
+            end
+          end
+        end || false
       end
 
       alias :reset! :reconnect!

activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb

@@ -349,6 +349,7 @@ def active?
         @lock.synchronize do
           return false unless @raw_connection
           @raw_connection.query ";"
+          verified!
         end
         true
       rescue PG::Error

activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb

@@ -207,7 +207,12 @@ def connected?
         !(@raw_connection.nil? || @raw_connection.closed?)
       end
 
-      alias_method :active?, :connected?
+      def active?
+        if connected?
+          verified!
+          true
+        end
+      end
 
       alias :reset! :reconnect!
 

activerecord/lib/active_record/connection_adapters/trilogy_adapter.rb

@@ -121,7 +121,7 @@ def connected?
       end
 
       def active?
-        connected? && @lock.synchronize { @raw_connection&.ping } || false
+        connected? && @lock.synchronize { @raw_connection&.ping; verified! } || false
       rescue ::Trilogy::Error
         false
       end

activerecord/test/cases/adapter_test.rb

@@ -611,11 +611,14 @@ def teardown
         assert_predicate @connection, :active?
       end
 
-      test "querying a 'clean' failed connection restores and succeeds" do
+      test "querying a 'clean' long-failed connection restores and succeeds" do
         remote_disconnect @connection
 
         @connection.clean! # this simulates a fresh checkout from the pool
 
+        # Backdate last activity to simulate a connection we haven't used in a while
+        @connection.instance_variable_set(:@last_activity, Process.clock_gettime(Process::CLOCK_MONOTONIC) - 5.minutes)
+
         # Clean did not verify / fix the connection
         assert_not_predicate @connection, :active?
 
@@ -627,11 +630,30 @@ def teardown
         assert_predicate @connection, :active?
       end
 
+      test "querying a 'clean' recently-used but now-failed connection skips verification" do
+        remote_disconnect @connection
+
+        @connection.clean! # this simulates a fresh checkout from the pool
+
+        # Clean did not verify / fix the connection
+        assert_not_predicate @connection, :active?
+
+        # Because the query cannot be retried, and we (mistakenly) believe the
+        # connection is still good, the query will fail. This is what we want,
+        # because the alternative would be excessive reverification.
+        assert_raises(ActiveRecord::AdapterError) do
+          Post.delete_all
+        end
+      end
+
       test "quoting a string on a 'clean' failed connection will not prevent reconnecting" do
         remote_disconnect @connection
 
         @connection.clean! # this simulates a fresh checkout from the pool
 
+        # Backdate last activity to simulate a connection we haven't used in a while
+        @connection.instance_variable_set(:@last_activity, Process.clock_gettime(Process::CLOCK_MONOTONIC) - 5.minutes)
+
         # Clean did not verify / fix the connection
         assert_not_predicate @connection, :active?