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/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 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 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..99bf82f73 --- /dev/null +++ b/sentry-rails/spec/sentry/rails/client_spec.rb @@ -0,0 +1,61 @@ +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| + 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 + send_events + + sleep(0.5) + + expect(transport.events.count).to eq(5) + + expect(ActiveRecord::Base.connection_pool.stat[:busy]).to eq(1) + end + end + + 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 create any extra ActiveRecord connection when sending the event" do + send_events + + sleep(0.1) + + expect(transport.events.count).to eq(5) + + expect(ActiveRecord::Base.connection_pool.stat[:busy]).to eq(1) + end + end +end 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