diff --git a/CHANGELOG.md b/CHANGELOG.md index b61ccadd..001d1e2d 100644 --- a/CHANGELOG.md +++ b/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: diff --git a/lib/queue_classic.rb b/lib/queue_classic.rb index de867a20..2a5bb55a 100644 --- a/lib/queue_classic.rb +++ b/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) diff --git a/lib/queue_classic/queue.rb b/lib/queue_classic/queue.rb index 4dd9084f..5eedba12 100644 --- a/lib/queue_classic/queue.rb +++ b/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. diff --git a/queue_classic.gemspec b/queue_classic.gemspec index aebb5073..20b47c92 100644 --- a/queue_classic.gemspec +++ b/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 diff --git a/test/activerecord_integration_test.rb b/test/activerecord_integration_test.rb new file mode 100644 index 00000000..9b8560e6 --- /dev/null +++ b/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 diff --git a/test/helper.rb b/test/helper.rb index 63218f69..5348f9da 100644 --- a/test/helper.rb +++ b/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 diff --git a/test/lib/queue_classic_rails_connection_test.rb b/test/lib/queue_classic_rails_connection_test.rb index bbc05a86..a9522ed8 100644 --- a/test/lib/queue_classic_rails_connection_test.rb +++ b/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