Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: Get connection from connection_pool instead of ActiveRecord::Base.connection #2278

Merged

Conversation

Iwaide
Copy link
Contributor

@Iwaide Iwaide commented Mar 20, 2024

Issue

When attempting to retrieve a connection in ActiveRecordSubscriber, a loop occurs, eventually leading to ActiveRecord::ConnectionTimeoutError.

In versions of Rails lower than 6, since the payload does not contain a connection, it tries to use ActiveRecord::Base.connection to get the current connection. However, it seems that this causes subscribe! to be called again.
I am not sure if this issue is specific to MySQL. Please refer to the following repository for a reproduction environment:
https://github.com/Iwaide/re_app_for_sentry/

Fix

I have modified the code to search for and retrieve a connection from ActiveRecord::Base.connection_pool.connections.
I am not very familiar with connection management, so I am not entirely confident that this is the appropriate approach. Please make any necessary corrections.

Copy link

codecov bot commented Mar 20, 2024

Codecov Report

Merging #2278 (11a1f51) into master (7d29d3c) will increase coverage by 0.02%.
Report is 2 commits behind head on master.
The diff coverage is 0.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2278      +/-   ##
==========================================
+ Coverage   97.61%   97.64%   +0.02%     
==========================================
  Files         112      112              
  Lines        4155     4154       -1     
==========================================
  Hits         4056     4056              
+ Misses         99       98       -1     
Components Coverage Δ
sentry-ruby 98.30% <ø> (-0.04%) ⬇️
sentry-rails 95.22% <0.00%> (+0.17%) ⬆️
sentry-sidekiq 94.70% <ø> (ø)
sentry-resque 92.30% <ø> (+1.53%) ⬆️
sentry-delayed_job 95.60% <ø> (ø)
sentry-opentelemetry 100.00% <ø> (ø)
Files Coverage Δ
...b/sentry/rails/tracing/active_record_subscriber.rb 85.71% <0.00%> (+2.95%) ⬆️

... and 2 files with indirect coverage changes

Copy link
Member

@sl0thentr0py sl0thentr0py left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @Iwaide !

@sl0thentr0py sl0thentr0py merged commit ffffce9 into getsentry:master Mar 20, 2024
119 of 122 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants