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

Improve AR connection fork safety #31173

Merged
merged 1 commit into from Nov 25, 2017
Merged

Conversation

@matthewd
Copy link
Member

@matthewd matthewd commented Nov 17, 2017

Use whatever adapter-provided means we have available to ensure forked children don't send quit/shutdown/goodbye messages to the server on connections that belonged to their parent.

  • mysql2 provides a method specifically for this purpose: brianmario/mysql2#684
  • pg is not quite so helpful, but it does expose a socket_io, which is close enough.
  • I think sqlite3 is immune to this problem just because there is no server to confuse.

I believe this should eliminate the need for advice like this, along with much of what's currently described in #29807.

yahonda added a commit to yahonda/oracle-enhanced that referenced this pull request Nov 17, 2017
activerecord/test/cases/connection_adapters/connection_handler_test.rb Outdated
assert_not_equal object_id, Marshal.load(rd.read)
rd.close

assert_equal 5, ActiveRecord::Base.connection.select_value("SELECT 2 + 3")

This comment has been minimized.

@yahonda

yahonda Nov 17, 2017
Contributor

Hi, I'm testing this pull request with Oracle enhanced adapter to see how to implement discard! method. As of right now what I can say is Oracle database always need from for every select statement.

ActiveRecord::StatementInvalid: OCIError: ORA-00923: FROM keyword not found where expected: SELECT 2 + 3

Would you consider this so that all bundled + Oracle enhanced adapter can support this test?

This comment has been minimized.

@matthewd

matthewd Nov 18, 2017
Author Member

Sure! I'll look through existing tests for a better idea of a query to run.

I initially used #active?, but that just returned false when the test failed (i.e., without the corresponding change), and I wanted to see the actual "connection is broken / server has gone away" exception.

@jeremy
jeremy approved these changes Nov 17, 2017
Copy link
Member

@jeremy jeremy left a comment

Nice 👍

pools.values.compact.each(&:discard!) unless pid == Process.pid
end
end

This comment has been minimized.

@jeremy

jeremy Nov 17, 2017
Member

:nodoc: these?

Use whatever adapter-provided means we have available to ensure forked
children don't send quit/shutdown/goodbye messages to the server on
connections that belonged to their parent.
@matthewd matthewd force-pushed the matthewd:connection-fork-safety branch to f32cff5 Nov 18, 2017
@matthewd matthewd merged commit 3313912 into rails:master Nov 25, 2017
2 checks passed
2 checks passed
codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@matthewd matthewd deleted the matthewd:connection-fork-safety branch Nov 25, 2017
@kaspth
Copy link
Member

@kaspth kaspth commented Nov 25, 2017

@matthewd nice! I guess we should remove

# If you are preloading your application and using Active Record, it's
# recommended that you close any connections to the database before workers
# are forked to prevent connection leakage.
#
# before_fork do
# ActiveRecord::Base.connection_pool.disconnect! if defined?(ActiveRecord)
# end
# The code in the `on_worker_boot` will be called if you are using
# clustered mode by specifying a number of `workers`. After each worker
# process is booted, this block will be run. If you are using the `preload_app!`
# option, you will want to use this block to reconnect to any threads
# or connections that may have been created at application boot, as Ruby
# cannot share connections between processes.
#
# on_worker_boot do
# ActiveRecord::Base.establish_connection if defined?(ActiveRecord)
# end
#
from the default Puma template.

yahonda added a commit to yahonda/oracle-enhanced that referenced this pull request Nov 27, 2017
yahonda added a commit to yahonda/oracle-enhanced that referenced this pull request Nov 27, 2017
yahonda added a commit to yahonda/oracle-enhanced that referenced this pull request Nov 27, 2017
yahonda added a commit to yahonda/oracle-enhanced that referenced this pull request Nov 27, 2017
kamipo added a commit to kamipo/rails that referenced this pull request Nov 27, 2017
Since rails#31173, mysql2 adapter depends on `automatic_close` which is
introduced since mysql2 0.4.3. So the adapter with the mysql2 version
before doesn't work with fork now.

```
% ARCONN=mysql2 be ruby -w -Itest test/cases/connection_adapters/connection_handler_test.rb -n test_forked_child_doesnt_mangle_parent_connection
Using mysql2
Run options: -n test_forked_child_doesnt_mangle_parent_connection --seed 19988

/Users/kamipo/src/github.com/rails/rails/activerecord/lib/active_record/connection_adapters/mysql2_adapter.rb:108:in `discard!': undefined method `automatic_close=' for #<Mysql2::Client:0x00007fedaa91dfd0> (NoMethodError)
```

This drops mysql2 version less than 0.4.3 to guarantee fork safety.
kamipo added a commit that referenced this pull request Nov 27, 2017
Since #31173, mysql2 adapter depends on `automatic_close` which is
introduced since mysql2 0.4.3. So the adapter with the mysql2 version
before doesn't work with fork now.

```
% ARCONN=mysql2 be ruby -w -Itest test/cases/connection_adapters/connection_handler_test.rb -n test_forked_child_doesnt_mangle_parent_connection
Using mysql2
Run options: -n test_forked_child_doesnt_mangle_parent_connection --seed 19988

/Users/kamipo/src/github.com/rails/rails/activerecord/lib/active_record/connection_adapters/mysql2_adapter.rb:108:in `discard!': undefined method `automatic_close=' for #<Mysql2::Client:0x00007fedaa91dfd0> (NoMethodError)
```

This drops mysql2 version less than 0.4.3 to guarantee fork safety.
@siegy22
Copy link
Contributor

@siegy22 siegy22 commented May 27, 2019

@matthewd May this be the cause that https://github.com/QueueClassic/queue_classic isn't working correctly since rails 5.2?

See the line here: https://github.com/QueueClassic/queue_classic/blob/master/lib/queue_classic.rb#L55

Initially it works, but after a period of high(er) load, queue classic's initially passed ActiveRecord::Base.connection.raw_connection is closed, resulting in an error: PG::ConnectionBad: connection closed.

mriddle added a commit to zendesk/active_record_host_pool that referenced this pull request Aug 22, 2019
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 added a commit to zendesk/active_record_host_pool that referenced this pull request Aug 22, 2019
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 added a commit to zendesk/active_record_host_pool that referenced this pull request Aug 22, 2019
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 added a commit to zendesk/active_record_host_pool that referenced this pull request Aug 22, 2019
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 added a commit to zendesk/active_record_host_pool that referenced this pull request Aug 23, 2019
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 added a commit to zendesk/active_record_host_pool that referenced this pull request Aug 23, 2019
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 added a commit to zendesk/active_record_host_pool that referenced this pull request Aug 23, 2019
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.