From 3b2f6086be8abc96ec23b0c4a4f7bde23cdbb1c0 Mon Sep 17 00:00:00 2001 From: st0012 Date: Mon, 8 Mar 2021 00:20:14 +0800 Subject: [PATCH 1/5] Use file instead of memory as sqlite database For concurrency testing. --- sentry-rails/.gitignore | 1 + sentry-rails/spec/support/test_rails_app/app.rb | 9 +++++---- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/sentry-rails/.gitignore b/sentry-rails/.gitignore index b04a8c840..a618694de 100644 --- a/sentry-rails/.gitignore +++ b/sentry-rails/.gitignore @@ -5,6 +5,7 @@ /doc/ /pkg/ /spec/reports/ +/spec/support/test_rails_app/db /tmp/ # rspec failure tracking diff --git a/sentry-rails/spec/support/test_rails_app/app.rb b/sentry-rails/spec/support/test_rails_app/app.rb index 9fcc0b000..acdc79966 100644 --- a/sentry-rails/spec/support/test_rails_app/app.rb +++ b/sentry-rails/spec/support/test_rails_app/app.rb @@ -10,7 +10,11 @@ ActiveSupport::Deprecation.silenced = true -ActiveRecord::Base.establish_connection(adapter: "sqlite3", database: ":memory:") +# need to init app before establish connection so sqlite can place the database file under the correct project root +class TestApp < Rails::Application +end + +ActiveRecord::Base.establish_connection(adapter: "sqlite3", database: "db") ActiveRecord::Base.logger = Logger.new(nil) ActiveRecord::Schema.define do @@ -30,9 +34,6 @@ class Comment < ActiveRecord::Base belongs_to :post end -class TestApp < Rails::Application -end - class PostsController < ActionController::Base def index Post.all.to_a From 6cb8b954baac737bfc5a2b7c17fd864108620f24 Mon Sep 17 00:00:00 2001 From: st0012 Date: Sun, 7 Mar 2021 16:28:36 +0800 Subject: [PATCH 2/5] Add failing spec --- sentry-rails/spec/sentry/rails/client_spec.rb | 63 +++++++++++++++++++ 1 file changed, 63 insertions(+) create mode 100644 sentry-rails/spec/sentry/rails/client_spec.rb diff --git a/sentry-rails/spec/sentry/rails/client_spec.rb b/sentry-rails/spec/sentry/rails/client_spec.rb new file mode 100644 index 000000000..d0d0ffca5 --- /dev/null +++ b/sentry-rails/spec/sentry/rails/client_spec.rb @@ -0,0 +1,63 @@ +require "spec_helper" + +RSpec.describe Sentry::Client, type: :request do + let(:transport) do + Sentry.get_current_client.transport + end + + context "when serialization triggers ActiveRecord queries" do + before do + make_basic_app do |config| + config.background_worker_threads = 5 + # simulate connection being obtained during event serialization + # this could happen when serializing breadcrumbs + config.before_send = lambda do |event, hint| + Post.count + event + end + end + end + + it "doesn't hold the ActiveRecord connection after sending the event" do + threads = 5.times.map do |i| + Thread.new do + Sentry::Rails.capture_message("msg", hint: { index: i }) + end + end + + threads.join + + sleep(0.1) + + expect(transport.events.count).to eq(5) + + pool = ActiveRecord::Base.connection_pool + expect(pool.stat[:busy]).to eq(1) + end + end + + context "when doesn't serialization trigger ActiveRecord queries" do + before do + make_basic_app do |config| + config.background_worker_threads = 5 + end + end + + it "doesn't hold the ActiveRecord connection after sending the event" do + threads = 5.times.map do |i| + Thread.new do + Sentry::Rails.capture_message("msg", hint: { index: i }) + end + end + + threads.join + + sleep(0.1) + + expect(transport.events.count).to eq(5) + + pool = ActiveRecord::Base.connection_pool + expect(pool.stat[:busy]).to eq(1) + end + end +end From e59a7a1f071bcc0e493a6cd73f4a3b6608cd1fcf Mon Sep 17 00:00:00 2001 From: st0012 Date: Sun, 7 Mar 2021 16:19:12 +0800 Subject: [PATCH 3/5] Improve test cases --- sentry-rails/spec/sentry/rails/client_spec.rb | 40 +++++++++---------- 1 file changed, 19 insertions(+), 21 deletions(-) diff --git a/sentry-rails/spec/sentry/rails/client_spec.rb b/sentry-rails/spec/sentry/rails/client_spec.rb index d0d0ffca5..99bf82f73 100644 --- a/sentry-rails/spec/sentry/rails/client_spec.rb +++ b/sentry-rails/spec/sentry/rails/client_spec.rb @@ -1,10 +1,22 @@ require "spec_helper" +return unless Gem::Version.new(Rails.version) >= Gem::Version.new('5.1.0') + RSpec.describe Sentry::Client, type: :request do let(:transport) do Sentry.get_current_client.transport end + before do + expect(ActiveRecord::Base.connection_pool.stat[:busy]).to eq(1) + end + + def send_events + 5.times.map do + Thread.new { Sentry::Rails.capture_message("msg") } + end.join + end + context "when serialization triggers ActiveRecord queries" do before do make_basic_app do |config| @@ -19,45 +31,31 @@ end it "doesn't hold the ActiveRecord connection after sending the event" do - threads = 5.times.map do |i| - Thread.new do - Sentry::Rails.capture_message("msg", hint: { index: i }) - end - end + send_events - threads.join - - sleep(0.1) + sleep(0.5) expect(transport.events.count).to eq(5) - pool = ActiveRecord::Base.connection_pool - expect(pool.stat[:busy]).to eq(1) + expect(ActiveRecord::Base.connection_pool.stat[:busy]).to eq(1) end end - context "when doesn't serialization trigger ActiveRecord queries" do + context "when serialization doesn't trigger ActiveRecord queries" do before do make_basic_app do |config| config.background_worker_threads = 5 end end - it "doesn't hold the ActiveRecord connection after sending the event" do - threads = 5.times.map do |i| - Thread.new do - Sentry::Rails.capture_message("msg", hint: { index: i }) - end - end - - threads.join + it "doesn't create any extra ActiveRecord connection when sending the event" do + send_events sleep(0.1) expect(transport.events.count).to eq(5) - pool = ActiveRecord::Base.connection_pool - expect(pool.stat[:busy]).to eq(1) + expect(ActiveRecord::Base.connection_pool.stat[:busy]).to eq(1) end end end From 238f76ff33dfeef7954db6a87814f5a08954459c Mon Sep 17 00:00:00 2001 From: st0012 Date: Sun, 7 Mar 2021 16:22:18 +0800 Subject: [PATCH 4/5] Make sure BackgroundWorker returns AR connection --- sentry-rails/lib/sentry/rails.rb | 1 + sentry-rails/lib/sentry/rails/background_worker.rb | 14 ++++++++++++++ 2 files changed, 15 insertions(+) create mode 100644 sentry-rails/lib/sentry/rails/background_worker.rb diff --git a/sentry-rails/lib/sentry/rails.rb b/sentry-rails/lib/sentry/rails.rb index 66ed90314..e24d628af 100644 --- a/sentry-rails/lib/sentry/rails.rb +++ b/sentry-rails/lib/sentry/rails.rb @@ -1,5 +1,6 @@ require "sentry-ruby" require "sentry/integrable" +require "sentry/rails/background_worker" require "sentry/rails/configuration" require "sentry/rails/engine" require "sentry/rails/railtie" diff --git a/sentry-rails/lib/sentry/rails/background_worker.rb b/sentry-rails/lib/sentry/rails/background_worker.rb new file mode 100644 index 000000000..058956c89 --- /dev/null +++ b/sentry-rails/lib/sentry/rails/background_worker.rb @@ -0,0 +1,14 @@ +module Sentry + class BackgroundWorker + alias_method :original_perform, :perform + + def perform(&block) + @executor.post do + # make sure the background worker returns AR connection if it accidentally acquire one during serialization + ActiveRecord::Base.connection_pool.with_connection do + block.call + end + end + end + end +end From 05db7ab8b69db965fdd067600b1cd1b552c7dab8 Mon Sep 17 00:00:00 2001 From: st0012 Date: Tue, 9 Mar 2021 21:42:31 +0800 Subject: [PATCH 5/5] Update changelog --- sentry-rails/CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/sentry-rails/CHANGELOG.md b/sentry-rails/CHANGELOG.md index 9b15ba2e8..6edc1c95c 100644 --- a/sentry-rails/CHANGELOG.md +++ b/sentry-rails/CHANGELOG.md @@ -3,6 +3,7 @@ ## Unreleased - Support performance monitoring on ActiveJob execution [#1304](https://github.com/getsentry/sentry-ruby/pull/1304) +- Prevent background workers from holding ActiveRecord connections [#1320](https://github.com/getsentry/sentry-ruby/pull/1320) ## 4.2.2