Helper middleware to preserve implicit context?

[mperham]

Implicit context via thread-local variables can be a powerful tool for things like multi-tenancy, logging, i18n, etc. Rails has ActiveSupport::CurrentAttributes, there's also gems like RequestStore. It would be excellent if Sidekiq included helper middleware that knows how to de-hydrate and re-hydrate variables between client and server so jobs can preserve this implicit context for their execution.

For example, one possible API:

Sidekiq.configure_client do |config|
  config.client_middleware do |chain|
    chain.add Sidekiq::Context::Save, with: Myapp::Current
  end
end

class Sidekiq::Context::Save
  def call(_, job, _, _)
    # enumerate all saved attributes and de-hydrate them into the job hash
  end
end

[mperham]

module Myapp
  class Current < ActiveSupport::CurrentAttributes
    attribute :user_id, :org_id, :tz, :locale, :user
  end
end

Myapp::Current.tz = 'UTC'
Myapp::Current.org_id = "123456"
Myapp::Current.user_id = 123
Myapp::Current.locale = 'pt-BR'
# will not persist because it is not a native JSON type
# Myapp::Current.user = User.new(id: 123)

Attributes will only persist correctly if they are native JSON types.

[teoljungberg]

👋 I saw your tweet around this last night and was going to post how our middleware looks that solves mirroring what Current was in our application when the job was enqueued. In case it is of any interest

  module Middleware
    class SidekiqCurrent
      CURRENT_USER_KEY = 'current_user_id'.freeze
      REQUEST_ID_KEY = 'request_id'.freeze

      class Client
        def call(_worker_class, job, _queue, _redis_pool)
          job[CURRENT_USER_KEY] = Current.user.id if Current.user&.signed_in?
          job[REQUEST_ID_KEY] = Current.request_id if Current.request_id
          yield
        end
      end

      class Server
        def call(_worker_class, job, _queue)
          Current.user = User.find(job[CURRENT_USER_KEY]) if job.key?(CURRENT_USER_KEY)
          Current.request_id = job[REQUEST_ID_KEY] if job.key?(REQUEST_ID_KEY)
          yield
          Current.user = nil
          Current.request_id = nil
        end
      end
    end
  end

[edwinv]

Attributes will only persist if they are native JSON types. Helper will skip any context attributes that are complex Ruby objects.

This matches the way Sidekiq handled arguments for jobs. I'm a bit afraid this might cause unwanted side effects in regard to the CurrentAttributes.

In our case we have a Current.administration and a Current.user. Both are ActiveRecord instances. When we don't know which user is performing an action, we fall back to administration.system_user. If the context save silently fails for non-native JSON types, aren't we at risk the job is executed in a different context than intended? I think an "all or nothing" approach for the context is better than having a halve context.

That being said, I'm not sure if the middleware should solve the serialization. In that case support for GlobalID would be needed and given Sidekiq doesn't support this for regular arguments, I assume this is not something you want to introduce for this middleware.

When the middleware raises an error when something is not serializable, I would probably rewrite the Current model to something that supports both the object and the id:

class Current < ActiveSupport::CurrentAttributes
  attribute :user, :user_id

  resets { Time.zone = nil }

  def user=(user)
    super
    self.user_id = user.id if self.user_id != user.id
    Time.zone = user.time_zone
  end

  def user_id=(id)
    super
    self.user = User.find(id)
  end
end

This allows us to use Current.user = User.find(...) in requests and still serialized the user_id for jobs. We only need to let the middleware know we can safely discard the object:

chain.add Sidekiq::Context::Save, with: Myapp::Current, skip_attributes: %i[user]

[edwinv]

After working on a Current model for our own application, I'm in doubt if we need all attributes in a job. Maybe we can change the skip_attributes to a only parameter:

chain.add Sidekiq::Context::Save, with: Myapp::Current, only: %i[user_id]

On our Current model, we also store ip, request_id and user_agent. When jobs are started with these in the context, things might get confusing. Why does a job suddenly have a request_id and an ip address?

When we explicitly need to define which attributes should be transferred to a job, I can also see an approach were the Current model is responsible for serializing an deserializing. In that case, the user_id / user situation is also solved nicely:

class Current < ActiveSupport::CurrentAttributes
  attribute :user

  resets do
    I18n.locale = I18n.default_locale
    Time.zone = nil
  end

  def user=(user)
    super
    I18n.locale = user.language
    Time.zone = user.time_zone
  end

  def serialize
    { user_id: user.id }
  end

  def deserialize(attributes)
    self.user = User.find(attributes[:id])
  end
end

[mperham]

One rule I try to always follow is "do the simplest thing possible" as that's also usually the highest performance and easiest to understand. I would prefer to stay away from only or skip type logic. If you put an attribute on Current, you are saying this is important to preserve everywhere this request and its child work execute.

I believe that the Ruby JSON API does have hooks for serialization/deserialization if you want custom objects to transparently work but I've never tried it myself.

Maybe you can also use a thread_mattr_accessor to hold a user object associated with a user_id attribute; the user_id is seamlessly transmitted across process boundaries and the user object can be lazy loaded inside the accessor. t_m_a would be useful if you want attributes per-thread on Current but don't want them to be serialized across process boundaries.

[edwinv]

I agree that we should find the simplest solution possible. I'm still not convinced you should just ignore attributes that couldn't be serialized. Because all attributes in a context form the whole context, one missing attribute makes it another context and therefore can cause unexpected behavior. Without a warning or error, I can even be a security risk. At least the documentation should clearly state this behavior.

I think the JSON additions API is also worth mentioning in the documentation for this feature.

Furthermore, I'm thinking about the concept of a JobContext that automatically syncs with the Current model. This allows for more control over the context that needs to be shared with the jobs. Like I said, I don't like the idea of jobs running in a context with a user_agent or request_id that isn't actually there. This architecture might be nice to mention in the documentation too? Pseudo code of my idea, which needs some tweaking due to potential infinite loops:

class Current < ActiveSupport::CurrentAttributes
  attribute :user, :user_agent, :request_id

  resets do
    JobContext.user_id = nil
  end

  def user=(user)
    super
    JobContext.user_id = user.id
  end
end

class JobContext < ActiveSupport::CurrentAttributes
  attribute :user_id

  resets do
    Current.user = nil
  end

  def user_id=(id)
    super
    Current.user = User.find(id)
  end
end

chain.add Sidekiq::Context::Save, with: Myapp::JobContext

So all in all, I think with the right documentation and warnings about caveats, this simple implementation is a good one. Thanks Mike! 👍

[mperham]

My comment above was incorrect, the code does persist all attributes but they will only persist correctly if they convert to/from JSON types. Glad you tentatively approve, send an issue or PR if you have further ideas/improvements.

[edwinv]

My comment above was incorrect, the code does persist all attributes but they will only persist correctly if they convert to/from JSON types. Glad you tentatively approve, send an issue or PR if you have further ideas/improvements.

Perfect, that eliminates my fear of incorrect contexts. 🚀

[bf4]

In case anyone cares, here's our paper_trail/request_store based version, more or less

config/initializers/sidekiq.rb

require "sidekiq/middleware/current_attributes"
Rails.application.configure do
  config.after_initialize do
    # Avoid 'Initialization autoloaded the constant CurrentRequest'
    Sidekiq::CurrentAttributes.persist(CurrentRequest)
  end
end

app/services/current_request.rb

module CurrentRequest
  extend self

  delegate :[], :[]=, :exist?, :fetch, :delete, :clear!, to: RequestStore

  def user_id
    ::PaperTrail.request.whodunnit
  end

  def user_id=(user_id)
    ::PaperTrail.request.whodunnit = user_id
  end

  def controller_info
    PaperTrail.request.controller_info
  end

  def controller_info=(controller_info)
    PaperTrail.request.controller_info = controller_info
  end

  def request_id=(request_id)
    self[:request_id] = request_id
  end

  def request_id
    self[:request_id]
  end

  # ActiveSupport::CurrentAttributes.attributes compatibility
  # For use with https://github.com/mperham/sidekiq/pull/4982
  def current_attributes_keys
    @current_attributes_keys ||= %i[
      request_id
      controller_info
      user_id
    ]
  end

  # ActiveSupport::CurrentAttributes.attributes compatibility
  # https://github.com/rails/rails/blob/6-0-stable/activesupport/lib/active_support/current_attributes.rb
  def attributes
    current_attributes_keys.each_with_object({}) { |attribute_name, attributes|
      attributes[attribute_name] = public_send(attribute_name)
    }
  end

  # ActiveSupport::CurrentAttributes.set compatibility
  # https://github.com/rails/rails/blob/6-0-stable/activesupport/lib/active_support/current_attributes.rb
  def set(set_attributes)
    old_attributes = compute_attributes(set_attributes.keys)
    assign_attributes(set_attributes)
    yield
  ensure
    assign_attributes(old_attributes)
  end

  private

  def assign_attributes(new_attributes)
    new_attributes.each do |attribute_name, attribute_value|
      public_send("#{attribute_name}=", attribute_value)
    end
  end

  def compute_attributes(keys)
    keys.collect { |key| [key, public_send(key)] }.to_h
  end
end

xref what I had been looking at prior to this PR since 'why not'

• https://karolgalanciak.com/blog/2020/09/20/tracking-all-paper-trail-version-from-a-single-request-with-correlation-uuids/
• see https://github.com/madebylotus/request_store-sidekiq/blob/d095642cbf795b0301c45d423af3f85792d37d9b/lib/request_store/sidekiq/server_middleware.rb#L1-L11
• https://api.rubyonrails.org/classes/ActiveSupport/CurrentAttributes.html#method-c-after_reset
• https://ryanbigg.com/2017/06/current-considered-harmful
• https://www.mintbit.com/blog/rails-current-attributes-usage-pros-and-cons