Fix connection leak when outside Rails executor with no connection op…

…en (#21)

Usually all of `after_commit` invocations (whether it happens during serving HTTP request in Rails controller or performing job in Sidekiq worker process) are always done inside Rails executor which checks in any connections back to the connection pool that were checked out inside its block.
See https://guides.rubyonrails.org/threading_and_code_execution.html for details

However, in cases when
 a) `after_commit` was called outside of Rails executor (3-rd party gems or non-Rails apps using ActiveRecord)
 b) **and** database connection hasn't been checked out yet
then connection will be checked out by `after_commit` implicitly by call to `ActiveRecord::Base.connection` and not checked in back afterwards causing it to _leak_.

But in that case we can be pretty sure that there is no transaction in progress (cause one need to checkout connection and issue `BEGIN` to it), so we don't need to check it out at all and can fast-forward to `without_tx` action.

Fixes Envek/after_commit_everywhere#20

lib/after_commit_everywhere.rb

@@ -30,7 +30,7 @@ class << self
     # Runs +callback+ after successful commit of outermost transaction for
     # database +connection+.
     #
-    # @param connection [ActiveRecord::ConnectionAdapters::AbstractAdapter]
+    # @param connection [ActiveRecord::ConnectionAdapters::AbstractAdapter] Database connection to operate in. Defaults to +ActiveRecord::Base.connection+
     # @param without_tx [Symbol] Determines the behavior of this function when
     #   called without an open transaction.
     #
@@ -39,7 +39,7 @@ class << self
     # @param callback   [#call] Callback to be executed
     # @return           void
     def after_commit(
-      connection: ActiveRecord::Base.connection,
+      connection: nil,
       without_tx: EXECUTE,
       &callback
     )
@@ -55,7 +55,7 @@ def after_commit(
     #
     # Available only since Ruby on Rails 5.0. See https://github.com/rails/rails/pull/18936
     #
-    # @param connection [ActiveRecord::ConnectionAdapters::AbstractAdapter]
+    # @param connection [ActiveRecord::ConnectionAdapters::AbstractAdapter] Database connection to operate in. Defaults to +ActiveRecord::Base.connection+
     # @param without_tx [Symbol] Determines the behavior of this function when
     #   called without an open transaction.
     #
@@ -64,7 +64,7 @@ def after_commit(
     # @param callback   [#call] Callback to be executed
     # @return           void
     def before_commit(
-      connection: ActiveRecord::Base.connection,
+      connection: nil,
       without_tx: WARN_AND_EXECUTE,
       &callback
     )
@@ -86,11 +86,11 @@ def before_commit(
     # Caveat: do not raise +ActivRecord::Rollback+ in nested transaction block!
     # See http://api.rubyonrails.org/classes/ActiveRecord/Transactions/ClassMethods.html#module-ActiveRecord::Transactions::ClassMethods-label-Nested+transactions
     #
-    # @param connection [ActiveRecord::ConnectionAdapters::AbstractAdapter]
+    # @param connection [ActiveRecord::ConnectionAdapters::AbstractAdapter] Database connection to operate in. Defaults to +ActiveRecord::Base.connection+
     # @param callback   [#call] Callback to be executed
     # @return           void
     # @raise            [NotInTransaction] if called outside transaction.
-    def after_rollback(connection: ActiveRecord::Base.connection, &callback)
+    def after_rollback(connection: nil, &callback)
       register_callback(
         connection: connection,
         name: __method__,
@@ -100,7 +100,7 @@ def after_rollback(connection: ActiveRecord::Base.connection, &callback)
     end
 
     # @api private
-    def register_callback(connection:, name:, without_tx:, callback:)
+    def register_callback(connection: nil, name:, without_tx:, callback:)
       raise ArgumentError, "Provide callback to #{name}" unless callback
 
       unless in_transaction?(connection)
@@ -116,14 +116,26 @@ def register_callback(connection:, name:, without_tx:, callback:)
           raise ArgumentError, "Invalid \"without_tx\": \"#{without_tx}\""
         end
       end
+
+      connection ||= default_connection
       wrap = Wrap.new(connection: connection, "#{name}": callback)
       connection.add_transaction_record(wrap)
     end
 
     # Helper method to determine whether we're currently in transaction or not
-    def in_transaction?(connection = ActiveRecord::Base.connection)
+    def in_transaction?(connection = nil)
+      # Don't establish new connection if not connected: we apparently not in transaction
+      return false unless connection || ActiveRecord::Base.connection_pool.connected?
+
+      connection ||= default_connection
       # service transactions (tests and database_cleaner) are not joinable
       connection.transaction_open? && connection.current_transaction.joinable?
     end
+
+    private
+
+    def default_connection
+      ActiveRecord::Base.connection
+    end
   end
 end

spec/after_commit_everywhere_spec.rb

@@ -23,7 +23,7 @@
     subject do
       example_class.new.after_commit do
         handler.call
-        expect(ActiveRecord::Base.connection.transaction_open?).to be_falsey
+        expect(ActiveRecord::Base.connection.transaction_open?).to(be_falsey) if ActiveRecord::Base.connection_pool.connected?
       end
     end
 
@@ -174,6 +174,18 @@
         expect(handler).not_to have_received(:call)
       end
     end
+
+    it "doesn't leak connections" do
+      expect { subject }.not_to change { ActiveRecord::Base.connection_pool.connections.size }
+    end
+
+    context "when connection to the database isn't established" do
+      before { ActiveRecord::Base.connection_pool.disconnect! }
+
+      it "doesn't leak connections" do
+        expect { subject }.not_to change { ActiveRecord::Base.connection_pool.connections.size }
+      end
+    end
   end
 
   describe "#before_commit" do
@@ -261,6 +273,18 @@
           expect(handler).not_to have_received(:call)
         end
       end
+
+      it "doesn't leak connections" do
+        expect { subject }.not_to change { ActiveRecord::Base.connection_pool.connections.size }
+      end
+
+      context "when connection to the database isn't established" do
+        before { ActiveRecord::Base.connection_pool.disconnect! }
+
+        it "doesn't leak connections" do
+          expect { subject }.not_to change { ActiveRecord::Base.connection_pool.connections.size }
+        end
+      end
     end
 
     context "with nested transactions" do