Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sidekiq::Config#as_json causes StackLevelTooDeep error #6000

Closed
Haniyya opened this issue Aug 10, 2023 · 4 comments
Closed

Sidekiq::Config#as_json causes StackLevelTooDeep error #6000

Haniyya opened this issue Aug 10, 2023 · 4 comments

Comments

@Haniyya
Copy link

Haniyya commented Aug 10, 2023

Hi there!

I know this is not a sidekiq issue per se, since as_json is a method from ActiveSupport but the solution seems to be easily fixable here.

First of all, how to replicate this:

require 'active_support/core_ext'
require 'sidekiq'

# Then add some client middleware to make the configuration recursive

class Client
  def initialize(optional_args)
    @args = optional_args
  end
  def call(worker, job, queue, redis_pool)
    yield
  end
end

Sidekiq.configure_client do |config|
  config.client_middleware do |chain|
    chain.add Client, []
  end
end

# And now this will cause a StackLevelTooDeep error
Sidekiq.default_configuration.as_json

This happened to us while using https://github.com/reidmorrison/rails_semantic_logger in conjunction with sidekiq.
In the Sidekiq::Launcher#run method the config is logged

    # Start this Sidekiq instance. If an embedding process already
    # has a heartbeat thread, caller can use `async_beat: false`
    # and instead have thread call Launcher#heartbeat every N seconds.
    def run(async_beat: true)
      Sidekiq.freeze!
      logger.debug { @config.merge!({}) }
      @thread = safe_thread("heartbeat", &method(:start_heartbeat)) if async_beat
      @poller.start
      @managers.each(&:start)
    end

The SemanticLogger (when using the Json formatter) will then try to call as_json on the Sidekiq::Config value, causing the crash. This is not that serious because (in our case) the error is only thrown in the logging thread, which resumes fine afterwards.
For reference the as_json implementation from active_support:

# activesupport-7.0.7/lib/active_support/core_ext/object/json.rb:58
class Object
  def as_json(options = nil) # :nodoc:
    if respond_to?(:to_hash)
      to_hash.as_json(options)
    else
      instance_values.as_json(options)
    end
  end
end

My proposed solution would be to add the following to Sidekiq::Config:

# Add a new delegator so that
    def_delegators :@options, :[], :[]=, :fetch, :key?, :has_key?, :merge!
# Becomes
    def_delegators :@options, :[], :[]=, :fetch, :key?, :has_key?, :merge!, :to_hash

Then Sidekiq::Config#as_json would behave similar to Sidekiq::Config#to_json and since only @options is rendered, no recursion happens.
Feel free to close the issue if you feel like this isn't sidekiqs problem.

@mperham
Copy link
Collaborator

mperham commented Aug 10, 2023

Why does this only happen with rails_semantic_logger? I was under the impression it only happens if the Oj gem is activated. In that case, I consider it an Oj issue.

@Haniyya
Copy link
Author

Haniyya commented Aug 10, 2023

Thanks for the quick reply!

The connection to rails_semantic_logger is incidental. The culprit is the implementation of as_json (recursively iterating over instance_values) in active_support. We don't use Oj. If Sidekiq::Config had an implementation of to_hash, active_support would instaed choose to use to_hash and would avoid iterating over instance_values, thus avoiding the recursion.

@mperham
Copy link
Collaborator

mperham commented Aug 10, 2023

Got it, so there is a simple workaround by putting that monkeypatch in your initializer. I don’t really like your suggestion as it feels magical, I put a delegator in and the error goes away. Feels very random. I think I might prefer to only log the options as suggested earlier or exclude the reloader. Either way should fix the issue.

@Haniyya
Copy link
Author

Haniyya commented Aug 10, 2023

Alright I will go with the initializer.

Thank you for your time 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants