Avoid race condition with unprepared_statement

[97jaz]

In system tests, a single database connection is shared among all
the server threads. A call to unprepared_statement temporarily
modifies an instance variable on the connection object, which is
then visible to other concurrently running threads. This leads to
a situation where prepared statements may end up with the wrong
binds.

Addresses #36763

[97jaz]

@matthewd This does appear to fix the problem (at least with the racy test app).

I'm not sure what kind of tests would be useful here.

[97jaz]

Given that #insert, #update, and #delete (in DatabaseStatements) all call #to_sql_and_binds, which potentially inspects prepared_statements more than once (and doesn't go through #select_all), I suppose #to_sql_and_binds should also be locked.

Done

[rafaelfranca]

I think it is better to change the prepared_statement instance variable to be a thread local variable. Can you change that?

[97jaz]

@rafaelfranca Sure

[97jaz]

@rafaelfranca Actually, making this (simply) thread-local is a breaking change. prepared_statements is a DB config option, so it could be set separately for different databases that are used in the same thread.

Concretely, test/cases/adapters/postgresql/prepared_statements_disabled_test.rb:20 breaks if we make this a thread-local variable. The test itself could be made to pass if the establishment of a new connection were to mutate the thread-local, but that seems pretty clearly wrong.

I think prepared_statements will need to be a per-thread, per-instance value, like @thread_cached_conns in ConnectionPool.

[97jaz]

Sorry -- I should clarify: that test breaks when it's run as part of the suite. It passes when run individually, precisely because in the latter case, the preprared_statements: false config is used for the first connection on the thread. But when it's run as part of the suite, that's not the case.

[rafaelfranca]

Right, we could have a db config option, but the value inside the instance of the connection should only be set to the current thread, not to all threads. Something like this should work:

def prepared_statements
  thread_variable?("prepared_statement") ? Thread.currernt.thread_variable_get("prepared_statement") : @prepared_statementes
end

def prepared_statements=(value)
  Thread.currernt.thread_variable_set("prepared_statement", value)
end

[97jaz]

That's in fact what I did (or something equivalent):

      def prepared_statements # :nodoc:
        Thread.current.fetch(:prepared_statements, @default_prepared_statements)
      end

      def prepared_statements=(prepared_statements) # :nodoc:
        Thread.current[:prepared_statements] = prepared_statements
      end

That's breaking behavior.

[97jaz]

The problem is that once the thread-local is set, the setting now applies to all connections in the current thread, not just the connection it was called on.

[Edit] That means, for example, that it would apply across different connection pools that connect to different databases, regardless of what was in their connection configuration.

Possibly, this value could be in a thread-keyed map in the pool, instead of the individual connection. And the connection could delegate to the pool. That's assuming that there's never a case where two different connections from the same pool are used in the same thread with different prepared statement statuses.

[97jaz]

Closing in favor of #36949