Prevent background workers from holding ActiveRecord connections (#1320)

st0012 · web-flow · commit 76a6572fda2e · 2021-03-09T21:47:33.000+08:00
* Use file instead of memory as sqlite database

For concurrency testing.

* Add failing spec

* Improve test cases

* Make sure BackgroundWorker returns AR connection

* Update changelog
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
