Make ARHP's connection switching thread safe #63

zdennis · 2020-06-10T15:22:56Z

What does this PR do?

This PR makes ARHP database switching thread-safe by:

updating access to the the host pool database in a thread local manner.
always switch the database (and let mysql handle it being a no-op when it doesn't need to switch)
augment the connection adapter's internal @config to also work in a thread local manner

Why is this helpful?

This allows ARHP to be used in Rails with concurrency turned on. Specifically for my use-case, this lets me set config.allow_concurrency=true in config/environments/test.rb for a feature/integration suite. Without this change concurrency has to be disabled and this makes request processing extremely slow.

Trade-offs

No longer caching the current database

Turning thread-safety on means that caching the current database no longer makes sense since the cached value may be stale based on other threads that have interacted with the database. Rather than querying the database to see what the current database is (which would work) the code now always issues the select_db call and lets MySQL handle responding to things. MySQL seems to treat operations that don't require switching the current database as no-ops and these are very fast.

Always calling `clear_cache!`

clear_cache! is always called now since the code no longer keeps track of the current database (and lets mysql manage that). This essentially kills any prepared statement caching that occurred, but this was already happening to a large degree when an application would switch databases.

zdennis · 2020-06-10T19:44:14Z

lib/active_record_host_pool/connection_adapter_mixin.rb

@@ -3,13 +3,54 @@
 require "active_record/connection_adapters/mysql2_adapter"

 module ActiveRecordHostPool
+  module PerThreadHashAccess


This module only exists to preserve this line farther below:

@config[:database] = _host_pool_current_database if ActiveRecord::VERSION::MAJOR >= 5

zdennis · 2020-06-11T14:43:02Z

lib/active_record_host_pool/connection_adapter_mixin.rb

-        attr_reader(:_host_pool_current_database)
+        def _host_pool_current_database
+          _ahrp_storage[:_host_pool_current_database]
+        end


Any reason to not move the _host_pool_current_database accessor methods out of the class_eval and into the main module body?

zdennis · 2020-06-12T18:57:59Z

I'm converting this back to a draft. Using this branch locally for testing has been working well but when running this against an app on TravisCI there are some failures that I need to identify if they are related to this patch.

zdennis · 2020-06-15T19:37:20Z

lib/active_record_host_pool/connection_adapter_mixin.rb

-          @_host_pool_current_database = database
-          @config[:database] = _host_pool_current_database if ActiveRecord::VERSION::MAJOR >= 5
-        end
-


I moved _host_pool_current_database and _host_pool_current_database= methods down as methods on the module rather than added on through class_eval.

so that AHRP can be used in a multi-threaded environment. 1. Make _host_pool_current_database accessors thread local 2. Make the connection adapter's internal @config thread-safe with PerThreadHashAccess module inclusion 3. Make a ThreadSafe module which layers methods and overrides on top of DatabaseSwitch base functionality 4. No longer cache current database (via _cached_current_database) IMPLICATIONS OF NO LONGER CACHING THE CURRENT DATABASE In a multithreaded world it doesn't make sense to cache the current database since it's impossible for the code to know if we have switched away and back again in another thread. This means that AHRP will now always change the database for every statement. This also means that clear_cache! will always be called. Performance hit? The cost of issuing a #select_db for each query seems negligible (sub millisecond). If MySQL already has the current database selected then the switch is essentially a no-op and cost is really the round trip across the network. Locally, the average across 10,000 reads was a fraction of a millisecond. The biggest cost may come from calling #clear_cache! on every query. However, this appears to be the prepared statements cache and not the query (results) cache (which is cleared via a separate method: #clear_query_cache). Separate ThreadSafe module? Not knowing how big of a shift this change will be considered I thought it might be easier to make it possible to update the code so projects could opt-in to thread safety rather than making it the global default. Currently, this change-set assumes it will be the default and includes the ThreadSafe module automatically. REMOVING THE VERSION CHECK AROUND UPDATING THE @config[:database] The internal @config[:database] will now always be set rather than just on ActiveRecord >= 5. This ensures that each version of ActiveRecord is updated to do the right thing with respect to threads. Removing the conditional doesn't break anything in Rails 4.2 and it appears that it was only added to fix something broken in ActiveRecord 5+. Since the code continues to work correctly across Rails 4.2, 5.x, and 6.x it doesn't make sense to leave it in. Removing it also makes testing thread-safe behavior simpler since all versions can be tested in the same way.

zdennis · 2020-06-15T20:29:19Z

lib/active_record_host_pool/connection_adapter_mixin.rb

@@ -118,3 +181,4 @@ def establish_connection(owner, spec)
 # rubocop:enable Lint/DuplicateMethods

 ActiveRecord::ConnectionAdapters::Mysql2Adapter.include(ActiveRecordHostPool::DatabaseSwitch)
+ActiveRecord::ConnectionAdapters::Mysql2Adapter.include(ActiveRecordHostPool::ThreadSafe)


I made ThreadSafe a separate module in case this functionality should be considered opt-in.

If this is made opt-in it may make sense to separate test thread-safety separate from non-thread-safety (to run separately when we modify the Mysql2Adapter with thread-safety from running the original non-thread-safe code), but I did not tackle the in this PR since it may not be necessary.

zdennis · 2020-06-24T21:17:52Z

It just hit me that this works great for the database switching to be thread-safe, but it doesn't do anything for the connection pool. I'll see about writing a test for that and see what shakes.

…hreads.

zdennis · 2020-06-25T18:16:38Z

I've pushed up 3b3572a which exercises the connection pool by creating a number of records across threads.

KJTsanaktsidis · 2022-06-29T01:23:56Z

I spent the morning looking at this with @mervync and.... I think it should work??

KJTsanaktsidis

I want to try this in Classic.

HeyNonster · 2022-06-30T07:18:37Z

lib/active_record_host_pool/connection_adapter_mixin.rb

      disconnect_without_host_pooling!
    end

+    def _host_pool_current_database
+      _ahrp_storage[:_host_pool_current_database]


Suggested change

_ahrp_storage[:_host_pool_current_database]

_arhp_storage[:_host_pool_current_database]

I also make this mistake constantly when working in this repo but ahrp should be renamed arhp

HeyNonster · 2022-06-30T07:19:26Z

lib/active_record_host_pool/connection_adapter_mixin.rb

+    end
+
+    def _host_pool_current_database=(database)
+      _ahrp_storage[:_host_pool_current_database] = database


Suggested change

_ahrp_storage[:_host_pool_current_database] = database

_arhp_storage[:_host_pool_current_database] = database

etc

mervync · 2022-06-30T16:30:56Z

addendum: turns out this isn't production tested.

sircpl · 2022-06-30T18:18:25Z

lib/active_record_host_pool/connection_adapter_mixin.rb

+    def _ahrp_storage
+      @_ahrp_storage ||= {}
+    end
+
+    def _ahrp_fetch_database_to_switch_to
+      database_name = _host_pool_current_database
+      if database_name != _cached_current_database || connection.object_id != _cached_connection_object_id
+        database_name
+      end
+    end


Do we need these methods? They look like they're overridden by ThreadSafe

I guess because ThreadSafe is opt-in

zdennis changed the title ~~Make AHRP thread safe connection switching~~ Make AHRP's connection switching thread safe Jun 10, 2020

zdennis force-pushed the testdouble/thread-safe-connection-switching branch from 1aaaf27 to e68fc01 Compare June 10, 2020 19:35

zdennis marked this pull request as ready for review June 10, 2020 19:41

zdennis requested a review from bquorning as a code owner June 10, 2020 19:41

zdennis commented Jun 10, 2020

View reviewed changes

zdennis force-pushed the testdouble/thread-safe-connection-switching branch 2 times, most recently from 610fcb1 to e312b2a Compare June 11, 2020 14:41

zdennis commented Jun 11, 2020

View reviewed changes

zdennis force-pushed the testdouble/thread-safe-connection-switching branch 2 times, most recently from 20b41cf to 8ccf524 Compare June 12, 2020 14:00

zdennis marked this pull request as draft June 12, 2020 18:57

zdennis force-pushed the testdouble/thread-safe-connection-switching branch from 20283b8 to 3cfd562 Compare June 15, 2020 19:24

zdennis commented Jun 15, 2020

View reviewed changes

zdennis force-pushed the testdouble/thread-safe-connection-switching branch from 3cfd562 to a04a11c Compare June 15, 2020 20:03

zdennis commented Jun 15, 2020

View reviewed changes

zdennis marked this pull request as ready for review June 15, 2020 20:47

Adding a test that creates records across two models across various t…

3b3572a

…hreads.

mervync mentioned this pull request Feb 4, 2021

Thread safety fixes and tests #66

Closed

KJTsanaktsidis approved these changes Jun 29, 2022

View reviewed changes

HeyNonster requested changes Jun 30, 2022

View reviewed changes

sircpl reviewed Jun 30, 2022

View reviewed changes

bquorning changed the title ~~Make AHRP's connection switching thread safe~~ Make ARHP's connection switching thread safe Jun 30, 2022

mriddle added the teams/ruby-core Ensures ruby-core team are notified of activity over in #ruby-core-team-github label Jul 29, 2022

HeyNonster requested a review from a team as a code owner January 20, 2023 13:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make ARHP's connection switching thread safe #63

Make ARHP's connection switching thread safe #63

zdennis commented Jun 10, 2020 •

edited by bquorning

zdennis Jun 10, 2020

zdennis Jun 11, 2020

zdennis commented Jun 12, 2020

zdennis Jun 15, 2020

zdennis Jun 15, 2020

zdennis commented Jun 24, 2020

zdennis commented Jun 25, 2020

KJTsanaktsidis commented Jun 29, 2022

KJTsanaktsidis left a comment

HeyNonster Jun 30, 2022

HeyNonster Jun 30, 2022

mervync commented Jun 30, 2022

sircpl Jun 30, 2022

sircpl Jun 30, 2022

	_ahrp_storage[:_host_pool_current_database]
	_arhp_storage[:_host_pool_current_database]

Make ARHP's connection switching thread safe #63

Are you sure you want to change the base?

Make ARHP's connection switching thread safe #63

Conversation

zdennis commented Jun 10, 2020 • edited by bquorning

What does this PR do?

Why is this helpful?

Trade-offs

No longer caching the current database

Always calling clear_cache!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zdennis commented Jun 12, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zdennis commented Jun 24, 2020

zdennis commented Jun 25, 2020

KJTsanaktsidis commented Jun 29, 2022

KJTsanaktsidis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mervync commented Jun 30, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zdennis commented Jun 10, 2020 •

edited by bquorning

Always calling `clear_cache!`