Introduce rubocop-thread_safety without actually migrating to Puma (#…

…9681)

* Install Puma in Gemfile

* Migrate Unicorn config to Puma

* Run test suite on Puma

* Add thread-safety Rubocop

* Use Puma in production Docker

* Replace Rails model concerns with proper caching

* Store Regulations in actual Rails cache

* Cache variable in TranslationsController

* Instantiate city validator on-the-fly

* Turn Results Validator descriptions into override class methods

* Cache WRT Person script in Rails cache

* Ignore class method instance variable in avatar_uploader

* Delegate Vault credential fetching to SuperConfig property

* Partially roll back WRT script changes: C compilation caches itself

* Un-privatize cache access in Regulation delegator model

* Move Puma workers config into if-production block

* Fix cache loading in Regulation class

* Wrap Regulations load errors because S3 uses anonymous classes that cannot be serialized :/

* Enable in-memory caching in tests

* Clear user search cache in corresponding tests

* Use correct naming of AppSecret Vault variables

* Use more efficient caching for Cachable concern

* Use Rails thread-based caching for Cachable concerns

* Use Rails thread-safe caching approach for Regulations

* Use Rails thread-safe caching approach for TranslationsController

* Set Puma thread count default to 3

* Reinstate Unicorn in production

.rubocop.yml

@@ -1,5 +1,7 @@
 inherit_from: .rubocop_todo.yml
 
+require: rubocop-thread_safety
+
 AllCops:
   TargetRubyVersion: 3.3
   DisplayCopNames: true

Gemfile

@@ -130,6 +130,7 @@ end
 group :development do
   gem 'overcommit', require: false
   gem 'rubocop', require: false
+  gem 'rubocop-thread_safety', require: false
   gem 'better_errors'
   gem 'binding_of_caller'
   gem 'bullet'

Gemfile.lock

@@ -669,6 +669,8 @@ GEM
       unicode-display_width (>= 2.4.0, < 3.0)
     rubocop-ast (1.31.3)
       parser (>= 3.3.1.0)
+    rubocop-thread_safety (0.5.1)
+      rubocop (>= 0.90.0)
     ruby-ll (2.1.2)
       ansi
       ast
@@ -912,6 +914,7 @@ DEPENDENCIES
   rspec-rails
   rspec-retry
   rubocop
+  rubocop-thread_safety
   sass-rails
   sassc-embedded
   sdoc

app/controllers/translations_controller.rb

@@ -3,11 +3,14 @@
 class TranslationsController < ApplicationController
   before_action :authenticate_user!, except: [:index]
 
+  thread_mattr_accessor :bad_keys, instance_writer: false
+
   def self.bad_i18n_keys
-    @bad_keys ||= begin
+    self.bad_keys ||= begin
       english = locale_to_translation('en')
-      (I18n.available_locales - [:en]).to_h do |locale|
-        [locale, locale_to_translation(locale).compare_to(english)]
+
+      (I18n.available_locales - [:en]).index_with do |locale|
+        locale_to_translation(locale).compare_to(english)
       end
     end
   end

app/models/concerns/cachable.rb

@@ -18,24 +18,31 @@
 module Cachable
   extend ActiveSupport::Concern
 
+  included do
+    thread_mattr_accessor :models_by_id
+
+    after_commit :clear_cache
+
+    def clear_cache
+      self.models_by_id = nil
+    end
+  end
+
   class_methods do
-    # We want to keep the cache in a class variable.
-    # Assuming we have a class like Country < AbstractCachedModel, we want the
-    # class variable within Country, not within AbstractCachedModel.
-    # Using directly @@models_by_id would set it in the latter, so we need to rely
-    # on 'class_variable_get' and 'class_variable_set' that will act on the extending
-    # class (here Country).
     def c_all_by_id
-      class_variable_set(:@@models_by_id, all.index_by(&:id)) unless class_variable_defined?(:@@models_by_id)
-      class_variable_get(:@@models_by_id)
+      self.models_by_id ||= self.all.index_by(&:id)
     end
 
     def c_find(id)
-      c_all_by_id[id]
+      self.c_all_by_id[id]
     end
 
     def c_find!(id)
-      self.c_find(id) || raise("id not found: #{id}")
+      self.c_find(id) || raise("Cached model #{self.name} ID not found: #{id}")
+    end
+
+    def c_values
+      self.c_all_by_id.values
     end
   end
 end

app/models/concerns/localized_sortable.rb

@@ -21,6 +21,8 @@ def real?
 
   included do
     scope :uncached_real, -> { where.not(id: fictive_ids) }
+
+    thread_mattr_accessor :real_objects, :all_sorted_by_locale, instance_accessor: false
   end
 
   class_methods do
@@ -29,15 +31,14 @@ def fictive_ids
     end
 
     def real
-      @real_objects ||= self.uncached_real
+      self.real_objects ||= self.uncached_real
     end
 
     def all_sorted_by(locale, real: false)
-      @all_sorted_by_locale ||= I18n.available_locales.to_h do |available_locale|
-        objects = I18nUtils.localized_sort_by!(available_locale, self.c_all_by_id.values) { |object| object.name_in(available_locale) }
-        [available_locale, objects]
+      self.all_sorted_by_locale ||= I18n.available_locales.index_with do |available_locale|
+        I18nUtils.localized_sort_by!(available_locale, self.c_all_by_id.values) { |object| object.name_in(available_locale) }
       end.freeze
-      real ? @all_sorted_by_locale[locale].select(&:real?) : @all_sorted_by_locale[locale]
+      real ? self.all_sorted_by_locale[locale].select(&:real?) : self.all_sorted_by_locale[locale]
     end
   end
 end

app/models/continent.rb

@@ -13,10 +13,10 @@ class Continent < ApplicationRecord
   alias_attribute :record_name, :recordName
 
   def self.country_ids(continent_id)
-    c_all_by_id[continent_id]&.countries&.map(&:id)
+    c_find(continent_id)&.countries&.map(&:id)
   end
 
   def self.country_iso2s(continent_id)
-    c_all_by_id[continent_id]&.countries&.map(&:iso2)
+    c_find(continent_id)&.countries&.map(&:iso2)
   end
 end

app/models/country.rb

@@ -65,7 +65,7 @@ def continent
   end
 
   def self.find_by_iso2(iso2)
-    c_all_by_id.values.select { |c| c.iso2 == iso2 }.first
+    c_values.select { |c| c.iso2 == iso2 }.first
   end
 
   def multiple_countries?

app/models/regulation.rb

@@ -3,44 +3,43 @@
 class Regulation < SimpleDelegator
   REGULATIONS_JSON_PATH = "wca-regulations.json"
 
+  thread_mattr_accessor :regulations, instance_accessor: false, default: []
+  thread_mattr_accessor :regulations_load_error, instance_accessor: false
+
+  def self.regulations_by_id
+    self.regulations.index_by { |r| r["id"] }
+  end
+
   def self.reload_regulations(s3)
-    @regulations = JSON.parse(s3.bucket(RegulationTranslationsHelper::BUCKET_NAME).object(REGULATIONS_JSON_PATH).get.body.read).freeze
-    @regulations_by_id = @regulations.index_by { |r| r["id"] }
-    @regulations_load_error = nil
-  rescue StandardError => e
     reset_regulations
-    @regulations_load_error = e
+
+    self.regulations = JSON.parse(s3.bucket(RegulationTranslationsHelper::BUCKET_NAME).object(REGULATIONS_JSON_PATH).get.body.read).freeze
+  rescue StandardError => e
+    self.regulations_load_error = e
   end
 
   def self.reset_regulations
-    @regulations = []
-    @regulations_by_id = {}
-    @regulations_load_error = nil
+    self.regulations = []
+    self.regulations_load_error = nil
   end
 
   if Rails.env.production?
     reload_regulations(Aws::S3::Resource.new(
                          region: EnvConfig.STORAGE_AWS_REGION,
                          credentials: Aws::ECSCredentials.new,
                        ))
-  else
-    reset_regulations
-  end
-
-  class << self
-    attr_accessor :regulations_load_error
   end
 
   def limit(number)
     first(number)
   end
 
   def self.find_or_nil(id)
-    @regulations_by_id[id]
+    self.regulations_by_id[id]
   end
 
   def self.search(query, *)
-    matched_regulations = @regulations.dup
+    matched_regulations = self.regulations.dup
     query.downcase.split.each do |part|
       matched_regulations.select! do |reg|
         %w(content_html id).any? { |field| reg[field].downcase.include?(part) }

app/uploaders/avatar_uploader_base.rb

@@ -6,7 +6,12 @@ class AvatarUploaderBase < CarrierWave::Uploader::Base
   include CarrierWave::MiniMagick
 
   def self.connection_cache
+    # rubocop:disable ThreadSafety/InstanceVariableInClassMethod
+    #
+    # Worst thing that can happen is that two Puma workers open their own connections when they could have re-used one.
+    # And the avatar upload through this Carrierwave system is deprecated anyways.
     @connection_cache ||= {}
+    # rubocop:enable ThreadSafety/InstanceVariableInClassMethod
   end
 
   def cloudfront_sdk

app_secrets.rb

@@ -7,20 +7,26 @@
 SuperConfig::Base.class_eval do
   # The skeleton is stolen from the source code of the `superconfig` gem, file lib/superconfig.rb:104
   #   (method SuperConfig::Base#credential). The inner Vault fetching logic is custom-written :)
-  def vault(secret_name, &block)
-    define_singleton_method(secret_name) do
-      @__cache__[:"_vault_#{secret_name}"] ||= begin
-        value = self.vault_read(secret_name)[:value]
-        block ? block.call(value) : value
+  def vault(secret_name, cache: true)
+    self.property(secret_name, cache: cache) do
+      value = self.vault_read(secret_name)
+
+      if block_given?
+        yield value
+      else
+        # Vault stores things in a JSON with lots of metadata entries.
+        # The actual secret itself is stored inside that JSON under the key "value"
+        value[:value]
       end
     end
   end
 
-  def vault_file(secret_name, file_path)
-    define_singleton_method(secret_name) do
+  def vault_file(secret_name, file_path, refresh: true)
+    File.delete(file_path) if refresh && File.exist?(file_path)
+
+    self.vault(secret_name, cache: true) do |vault_secret|
       unless File.exist? file_path
-        value_raw = self.vault_read secret_name
-        File.write file_path, value_raw.to_json
+        File.write file_path, vault_secret.to_json
       end
 
       File.expand_path file_path

config/environments/test.rb

@@ -24,10 +24,11 @@
     'Cache-Control' => "public, max-age=#{1.hour.to_i}",
   }
 
-  # Show full error reports and disable caching.
+  # Show full error reports and disable controller caching.
   config.consider_all_requests_local       = true
   config.action_controller.perform_caching = false
-  config.cache_store = :null_store
+  # We do want to use a memory cache for other tests though, for example to include Country, Continent, etc. models.
+  config.cache_store = :memory_store
 
   # Raise exceptions instead of rendering exception templates.
   config.action_dispatch.show_exceptions = :none

lib/city_validator.rb

@@ -17,11 +17,11 @@ def validate_each(record, attribute, value)
     end
   end
 
-  @validators_by_country_iso2 = CountryCityValidators::Utils::ALL_VALIDATORS
-                                .index_by(&:country_iso_2)
-                                .transform_values(&:new)
-
   def self.get_validator_for_country(country_iso2)
-    @validators_by_country_iso2[country_iso2]
+    validator_for_country = CountryCityValidators::Utils::ALL_VALIDATORS.find do |validator|
+      validator.country_iso_2 == country_iso2
+    end
+
+    validator_for_country&.new
   end
 end

lib/finish_unfinished_persons.rb

@@ -30,7 +30,7 @@ def self.search_persons(competition_ids = nil)
     unfinished_persons = []
     available_id_spots = {} # to make sure that all of the newcomer IDs that we're creating in one batch are unique among each other
 
-    @persons_cache = nil
+    persons_cache = Person.select(:id, :wca_id, :name, :dob, :countryId)
 
     unfinished_person_results.each do |res|
       next if unfinished_persons.length >= MAX_PER_BATCH
@@ -42,7 +42,7 @@ def self.search_persons(competition_ids = nil)
 
       inbox_dob = res.inbox_person&.dob
 
-      similar_persons = compute_similar_persons(res)
+      similar_persons = compute_similar_persons(res, persons_cache)
 
       unfinished_persons.push({
                                 person_id: res.person_id,
@@ -74,18 +74,14 @@ def self.remove_accents(name)
     end.join
   end
 
-  def self.persons_cache
-    @persons_cache ||= Person.select(:id, :wca_id, :name, :dob, :countryId)
-  end
-
-  def self.compute_similar_persons(result, n = 5)
+  def self.compute_similar_persons(result, persons_cache, n = 5)
     res_roman_name = self.extract_roman_name(result.person_name)
 
     only_probas = []
     persons_with_probas = []
 
     # pre-cache probabilities, so that we avoid doing string computations on _every_ comparison
-    self.persons_cache.each do |p|
+    persons_cache.each do |p|
       p_roman_name = self.extract_roman_name(p.name)
 
       name_similarity = self.string_similarity(res_roman_name, p_roman_name)
@@ -106,7 +102,7 @@ def self.compute_similar_persons(result, n = 5)
   def self.string_similarity_algorithm
     # Original PHP implementation uses PHP stdlib `string_similarity` function, which is custom built
     # and "kinda like" Jaro-Winkler. I felt that the rewrite warrants a standardised matching algorithm.
-    @jaro_winkler ||= FuzzyStringMatch::JaroWinkler.create(:native)
+    FuzzyStringMatch::JaroWinkler.create(:native)
   end
 
   def self.string_similarity(a, b)

lib/results_validators/advancement_conditions_validator.rb

@@ -24,7 +24,9 @@ class AdvancementConditionsValidator < GenericValidator
     # They are not taken into account in advancement conditions.
     IGNORE_ROUND_TYPES = ["0", "h", "b"].freeze
 
-    @desc = "This validator checks that advancement between rounds is correct according to the regulations."
+    def self.description
+      "This validator checks that advancement between rounds is correct according to the regulations."
+    end
 
     def self.has_automated_fix?
       false

lib/results_validators/competitions_results_validator.rb

@@ -2,7 +2,9 @@
 
 module ResultsValidators
   class CompetitionsResultsValidator < GenericValidator
-    @desc = "This validator is an aggregate of an arbitrary set of other validators, running on an arbitrary set of competitions."
+    def self.description
+      "This validator is an aggregate of an arbitrary set of other validators, running on an arbitrary set of competitions."
+    end
 
     def self.has_automated_fix?
       false

lib/results_validators/competitor_limit_validator.rb

@@ -5,7 +5,9 @@ class CompetitorLimitValidator < GenericValidator
     COMPETITOR_LIMIT_WARNING = "The number of persons in the competition (%{n_competitors}) is above the competitor limit (%{competitor_limit}). " \
                                "The results of the competitors registered after the competitor limit was reached must be removed."
 
-    @desc = "For competition with a competitor limit, this validator checks that this limit is respected."
+    def self.description
+      "For competition with a competitor limit, this validator checks that this limit is respected."
+    end
 
     def self.has_automated_fix?
       false

lib/results_validators/events_rounds_validator.rb

@@ -13,7 +13,9 @@ class EventsRoundsValidator < GenericValidator
     MISSING_ROUND_RESULTS_ERROR = "There are no results for round %{round_id} but it is listed in the events tab. If this round was not held, please remove the round in the competition's manage events page."
     UNEXPECTED_COMBINED_ROUND_ERROR = "No cutoff was announced for '%{round_name}', but it has been detected as a cutoff round in the results. Please update the round's information in the competition's manage events page."
 
-    @desc = "This validator checks that all events and rounds match between what has been announced and what is present in the results. It also check for a main event and emit a warning if there is none (and if 3x3 is not in the results)."
+    def self.description
+      "This validator checks that all events and rounds match between what has been announced and what is present in the results. It also check for a main event and emit a warning if there is none (and if 3x3 is not in the results)."
+    end
 
     def self.has_automated_fix?
       false

lib/results_validators/generic_validator.rb

@@ -4,8 +4,6 @@ module ResultsValidators
   class GenericValidator
     attr_reader :errors, :warnings, :infos, :apply_fixes
 
-    @desc = "Please override that class variable with a proper description when you inherit the class."
-
     def initialize(apply_fixes: false)
       @apply_fixes = apply_fixes
       reset_state
@@ -53,7 +51,7 @@ def run_validation(validator_data)
     end
 
     def self.description
-      @desc
+      raise "Please override that class variable with a proper description when you inherit the class."
     end
 
     def self.class_name