Fix ActiveSupport::CurrentAttributes attributes to be thread local.

[tgxworld]

Summary

Thread#[] and Thread#[]= are getting or setting a fibre-local variable when we want a thread local variable in this case. Instead we need to use Thread#thread_variable_get or Thread#thread_variable_set.

From Ruby API doc:

thr[sym] → obj or nilclick to toggle source
Attribute Reference—Returns the value of a fiber-local variable (current thread's root fiber if not explicitly inside a Fiber), using either a symbol or a string name. If the specified variable does not exist, returns nil.

thr[sym] = obj → objclick to toggle source
Attribute Assignment—Sets or creates the value of a fiber-local variable, using either a symbol or a string.
See also #[].
For thread-local variables, please see thread_variable_set and thread_variable_get.

[tgxworld]

cc @rafaelfranca

@ioquatix I think this will affect Falcon which uses one Fibre per request and there can be multiple Fibres in a given Thread. However, CurrentAttributes is meant to be local to the current thread and creating a new Fibre now within a Thread will actually create a new instance of CurrentAttributes.

[rafaelfranca]

Thank you for the pull request but we want fiber local. There are many other PRs doing the same and we decided to go with fiber locals.

[rafaelfranca]

Ah, but I just remember that we discussed on making this configurable. Can you change the PR to make it configurable?

[rafaelfranca]

There is a huge risk of setting it in fiber locals and nesting blocks as you can see in this gist https://gist.github.com/matthewd/137de9cc7ad525e0ab650967fe31236c. This is why we are not making the default.

[tgxworld]

Ah, but I just remember that we discussed on making this configurable. Can you change the PR to make it configurable?

Sure thing. I guess the default should be fiber local variable with the option of making it thread local?

Looks like there is actually a test case to ensure it is using a fiber local variable. Got to remember how to run the tests locally again.

[ioquatix]

Thanks for pinging me.

My advice would be to avoid using true thread local variables unless it's actually equivalent to global state. In a multi-fiber system running on a single thread, thread local storage is no different from globals.

As you can imagine, having globals storing things which are essentially tied to a specific sequential body of code can cause problems, especially if they are interleaved (like light weight fibers with non-blocking I/O).

It's highly likely that this feature as well as some kind of Guilds implementation is coming to Ruby 3. Therefore, to take advantage of it, there are definitely things you want to avoid, and I'd say, using thread locals when you probably mean fiber locals, is a big one which will cause a lot of migration pain.

[ioquatix]

Just since I'm here, one thing that you could help with is doing an audit of all places in rails where actual thread local storage IS used. It would be great to have a summary so we can figure out how and what needs to be addressed to improve compatibility. It would be great to hit the ground running with Ruby 3 and light weight concurrency.

Ruby 3 will be here, all things going well, in about 8 months. You could set up tests against ruby-head.

[kaspth]

Great points @ioquatix. We’ve got tests running against ruby head on our builds already, so we’re good there. What’s the latest on guilds? I don’t think I’ve seen an API or something that’s been merged yet?

[ioquatix]

The problem is not running on ruby-head itself.

It's that you need to audit places where you are using thread local storage.

Thread local storage can cause problems for light weight threads. In a way, it's not so different from a global variable shared between threads, a thread local is shared between fibers has roughly the same exposure to race conditions.

Guilds are like separate processes. Not much can be shared between them. So anything that is providing shared resources e.g. the activerecord connection pool may need to be checked.

[ioquatix]

Finally, dynamic code modification, e.g. how activerecord modifies classes when loading the database schema, may pose an issue. I'm not sure yet what the exposure is, but I assume at least some code is shared between guilds.

[kaspth]

Yeah, I’m with you on all that. The test builds were in reply to:

Ruby 3 will be here, all things going well, in about 8 months. You could set up tests against ruby-head.

[ioquatix]

Understood :)

[tgxworld]

@ioquatix I did a quick search and it only seems like we're using thread local variable in the following places at the moment:

• rails/activerecord/lib/active_record/core.rb

  Lines 140 to 154 in bfbf696

  	def self.connection_handler
  	Thread.current.thread_variable_get(:ar_connection_handler) || default_connection_handler
  	end
  	def self.connection_handler=(handler)
  	Thread.current.thread_variable_set(:ar_connection_handler, handler)
  	end
  	def self.current_pool_key
  	Thread.current.thread_variable_get(:ar_pool_key) || default_pool_key
  	end
  	def self.current_pool_key=(pool_key)
  	Thread.current.thread_variable_set(:ar_pool_key, pool_key)
  	end

• rails/activerecord/lib/active_record/connection_adapters/sqlite3/database_statements.rb

  Line 81 in bfbf696

  Thread.current.thread_variable_set("read_uncommitted", @connection.get_first_value("PRAGMA read_uncommitted"))

[tgxworld]

Closing this as the usage of thread local variable isn't something which we want to encourage.

[ioquatix]

@tgxworld Thanks for checking.

#37070 FYI.

Also just saw #38930

You also need to check for any hash that uses Thread.current as a key.