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 prepared statement status thread and instance-specific #36949

Merged
merged 1 commit into from Aug 16, 2019

Conversation

@97jaz
Copy link
Contributor

commented Aug 16, 2019

This fixes a race condition in system tests where prepared
statements can be incorrectly parameterized when multiple
threads observe the mutation of the @prepared_statements
instance variable on the connection.

Fixes #36763

@rafaelfranca rafaelfranca requested a review from matthewd Aug 16, 2019

@rafaelfranca rafaelfranca added this to the 6.0.0 milestone Aug 16, 2019

@97jaz

This comment has been minimized.

Copy link
Contributor Author

commented Aug 16, 2019

Come to think of it, this isn't memory-safe. The Concurrent::Map isn't a weak map (as far as I know), so it will hold onto the threads even after they're dead.

I think there's a better tool in this tool-box...

@97jaz 97jaz force-pushed the 97jaz:thread-local-prepared-statements branch from f9e0c47 to 95f87cc Aug 16, 2019

@97jaz

This comment has been minimized.

Copy link
Contributor Author

commented Aug 16, 2019

@rafaelfranca I just changed the implementation to use Concurrent::ThreadLocalVar, instead of Concurrent::Map. Otherwise, it's the same. This should be memory-safe.

@matthewd
Copy link
Member

left a comment

Looks great!

@@ -174,6 +175,14 @@ def schema_migration # :nodoc:
end
end

def prepared_statements # :nodoc:

This comment has been minimized.

Copy link
@matthewd

matthewd Aug 16, 2019

Member

Should this be documented? Sounds like the attribute was before.

This comment has been minimized.

Copy link
@97jaz

97jaz Aug 16, 2019

Author Contributor

It was previously an attr_reader without documentation.

This comment has been minimized.

Copy link
@matthewd

matthewd Aug 16, 2019

Member

Right, so it's listed under Attributes in https://api.rubyonrails.org/classes/ActiveRecord/ConnectionAdapters/AbstractAdapter.html. :nodoc: means "this is explicitly undocumented/unsupported, and not part of our promised API". To be clear, I'm not suggesting we need to write words.. just that it shouldn't disappear completely.

Suggested change
def prepared_statements # :nodoc:
def prepared_statements

This comment has been minimized.

Copy link
@97jaz

97jaz Aug 16, 2019

Author Contributor

Ah! Ok, got it.

@prepared_statement_status.value
end

def prepared_statements=(prepared_statements) # :nodoc:

This comment has been minimized.

Copy link
@matthewd

matthewd Aug 16, 2019

Member

I think this can (should) be private -- no external callers

This comment has been minimized.

Copy link
@97jaz

97jaz Aug 16, 2019

Author Contributor

Good point. Changed.

@97jaz 97jaz force-pushed the 97jaz:thread-local-prepared-statements branch from 95f87cc to 27cd28e Aug 16, 2019

@@ -40,7 +40,7 @@ class Mysql2Adapter < AbstractMysqlAdapter

def initialize(connection, logger, connection_options, config)
super
@prepared_statements = false unless config.key?(:prepared_statements)
self.prepared_statements = false unless config.key?(:prepared_statements)

This comment has been minimized.

Copy link
@97jaz

97jaz Aug 16, 2019

Author Contributor

This isn't quite right. It will set the value for the current thread, but the default value will already have been set in the super call. So other threads won't necessarily behave correctly.

The simplest fix is just to overwrite the instance variable here with a new ThreadLocalVar that has the correct default. The alternative would be to communicate the default to the superclass implementation, but that seems like a pain.

Or set it before the super call and use ||= in the superclass constructor.

This comment has been minimized.

Copy link
@matthewd

matthewd Aug 16, 2019

Member

Ah, good catch. Something like super(connection, logger, connection_options, config.reverse_merge(prepared_statements: false)) doesn't seem too terrible, but I'm happy with overwriting the instance variable too. 👍🏻

This comment has been minimized.

Copy link
@97jaz

97jaz Aug 16, 2019

Author Contributor

Oh, well if we're allowed to pass a modified config, then I like your idea better.

This comment has been minimized.

Copy link
@97jaz

97jaz Aug 16, 2019

Author Contributor

Heh, once I change this and fix the mysql behavior, the test I added fails on mysql, because the test assumes that prepared statements are enabled by default. So I'm changing the test to be conditional.

@97jaz 97jaz force-pushed the 97jaz:thread-local-prepared-statements branch from 27cd28e to b11ecf0 Aug 16, 2019

Make prepared statement status thread and instance-specific
This fixes a race condition in system tests where prepared
statements can be incorrectly parameterized when multiple
threads observe the mutation of the @prepared_statements
instance variable on the connection.

Fixes #36763

@97jaz 97jaz force-pushed the 97jaz:thread-local-prepared-statements branch from b11ecf0 to d553213 Aug 16, 2019

@rafaelfranca rafaelfranca merged commit 5e2d3d1 into rails:master Aug 16, 2019

2 checks passed

buildkite/rails Build #63104 passed (8 minutes, 3 seconds)
Details
codeclimate All good!
Details
rafaelfranca added a commit that referenced this pull request Aug 16, 2019
Merge pull request #36949 from 97jaz/thread-local-prepared-statements
Make prepared statement status thread and instance-specific
@SamSaffron

This comment has been minimized.

Copy link
Contributor

commented Sep 12, 2019

@rafaelfranca / @matthewd

There is a slight unintended consequence to this change I will monkey patch out of Discourse.

The implementation of RubyThreadLocalVar wacks a finalizer on the object, the finalizer spins a thread.

https://github.com//blob/bbeacbcebf72668ed04df0738df7e3a654f7c177/lib/concurrent/atomic/ruby_thread_local_var.rb#L101-L111

We use no prepared statements, but are now paying the penalty of spinning up a thread every time a connection is closed.

This has this impact on my graphs for the big multisites:

https://meta.discourse.org/t/upgrading-discourse-to-rails-6/128004/11?u=sam

Since once false this thing can never be turned true I will just implement an Immutable ThreadLocalVar class here for the cases where prepared statements are false. (bind anything but current value and it raises.

index 91d5d08121..970a1f9e8f 100644
--- a/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb
+++ b/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb
@@ -114,6 +114,18 @@ def self.quoted_table_names # :nodoc:
         @quoted_table_names ||= {}
       end
 
+      class StaticThreadLocalVar
+        attr_reader :value
+
+        def initialize(value)
+          @value = value
+        end
+
+        def bind(value)
+          raise "attempting to change immutable local var" if value != @value
+          yield if block_given?
+        end
+      end
+
       def initialize(connection, logger = nil, config = {}) # :nodoc:
         super()
 
@@ -132,7 +144,7 @@ def initialize(connection, logger = nil, config = {}) # :nodoc:
           @prepared_statement_status = Concurrent::ThreadLocalVar.new(true)
           @visitor.extend(DetermineIfPreparableVisitor)
         else
-          @prepared_statement_status = Concurrent::ThreadLocalVar.new(false)
+          @prepared_statement_status = StaticThreadLocalVar.new(false)
         end
 
         @advisory_locks_enabled = self.class.type_cast_config_to_boolean(

Thoughts? Should we patch this in Rails?

SamSaffron added a commit to discourse/discourse that referenced this pull request Sep 12, 2019
PERF: avoid spinning a thread each time we close a connection
This is a temporary workaround for the issue in rails/rails#36949

Discussing a proper fix in Rails with the Rails team.

Prior to this fix we were spinning up a thread every time we closed a connection
to the db.
@rafaelfranca

This comment has been minimized.

Copy link
Member

commented Sep 12, 2019

That works for me. Can you open a PR? Don't forget to mention me in the PR because I'm not watching this repo anymore.

@SamSaffron

This comment has been minimized.

Copy link
Contributor

commented Sep 13, 2019

@rafaelfranca awesome, lets first give this a shot, but if it fails we can pull this into rails.

ruby-concurrency/concurrent-ruby#823

@talpava

This comment was marked as spam.

Copy link

commented Sep 13, 2019

SamSaffron added a commit to SamSaffron/rails that referenced this pull request Sep 14, 2019
Avoid expensive tracking objects for prepared statements
Per rails#36949 we introduce a race condition fix for rails#36763

This refines the fix to avoid using `Concurrent::ThreadLocalVar`

The implementation there is rather expensive, culminating in
a finalizer per object that spins off a thread to do
cleanup work.

None of this expense is needed as we can simply implement
the desired behavior using Ruby primitives.
SamSaffron added a commit to SamSaffron/rails that referenced this pull request Sep 15, 2019
Avoid expensive tracking objects for prepared statements
Per rails#36949 we introduce a race condition fix for rails#36763

This refines the fix to avoid using Concurrent::ThreadLocalVar

The implementation in the concurrent lib is rather expensive, culminating in
a finalizer per object that spins off a thread to do cleanup work.

None of this expense is needed as we can simply implement
the desired behavior using Ruby primitives. Additionally this moves to a Fiber bound implementation vs a thread bound implementation, something that is not desired for this particular usage.
SamSaffron added a commit to SamSaffron/rails that referenced this pull request Sep 15, 2019
Avoid expensive tracking objects for prepared statements
Per rails#36949 we introduce a race condition fix for rails#36763

This refines the fix to avoid using Concurrent::ThreadLocalVar

The implementation in the concurrent lib is rather expensive, culminating in
a finalizer per object that spins off a thread to do cleanup work.

None of this expense is needed as we can simply implement
the desired behavior using Ruby primitives. Additionally this moves to a Fiber bound implementation vs a thread bound implementation, something that is not desired for this particular usage.
kaspth added a commit that referenced this pull request Sep 15, 2019
Avoid expensive tracking objects for prepared statements
Per #36949 we introduce a race condition fix for #36763

This refines the fix to avoid using Concurrent::ThreadLocalVar

The implementation in the concurrent lib is rather expensive, culminating in
a finalizer per object that spins off a thread to do cleanup work.

None of this expense is needed as we can simply implement
the desired behavior using Ruby primitives. Additionally this moves to a Fiber bound implementation vs a thread bound implementation, something that is not desired for this particular usage.
rafaelfranca added a commit that referenced this pull request Sep 16, 2019
Avoid expensive tracking objects for prepared statements
Per #36949 we introduce a race condition fix for #36763

This refines the fix to avoid using Concurrent::ThreadLocalVar

The implementation in the concurrent lib is rather expensive, culminating in
a finalizer per object that spins off a thread to do cleanup work.

None of this expense is needed as we can simply implement
the desired behavior using Ruby primitives. Additionally this moves to a Fiber bound implementation vs a thread bound implementation, something that is not desired for this particular usage.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.