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

ActiveRecord connection re-establishing not consistent with documentation #2253

Closed
farnoy opened this issue Feb 20, 2020 · 3 comments
Closed

Comments

@farnoy
Copy link

farnoy commented Feb 20, 2020

Issue report

The documentation states this:

Am I responsible for reestablishing database connections after the preloader has forked a child process?
It depends. Passenger automatically reestablishes the ActiveRecord primary database connection. The vast majority of Rails only uses ActiveRecord (not any other database libraries), and only with a single database connection.
If your application uses any database libraries besides ActiveRecord, OR you use ActiveRecord with more than one database connection, then you are responsible for reestablishing all connections after forking. Use the smart spawning hooks.

But it's not consistent with the code that's supposed to do this:

# If we were forked from a preloader process then clear or
# re-establish ActiveRecord database connections. This prevents
# child processes from concurrently accessing the same
# database connection handles.
if forked && defined?(ActiveRecord::Base)
if ActiveRecord::Base.respond_to?(:clear_all_connections!)
ActiveRecord::Base.clear_all_connections!
elsif ActiveRecord::Base.respond_to?(:clear_active_connections!)
ActiveRecord::Base.clear_active_connections!
elsif ActiveRecord::Base.respond_to?(:connected?) &&
ActiveRecord::Base.connected?
ActiveRecord::Base.establish_connection
end
end

For recent ActiveRecord versions, (these clear_*_connections! methods were added in AR 2.2.1) it only disconnects it. If using AR older than 2.2.1, I assume it re-establishes the primary connection, which aligns with the docs.

I tried to verify this by doing:

if defined?(PhusionPassenger)
  PhusionPassenger.on_event(:starting_worker_process) do |forked|
    if forked
      puts "#{Process.pid} - #{ActiveRecord::Base.connected?}"
    end
  end
end

And indeed I only see false values there.

So it's not consistent with the documentation, as far as I can tell. Does it matter, though? I would assume that it does, because doing this setup in before_handling_requests prevents this not-fully-ready worker from accepting requests, which gives other workers a chance to do so. And other workers are going to start with an established connection, and thus be able to provide lower latencies. Does it make sense? My understanding is limited, so please correct me if I'm wrong.

Question 1: What is the problem?

  • What is the expected behavior?
    Spawned workers establish a DB connection before accepting the first request.
  • What is the actual behavior?
    They establish the first connection lazily, during the processing of their first request. This not only adds latency to the first SQL query executed, but also on the ActiveModel side, lots of attribute initialization is done when the schema is first discovered.
  • How can we reproduce it? Please try to provide a sample application (or Virtual Machine) demonstrating the issue. Otherwise, if we can't reproduce it, we might have to ask you a number of followup questions or run certain commands to try and figure out the problem.
    Any AR Rails app should do, I run it in smart spawning mode in standalone mode, but I believe it also happens at least in nginx deployments, probably in other configurations too

Question 2: Passenger version and integration mode:

  • 6.0.4 standalone and nginx

Question 3: OS or Linux distro, platform (including version):

  • Linux 5.5.4 Arch

Question 4: Passenger installation method:

Your answer:
[x] RubyGems + Gemfile
[ ] RubyGems, no Gemfile
[ ] Phusion APT repo
[ ] Phusion YUM repo
[ ] OS X Homebrew
[ ] source tarball
[ ] Other, please specify:

Question 5: Your app's programming language (including any version managers) and framework (including versions):

  • Ruby 2.6.5, Rails 5.2.2 and Rails 6 (tested on both)

Question 6: Are you using a PaaS and/or containerization? If so which one?

  • no

Question 7: Anything else about your setup that we should know?

no

I'm not sure what the best practices are around forking, but it seems to me like this strategy should cover all the bases:

if defined?(PhusionPassenger)
  PhusionPassenger.on_event(:starting_worker_process) do |forked|
    if forked
      # make sure the connection is ready
      ActiveRecord::Base.establish_connection
      # load all the code...
      Rails.application.eager_load!
      # ... so that we can initialize attributes on all models
      ApplicationRecord.descendants.each do |model|
        model.define_attribute_methods
      end
    end
  end
end
@CamJN
Copy link
Contributor

CamJN commented Apr 30, 2020

Going back through the source code of activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb it looks like these methods have always worked this way, and that we just wound up relying on the automatically recreated connection.

What are your thoughts on the following for Passenger's side of the equation?

      # If we were forked from a preloader process then clear or
      # re-establish ActiveRecord database connections. This prevents
      # child processes from concurrently accessing the same
      # database connection handles.
      if forked && defined?(ActiveRecord::Base)
        if ActiveRecord::Base.respond_to?(:clear_all_connections!)
          ActiveRecord::Base.clear_all_connections!
        elsif ActiveRecord::Base.respond_to?(:clear_active_connections!)
          ActiveRecord::Base.clear_active_connections!
        end
        ActiveRecord::Base.establish_connection
      end

The eager loading and model definition will have to be done app-side as this code is also used for development environments.

@felixbuenemann
Copy link

felixbuenemann commented May 31, 2020

Since this is supposedly fixed by 6.0.5, shouldn’t the issue be closed?

@CamJN
Copy link
Contributor

CamJN commented May 31, 2020

Yup

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

No branches or pull requests

4 participants