Conversation
Unless told otherwhise, when this proxy class intercepts methods, it will create a connection if one does not already exist. https://github.com/zendesk/active_record_host_pool/blob/v0.11.0/lib/active_record_host_pool/pool_proxy.rb#L107-L108 In Rails 5.2.0, they added the following functionality which caused issues with code mentioned above https://github.com/rails/rails/blob/v5.2.3/activerecord/lib/active_record/railtie.rb#L180-L195 Here's the associated PRs rails/rails#28057 rails/rails#31221 This patch overrides the methods, retrieves the connection if it's active and calls super without creating a new one. This resolves an issue for applications upgrading to Rails 5.2.3 whereby connections would be created and thrown away during Rails.application.initialize!
mriddle
force-pushed
the
mriddle-rails-5.2.3-support
branch
4 times, most recently
from
f375bf0 to
b255483
Compare
New functionality was introduced in Rails 5.2 to ensure forked children don't send quit/shutdown/goodbye messages to the server on connections that belonged to their parent. See rails/rails#31173 for more information. We need to ensure that when the following line is called we also discard the pools we've cached. ActiveRecord::ConnectionAdapters:::ConnectionHandler.discard_unowned_pools Without this change, applications using this gem and Rails 5.2.3 will run into `NoMethodError (undefined method 'any?' for nil:NilClass)` exceptions from forked processes (e.g. Puma/Unicorn workers) since they'll be using the cached and discarded connection. The upshot of this change is we can do away with code like this: https://github.com/rails/rails/blob/c39ed435eb578c79867552c66da7eeb035fa58ad/railties/lib/rails/generators/rails/app/templates/config/puma.rb.tt#L35-L53
mriddle
force-pushed
the
mriddle-rails-5.2.3-support
branch
from
b255483 to
26b879d
Compare
Contributor
Author
|
These changes have also been verified against a Rails 5.1 application to ensure they're backwards compatible. |
mriddle
force-pushed
the
mriddle-rails-5.2.3-support
branch
from
26b879d to
5547a83
Compare
bquorning
reviewed
Aug 26, 2019
| # Verify that when we fork, the process doesn't crash | ||
| pid = Process.fork do | ||
| if ActiveRecord::VERSION::MAJOR == 5 && ActiveRecord::VERSION::MINOR == 2 | ||
| assert_equal(false, ActiveRecord::Base.connected?) # New to Rails 5.2 |
Member
There was a problem hiding this comment.
Did this test fail before the code change was applied?
Contributor
Author
There was a problem hiding this comment.
I verified the fix in 3 different ways
- Stashing the changes to
pool_proxy.rband running this unit test - Booting a Rails 5.2 console, forking a process and making a request to the DB
- Deploying a Rails 5.2 application with these changes and running smoke tests
bquorning
approved these changes
Aug 26, 2019
Member
|
I’d like to have an extra set of eyes on this PR – @grosser ? |
grosser
reviewed
Aug 26, 2019
grosser
reviewed
Aug 26, 2019
| end | ||
|
|
||
| def release_connection(owner_thread = Thread.current) | ||
| p = _connection_pool(false) |
Contributor
There was a problem hiding this comment.
does this work ?
_connection_pool(false)&.release_connection(owner_thread)
Contributor
Author
There was a problem hiding this comment.
It does, but I'd prefer to leave the 3 methods as-is. It's easier to grok at a glance IMO.
grosser
approved these changes
Aug 26, 2019
Add an example command on how to run tests
mriddle
force-pushed
the
mriddle-rails-5.2.3-support
branch
from
5547a83 to
de9359a
Compare
Contributor
Author
|
Thanks for the feedback and quick reviews! 🙏 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Upon upgrading from Rails 5.1.6.2 to 5.2.3, deploys to staging started failing
with the following exception during the readiness probe (curl server:port/z/ping)
The backtrace for the error was originally obfuscated by Rails due to backtrace
silencers. To reveal the full backtrace we added the following lines to an
initializer
The exception was coming from here:
https://github.com/rails/rails/blob/v5.2.3/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb#L423
Rails has modified the way it handles database connections in
Full backtrace
Replication:
I could reproduce the behaviour with config.cache_classes = config.eager_load = true set in my config/environment/development.rb and executing rails runner "Process.fork { ActiveRecord::Base.connected? }"
This patch adds supports to this gem for Rails 5.2.3 with the following