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

Make ARHP's connection switching thread safe #63

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Commits on Jun 15, 2020

  1. Make database switching thread-safe

    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 committed Jun 15, 2020
    Configuration menu
    Copy the full SHA
    a04a11c View commit details
    Browse the repository at this point in the history

Commits on Jun 25, 2020

  1. Configuration menu
    Copy the full SHA
    3b3572a View commit details
    Browse the repository at this point in the history