From 540d2f41f6913cd6c5a71301540bfe1551c2acc5 Mon Sep 17 00:00:00 2001 From: Jean Boussier Date: Fri, 5 Nov 2021 10:30:36 +0100 Subject: [PATCH] Introduce ActiveSupport::IsolatedExecutionState for internal use Many places in Active Support and Rails in general use `Thread.current#[]` to store "request (or job) local data". This often cause problems with `Enumerator` because it runs in a different fiber. On the other hand, some places migrated to `Thread#thread_variable_get` which cause issues with fiber based servers (`falcon`). Based on this, I believe the isolation level should be an application configuration. For backward compatibility it could ship with `:fiber` isolation as a default but longer term :thread would make more sense as it would work fine for all deployment targets except falcon. Ref: https://github.com/rails/rails/pull/38905 Ref: https://github.com/rails/rails/pull/39428 Ref: https://github.com/rails/rails/pull/34495 (and possibly many others) --- activesupport/lib/active_support.rb | 5 +- .../lib/active_support/current_attributes.rb | 13 +---- .../isolated_execution_state.rb | 52 +++++++++++++++++ activesupport/lib/active_support/railtie.rb | 6 ++ activesupport/test/current_attributes_test.rb | 11 +++- .../test/isolated_execution_state_test.rb | 56 +++++++++++++++++++ guides/source/configuring.md | 6 ++ .../lib/rails/application/configuration.rb | 1 + .../new_framework_defaults_7_0.rb.tt | 5 ++ 9 files changed, 137 insertions(+), 18 deletions(-) create mode 100644 activesupport/lib/active_support/isolated_execution_state.rb create mode 100644 activesupport/test/isolated_execution_state_test.rb diff --git a/activesupport/lib/active_support.rb b/activesupport/lib/active_support.rb index f148992690..359be9440a 100644 --- a/activesupport/lib/active_support.rb +++ b/activesupport/lib/active_support.rb @@ -47,6 +47,7 @@ module ActiveSupport autoload :EventedFileUpdateChecker autoload :ForkTracker autoload :LogSubscriber + autoload :IsolatedExecutionState autoload :Notifications autoload :Reloader autoload :SecureCompareRotator @@ -115,10 +116,6 @@ module ActiveSupport DateAndTime::Compatibility.utc_to_local_returns_utc_offset_times = value end - def self.current_attributes_use_thread_variables=(value) - CurrentAttributes._use_thread_variables = value - end - @has_native_class_descendants = Class.method_defined?(:descendants) # RUBY_VERSION >= "3.1" end diff --git a/activesupport/lib/active_support/current_attributes.rb b/activesupport/lib/active_support/current_attributes.rb index c35bb72458..b4c6a5d426 100644 --- a/activesupport/lib/active_support/current_attributes.rb +++ b/activesupport/lib/active_support/current_attributes.rb @@ -155,24 +155,13 @@ module ActiveSupport current_instances.clear end - def _use_thread_variables=(value) # :nodoc: - clear_all - @@use_thread_variables = value - end - @@use_thread_variables = false - private def generated_attribute_methods @generated_attribute_methods ||= Module.new.tap { |mod| include mod } end def current_instances - if @@use_thread_variables - Thread.current.thread_variable_get(:current_attributes_instances) || - Thread.current.thread_variable_set(:current_attributes_instances, {}) - else - Thread.current[:current_attributes_instances] ||= {} - end + IsolatedExecutionState[:current_attributes_instances] ||= {} end def current_instances_key diff --git a/activesupport/lib/active_support/isolated_execution_state.rb b/activesupport/lib/active_support/isolated_execution_state.rb new file mode 100644 index 0000000000..5d972a4acd --- /dev/null +++ b/activesupport/lib/active_support/isolated_execution_state.rb @@ -0,0 +1,52 @@ +# frozen_string_literal: true + +require "fiber" + +module ActiveSupport + module IsolatedExecutionState # :nodoc: + @isolation_level = :thread + + Thread.attr_accessor :active_support_execution_state + Fiber.attr_accessor :active_support_execution_state + + class << self + attr_reader :isolation_level + + def isolation_level=(level) + unless %i(thread fiber).include?(level) + raise ArgumentError, "isolation_level must be `:thread` or `:fiber`, got: `#{level.inspect}`" + end + + if level != isolation_level + clear + singleton_class.alias_method(:current, "current_#{level}") + singleton_class.send(:private, :current) + @isolation_level = level + end + end + + def [](key) + current[key] + end + + def []=(key, value) + current[key] = value + end + + def clear + current.clear + end + + private + def current_thread + Thread.current.active_support_execution_state ||= {} + end + + def current_fiber + Fiber.current.active_support_execution_state ||= {} + end + + alias_method :current, :current_thread + end + end +end diff --git a/activesupport/lib/active_support/railtie.rb b/activesupport/lib/active_support/railtie.rb index 30177ac472..da1716fbfb 100644 --- a/activesupport/lib/active_support/railtie.rb +++ b/activesupport/lib/active_support/railtie.rb @@ -9,6 +9,12 @@ module ActiveSupport config.eager_load_namespaces << ActiveSupport + initializer "active_support.isolation_level" do |app| + if level = app.config.active_support.delete(:isolation_level) + ActiveSupport::IsolatedExecutionState.isolation_level = level + end + end + initializer "active_support.remove_deprecated_time_with_zone_name" do |app| config.after_initialize do if app.config.active_support.remove_deprecated_time_with_zone_name diff --git a/activesupport/test/current_attributes_test.rb b/activesupport/test/current_attributes_test.rb index 85cb111dee..8ed0706375 100644 --- a/activesupport/test/current_attributes_test.rb +++ b/activesupport/test/current_attributes_test.rb @@ -177,21 +177,28 @@ class CurrentAttributesTest < ActiveSupport::TestCase end test "CurrentAttributes use fiber-local variables" do + previous_level = ActiveSupport::IsolatedExecutionState.isolation_level + ActiveSupport::IsolatedExecutionState.isolation_level = :fiber + Session.current = 42 enumerator = Enumerator.new do |yielder| yielder.yield Session.current end assert_nil enumerator.next + ensure + ActiveSupport::IsolatedExecutionState.isolation_level = previous_level end test "CurrentAttributes can use thread-local variables" do - ActiveSupport::CurrentAttributes._use_thread_variables = true + previous_level = ActiveSupport::IsolatedExecutionState.isolation_level + ActiveSupport::IsolatedExecutionState.isolation_level = :thread + Session.current = 42 enumerator = Enumerator.new do |yielder| yielder.yield Session.current end assert_equal 42, enumerator.next ensure - ActiveSupport::CurrentAttributes._use_thread_variables = false + ActiveSupport::IsolatedExecutionState.isolation_level = previous_level end end diff --git a/activesupport/test/isolated_execution_state_test.rb b/activesupport/test/isolated_execution_state_test.rb new file mode 100644 index 0000000000..1420674237 --- /dev/null +++ b/activesupport/test/isolated_execution_state_test.rb @@ -0,0 +1,56 @@ +# frozen_string_literal: true + +require_relative "abstract_unit" + +class IsolatedExecutionStateTest < ActiveSupport::TestCase + setup do + ActiveSupport::IsolatedExecutionState.clear + @original_isolation_level = ActiveSupport::IsolatedExecutionState.isolation_level + end + + teardown do + ActiveSupport::IsolatedExecutionState.clear + ActiveSupport::IsolatedExecutionState.isolation_level = @original_isolation_level + end + + test "#[] when isolation level is :fiber" do + ActiveSupport::IsolatedExecutionState.isolation_level = :fiber + + ActiveSupport::IsolatedExecutionState[:test] = 42 + assert_equal 42, ActiveSupport::IsolatedExecutionState[:test] + enumerator = Enumerator.new do |yielder| + yielder.yield ActiveSupport::IsolatedExecutionState[:test] + end + assert_nil enumerator.next + + assert_nil Thread.new { ActiveSupport::IsolatedExecutionState[:test] }.value + end + + test "#[] when isolation level is :thread" do + ActiveSupport::IsolatedExecutionState.isolation_level = :thread + + ActiveSupport::IsolatedExecutionState[:test] = 42 + assert_equal 42, ActiveSupport::IsolatedExecutionState[:test] + enumerator = Enumerator.new do |yielder| + yielder.yield ActiveSupport::IsolatedExecutionState[:test] + end + assert_equal 42, enumerator.next + + assert_nil Thread.new { ActiveSupport::IsolatedExecutionState[:test] }.value + end + + test "changing the isolation level clear the old store" do + original = ActiveSupport::IsolatedExecutionState.isolation_level + other = ActiveSupport::IsolatedExecutionState.isolation_level == :fiber ? :thread : :fiber + + ActiveSupport::IsolatedExecutionState[:test] = 42 + ActiveSupport::IsolatedExecutionState.isolation_level = original + assert_equal 42, ActiveSupport::IsolatedExecutionState[:test] + + ActiveSupport::IsolatedExecutionState.isolation_level = other + assert_nil ActiveSupport::IsolatedExecutionState[:test] + + ActiveSupport::IsolatedExecutionState.isolation_level = original + assert_nil ActiveSupport::IsolatedExecutionState[:test] + end +end diff --git a/guides/source/configuring.md b/guides/source/configuring.md index bc127c22bd..00d92cddf0 100644 --- a/guides/source/configuring.md +++ b/guides/source/configuring.md @@ -1384,6 +1384,11 @@ Configures deprecation warnings that the Application considers disallowed. This Allows you to disable all deprecation warnings (including disallowed deprecations); it makes `ActiveSupport::Deprecation.warn` a no-op. This is enabled by default in production. +#### `active_support.isolation_level` + +Configures the locality of most of Rails internal state. If you use a fiber based server or job processor (e.g. `falcon`), you should set it to `:fiber`. +Otherwise it is best to use `:thread` locality. + #### `config.active_support.use_rfc4122_namespaced_uuids` Specifies whether generated namespaced UUIDs follow the RFC 4122 standard for namespace IDs provided as a `String` to `Digest::UUID.uuid_v3` or `Digest::UUID.uuid_v5` method calls. @@ -1816,6 +1821,7 @@ Accepts a string for the HTML tag used to wrap attachments. Defaults to `"action - `config.active_support.key_generator_hash_digest_class`: `OpenSSL::Digest::SHA1` - `config.active_support.cache_format_version`: `6.1` - `config.active_support.executor_around_test_case`: `false` +- `active_support.isolation_level`: `:thread` - ``config.active_support.use_rfc4122_namespaced_uuids``: `false` - `config.action_dispatch.return_only_request_media_type_on_content_type`: `true` - `ActiveSupport.utc_to_local_returns_utc_offset_times`: `false` diff --git a/railties/lib/rails/application/configuration.rb b/railties/lib/rails/application/configuration.rb index a832ba2530..1f66e84176 100644 --- a/railties/lib/rails/application/configuration.rb +++ b/railties/lib/rails/application/configuration.rb @@ -215,6 +215,7 @@ module Rails active_support.cache_format_version = 7.0 active_support.use_rfc4122_namespaced_uuids = true active_support.executor_around_test_case = true + active_support.isolation_level = :thread end if respond_to?(:action_mailer) diff --git a/railties/lib/rails/generators/rails/app/templates/config/initializers/new_framework_defaults_7_0.rb.tt b/railties/lib/rails/generators/rails/app/templates/config/initializers/new_framework_defaults_7_0.rb.tt index 43367fb97f..fba2225837 100644 --- a/railties/lib/rails/generators/rails/app/templates/config/initializers/new_framework_defaults_7_0.rb.tt +++ b/railties/lib/rails/generators/rails/app/templates/config/initializers/new_framework_defaults_7_0.rb.tt @@ -51,6 +51,11 @@ # and asynchronous queries will then be enabled. # Rails.application.config.active_support.executor_around_test_case = true +# Define the isolation level of most of Rails internal state. +# If you use a fiber based server or job processor, you should set it to `:fiber`. +# Otherwise the default of `:thread` if preferable. +# Rails.application.config.active_support.isolation_level = :thread + # Set both the `:open_timeout` and `:read_timeout` values for `:smtp` delivery method. # Rails.application.config.action_mailer.smtp_timeout = 5