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

@97jaz 97jaz 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 added this to the 6.0.0 milestone Aug 16, 2019
@97jaz
Copy link
Contributor Author

97jaz 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 thread-local-prepared-statements branch from f9e0c47 to 95f87cc Compare August 16, 2019 01:54
@97jaz
Copy link
Contributor Author

97jaz 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.

Copy link
Member

@matthewd matthewd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

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

def prepared_statements # :nodoc:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was previously an attr_reader without documentation.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah! Ok, got it.

@prepared_statement_status.value
end

def prepared_statements=(prepared_statements) # :nodoc:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. Changed.

@97jaz 97jaz force-pushed the thread-local-prepared-statements branch from 95f87cc to 27cd28e Compare August 16, 2019 03:55
@@ -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)
Copy link
Contributor Author

@97jaz 97jaz Aug 16, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. 👍🏻

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 thread-local-prepared-statements branch from 27cd28e to b11ecf0 Compare August 16, 2019 06:00
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 rails#36763
@97jaz 97jaz force-pushed the thread-local-prepared-statements branch from b11ecf0 to d553213 Compare August 16, 2019 06:11
@rafaelfranca rafaelfranca merged commit 5e2d3d1 into rails:master Aug 16, 2019
rafaelfranca added a commit that referenced this pull request Aug 16, 2019
Make prepared statement status thread and instance-specific
@SamSaffron
Copy link
Contributor

SamSaffron 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
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
Copy link
Member

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
Copy link
Contributor

@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 has been minimized.

SamSaffron added a commit to SamSaffron/rails that referenced this pull request Sep 15, 2019
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 pushed a commit that referenced this pull request Sep 15, 2019
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 pushed a commit that referenced this pull request Sep 16, 2019
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
Development

Successfully merging this pull request may close these issues.

Rails 6 RC2 regression: (postgres) prepared statement improperly parameterized
5 participants