Remove excess memoization of ConnAdapter instances

CHANGELOG.md

@@ -2,6 +2,11 @@
 
 All notable changes to this project will be documented in this file.
 
+## `master`
+
+Bug fixes:
+- [Fix excess memoization of `ConnAdapter` instances causing single global connection shared between threads and connections taken from ActiveRecord becoming shared between its pool and QC](https://github.com/QueueClassic/queue_classic/pull/318)
+
 ## [4.0.0-alpha1] - 2019-07-18
 
 Updates:

lib/queue_classic.rb

@@ -53,13 +53,17 @@ def self.has_connection?
   def self.default_conn_adapter
     t = Thread.current
     return t[:qc_conn_adapter] if t[:qc_conn_adapter]
-    adapter = if rails_connection_sharing_enabled?
-      ConnAdapter.new(ActiveRecord::Base.connection.raw_connection)
-    else
-      ConnAdapter.new
+
+    if rails_connection_sharing_enabled?
+      # AR connection should never be memoized, as call to
+      # ActiveRecord::Base.clear_active_connections! in current thread
+      # will cause it to return to AR pool, and it will become shared
+      # between memoized value and pool. And Rails does
+      # clear_active_connections! after each request
+      return ConnAdapter.new(ActiveRecord::Base.connection.raw_connection)
     end
 
-    t[:qc_conn_adapter] = adapter
+    t[:qc_conn_adapter] = ConnAdapter.new
   end
 
   def self.default_conn_adapter=(conn)

lib/queue_classic/queue.rb

@@ -19,7 +19,7 @@ def conn_adapter=(a)
     end
 
     def conn_adapter
-      @adapter ||= QC.default_conn_adapter
+      (defined?(@adapter) && @adapter) || QC.default_conn_adapter
     end
 
     # enqueue(m,a) inserts a row into the jobs table and trigger a notification.

queue_classic.gemspec

@@ -21,4 +21,5 @@ Gem::Specification.new do |spec|
   spec.require_paths = %w[lib]
 
   spec.add_dependency "pg", ">= 0.17", "< 2.0"
+  spec.add_development_dependency "activerecord", "~> 5.2.3"
 end

test/activerecord_integration_test.rb

@@ -0,0 +1,40 @@
+# frozen_string_literal: true
+
+require 'helper'
+require 'active_record'
+
+class QueueClassicActiverecordIntegrationTest < QCTest
+  def setup
+    super
+    reset_globals
+    ActiveRecord::Base.establish_connection "#{ENV["DATABASE_URL"]}?pool=1"
+  end
+
+  def teardown
+    super
+  end
+
+  def test_connection_not_returned_to_pool
+    take_connection_until_checked = Mutex.new
+    conn_adapter_ready = Queue.new
+    connection_from_adapter = nil
+    take_connection_until_checked.synchronize do
+      Thread.new do
+        with_env 'QC_RAILS_DATABASE' => 'true' do
+          conn_adapter_ready.push QC.default_conn_adapter.connection
+        end
+        take_connection_until_checked.synchronize {}
+        ActiveRecord::Base.clear_active_connections!
+      end
+
+      connection_from_adapter = conn_adapter_ready.pop
+
+      ActiveRecord::Base.connection_pool.reap
+
+      # Pool now has no free connections available
+      assert_raises(ActiveRecord::ConnectionTimeoutError) do
+        ActiveRecord::Base.connection_pool.checkout 0.1
+      end
+    end
+  end
+end

test/helper.rb

@@ -3,7 +3,7 @@
 require "bundler"
 require "minitest/reporters"
 
-Bundler.setup :default, :test
+Bundler.setup :default, :development, :test
 
 if ENV['CIRCLECI'] == "true"
   Minitest::Reporters.use! Minitest::Reporters::JUnitReporter.new
@@ -12,6 +12,10 @@
 end
 
 ENV["DATABASE_URL"] ||= "postgres:///queue_classic_test"
+# Rails integration is enabled if ActiveRecord module constant is
+# present, and it will be present after tests that use
+# ActiveRecord. So forcifully disable Rails by default.
+ENV["QC_RAILS_DATABASE"] = "false"
 
 require_relative '../lib/queue_classic'
 require "stringio"
@@ -25,6 +29,7 @@ def setup
 
   def teardown
     QC.delete_all
+    reset_globals
   end
 
   def init_db
@@ -92,4 +97,9 @@ def stub_any_instance(class_name, method_name, definition)
       class_name.send(:undef_method, new_method_name)
     end
   end
+
+  def reset_globals
+    QC.default_conn_adapter = nil
+    QC.default_queue = nil
+  end
 end

test/lib/queue_classic_rails_connection_test.rb

@@ -3,23 +3,72 @@
 require File.expand_path("../../helper.rb", __FILE__)
 
 class QueueClassicRailsConnectionTest < QCTest
-  def before_setup
-    Object.send :const_set, :ActiveRecord, Module.new
-    ActiveRecord.const_set :Base, Module.new
+  # Stubs ActiveRecord::Base with empty module. Works if real
+  # ActiveRecord is loaded too, restoring it when unstubbing.
+  class StubActiveRecord
+    def stub
+      stub_module
+      stub_base
+    end
+
+    def unstub
+      unstub_base
+      unstub_module
+    end
+
+    private
+
+    def stub_module
+      unless Object.const_defined? :ActiveRecord
+        Object.send :const_set, :ActiveRecord, Module.new
+        @module_stubbed = true
+      else
+        @module_stubbed = false
+      end
+    end
+
+    def unstub_module
+      if @module_stubbed
+        Object.send :remove_const, :ActiveRecord
+      end
+    end
 
+    def stub_base
+      if Object.const_defined? 'ActiveRecord::Base'
+        @activerecord_orig = ActiveRecord::Base
+        ActiveRecord.send :remove_const, :Base
+      else
+        @activerecord_orig = nil
+      end
+      ActiveRecord.const_set :Base, Module.new
+    end
+
+    def unstub_base
+      ActiveRecord.send :remove_const, :Base
+      if @activerecord_orig
+        ActiveRecord.send :const_set, :Base, @activerecord_orig
+      end
+    end
+  end
+
+  def setup
+    super
+    @active_record_stub = StubActiveRecord.new
+    @active_record_stub.stub
     @original_conn_adapter = QC.default_conn_adapter
     QC.default_conn_adapter = nil
   end
 
-  def after_teardown
-    ActiveRecord.send :remove_const, :Base
-    Object.send :remove_const, :ActiveRecord
-
+  def teardown
+    @active_record_stub.unstub
     QC.default_conn_adapter = @original_conn_adapter
+    super
   end
 
   def test_uses_active_record_connection_if_exists
-    connection = get_connection
+    connection = with_env 'QC_RAILS_DATABASE' => nil do
+      get_connection
+    end
     assert connection.verify
   end