Skip to content

Fix code reloading for Sidekiq in development

Stan Hu requested to merge sh-fix-code-reload-redis-client-deadlocks into master

What does this MR do and why?

We have been seeing deadlocks in development because code reloading is not thread-safe, since we have multiple Sidekiq threads attempting to reload ApplicationSetting after it is unloaded.

Sidekiq launches heartbeat, scheduler, and job processor threads that talk to Redis. We have a custom Redis instrumentation middleware that logs Prometheus metrics. Each of those those threads attempt to load ApplicationSetting to determine whether Prometheus metrics are enabled, but this behavior is not thread-safe.

As described in https://guides.rubyonrails.org/threading_and_code_execution.html, Rails automatically adds Rack middlewares in development to ensure thread-safety during code reloading. However, the Sidekiq threads don't use these Rack middlewares. We'd have to call Rails.application.executor.wrap manually around every Redis call, but that introduces too much locking overhead.

Instead, let's just disable the RedisClient instrumentation when code reloading is enabled since it doesn't play well with it.

Relates to:

MR acceptance checklist

Please evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.

How to set up and validate locally

How to set up and validate locally

  1. Check out this branch.
  2. Run gdk restart rails.
  3. Run gdk tail rails-background-jobs in another window.
  4. Make some code changes to GitLab: git checkout HEAD~12, for example.
  5. Create a merge request or perform other actions to cause background jobs to run.
  6. From bin/rails console, you could also kick off a job manually: AuthorizedProjectsWorker.perform_async(1).
  7. Watch that the log messages in step 3 continue to flow, and ensure merge requests still get refreshed.
Edited by Stan Hu

Merge request reports