rails / rails

Ruby on Rails
https://rubyonrails.org
MIT License
55.77k stars 21.59k forks source link

`CurrentAttribute`s are cleared when a job gets executed inline in tests #49227

Open Earlopain opened 1 year ago

Earlopain commented 1 year ago

Steps to reproduce

I have a test case like this

# frozen_string_literal: true

require "bundler/inline"

gemfile(true) do
  source "https://rubygems.org"

  git_source(:github) { |repo| "https://github.com/#{repo}.git" }

  gem "rails", github: "rails/rails", branch: "main"
end

require "active_support/railtie"
require "active_job/railtie"
require "minitest/autorun"

class TestApp < Rails::Application
  config.load_defaults 7.1

  config.active_job.queue_adapter = :inline
  config.eager_load = false
end

Rails.application.initialize!

class Current < ActiveSupport::CurrentAttributes
  attribute :user
end

class DummyJob < ActiveJob::Base
  def perform
  end
end

class CurrentAttributeTest < ActiveSupport::TestCase
  test "CurrentAttributes" do
    Current.user = "test"
    DummyJob.perform_later

    assert_equal("test", Current.user, "CurrentAttributes reset")
  end
end

Using the inline queue adapter the second assertion failed. I tried making a self-contained test script but couldn't manage, the attributes only seem to be reset when I run it in a full-blown rails app. I have created a sample app here https://github.com/Earlopain/rails-current-attributes, you can just run rake test there.

I found issue #37526 and pr #37568 which seem to be about the same thing and it looks like they were closed as fixed/completed. Don't know what to make of that, just wanted to document.

Expected behavior

CurrentAttributes are preserved.

Actual behavior

CurrentAttributes are cleared.

System configuration

Rails version: main

Ruby version: 3.2.2

bensheldon commented 1 year ago

I'm familiar with this. This happens because anytime a Executor/Reloader context closes, it resets the CurrentAttributes:

https://github.com/rails/rails/blob/5a92880f29226200019406a4369cce3668083b2a/activesupport/lib/active_support/railtie.rb#L48-L50

ActiveJob wraps every job execution with an Reloader:

https://github.com/rails/rails/blob/5a92880f29226200019406a4369cce3668083b2a/activejob/lib/active_job/railtie.rb#L63-L71

btw, the change in #37568 was undone in #40626, which has an explanation for why.

What I'm unsure about (and is likely what you're having trouble reproducing in your test) is that doubly-wrapped executors should be a noop:

https://github.com/rails/rails/blob/5a92880f29226200019406a4369cce3668083b2a/activesupport/lib/active_support/reloader.rb#L71-L72

So if you're running the job inline in a controller action, it will already be wrapped in an executor, so it should not reset the CurrentAttributes. But what I think is maybe the problem here is that Active Job specifically uses a Reloader (it's the only place directly in the Rails framework that does) which may have different behavior. e.g. (I have not tried this):

 Rails.application.executor.wrap
   Current.user = "test"
   Rails.application.reloader.wrap
     assert_equal("test", Current.user, "FIRST")
   end
   assert_equal("test", Current.user, "SECOND")
 end

Hopefully those are some helpful points 🤗

matthewd commented 1 year ago

Active Job specifically uses a Reloader (it's the only place directly in the Rails framework that does)

Requests do too.. there's just a bit more indirection (no wrap) because of the Rack API -- https://github.com/rails/rails/blob/b67bdfb8032d5b88d0a1912e8cef70aa73f6cee9/railties/lib/rails/application/default_middleware_stack.rb#L66

Granted, that is wrapped in a conditional that won't be true in a test environment. (Only because it's easy, and known to be a no-op -- there's no need to specifically avoid reloader.wrap in non-reloading environments.)

Earlopain commented 1 year ago

Thank you @bensheldon, that has given me some pointers into what to look at more closely.

Running tests through rails there are two executors active. The first is Rails.application.executor from ActiveSupport::Executor::TestHelper, the second is ActiveSupport::Reloader from initializer "active_job.set_reloader_hook". Both run, they are not considered active because they are not the same object. The flow is not all that clear to me but I assume that each test is wrapped in an executor because of executor_around_test_case and jobs that are being executed inline are as well. I traced the calls by printing caller in `before_reset of the current attributes.

My self-contained test script doesn't run executors at all. But even knowing about them I can't figure out how to simulate that behaviour. Perhaps someone else is able to.

Earlopain commented 1 year ago

I managed to produce a single-file reproduction script

# frozen_string_literal: true

require "bundler/inline"

gemfile(true) do
  source "https://rubygems.org"

  git_source(:github) { |repo| "https://github.com/#{repo}.git" }

  gem "rails", github: "rails/rails", branch: "main"
end

require "active_support/railtie"
require "active_job/railtie"
require "minitest/autorun"

class TestApp < Rails::Application
  config.load_defaults 7.1

  config.active_job.queue_adapter = :inline
  config.eager_load = false
end

Rails.application.initialize!

class Current < ActiveSupport::CurrentAttributes
  attribute :user
end

class DummyJob < ActiveJob::Base
  def perform
  end
end

class CurrentAttributeTest < ActiveSupport::TestCase
  test "CurrentAttributes" do
    Current.user = "test"
    DummyJob.perform_later

    assert_equal("test", Current.user, "CurrentAttributes reset")
  end
end
byroot commented 12 months ago

I'll have to dig deeper (cc @casperisfine), but I think this is semi-intended.

If you perform a request or run a job as part of the test, they should start with a clean context, like they would in production.

However, we should probably "push" the previous context, and "pop" it after.

rails-bot[bot] commented 8 months ago

This issue has been automatically marked as stale because it has not been commented on for at least three months. The resources of the Rails team are limited, and so we are asking for your help. If you can still reproduce this error on the 7-1-stable branch or on main, please reply with all of the information you have about it in order to keep the issue open. Thank you for all your contributions.

Earlopain commented 8 months ago

I'd love to fix this myself but unfortunatly this whole mechanism goes way over my head. Can this get the reproduction label so it doesn't become stale? I didn't have something on the initial issue report but have since then added a repro script.

rience commented 8 months ago

Meanwhile, I ended up using this workaround for tests. This stores current attribute in local variable and then sets it back after each execution of job.

current_attribute_value = Current.attribute

Rails.application.executor.set_callback :complete, :after, ->() {
  Current.attribute = current_attribute_value
}
bassemawhoob commented 8 months ago

To anyone still looking for a solution, I came up with this solution inspired by Sidekiq's approach of solving this, yet this is framework agnostic, only depends on ActiveJob so it works in all environments.

# frozen_string_literal: true
# app/lib/active_job/current_attributes.rb
module ActiveJob
  # Automatically save and load any current attributes in the execution context so context attributes “flow”
  # from Rails actions into any associated jobs. This can be useful for multi-tenancy, i18n locale, timezone,
  # any implicit per-request attribute.
  module CurrentAttributes
    extend ActiveSupport::Concern

    class_methods do
      # Defines the +CurrentAttributes+ class that will be persisted between the Rails action into the job
      # @see https://api.rubyonrails.org/classes/ActiveSupport/CurrentAttributes.html
      #
      # == Options
      #
      # * <tt>:key</tt> -  By default, +:attributes+ will be used which will cover *all* attributes defined
      #   in the store unless a specific attribute key is passed.
      #
      #  # You can pass :context or :user as the key or leave it empty which will store both attributes
      #  class Current < ::ActiveSupport::CurrentAttributes
      #    attribute :context
      #    attribute :user
      #  end
      def persist_current_attributes(store, key: :attributes)
        store_name = store.name
        accessor = store_name.parameterize.underscore
        send :attr_accessor, accessor
        stores[store_name] = { key:, accessor: }
      end
    end

    included do
      # rubocop:disable ThreadSafety/ClassAndModuleAttributes
      class_attribute :stores, instance_writer: false
      # rubocop:enable ThreadSafety/ClassAndModuleAttributes
      self.stores = {}

      # Step 1: Before enqueue, the thread data is still available - store them as instance variables
      # on the job level.
      before_enqueue do |job|
        stores.each do |store, options|
          attrs = store.classify.constantize.send(options[:key])
          next if attrs.blank?

          job.send("#{options[:accessor]}=", attrs)
        end
      end

      # Step 2: Serialize the instance including all the new instance variables introduced
      def serialize
        value = {}
        stores.each do |store, options|
          value[store] = send(options[:accessor])
        end
        super.merge(value)
      end

      # Step 3: Deserialize the instance by the time the job is about to be performed & set
      # the instance variables to their respective values
      def deserialize(job_data)
        super
        stores.each do |store, options|
          send("#{options[:accessor]}=", job_data[store])
        end
      end

      # Step 4: Before performing the job, retrieve the values of the instance variables &
      # set the store classes to their values before the job is enqueued
      before_perform do |job|
        stores.each do |store, options|
          store.classify.constantize.send("#{options[:key]}=",
                                          job.send(options[:accessor]))
        end
      end
    end
  end
end

And use it in your ApplicationJob.rb like:

# frozen_string_literal: true

class ApplicationJob < ActiveJob::Base
    persist_current_attributes Current
end    

cc @Earlopain @rience