Redmine

Hi Mischa!
thank you for catching this. Yes, there're potential thread-safety issues. Especially in production environments, everything should be eager-loaded.

by reviewing the PR again, I've found that the feature actually has to be enabled explicitly by lazy_load otherwise, it behaves the same way as SimpleBackend

I propose this patch to fix the issue, what do you think?

diff --git a/config/initializers/30-redmine.rb b/config/initializers/30-redmine.rb
index fc36977f6..887fcc8ee 100644
--- a/config/initializers/30-redmine.rb
+++ b/config/initializers/30-redmine.rb
@@ -4,7 +4,7 @@ require 'redmine/configuration'
 require 'redmine/plugin_loader'

 Rails.application.config.to_prepare do
-  I18n.backend = Redmine::I18n::Backend.new
+  I18n.backend = Redmine::I18n::Backend.new(lazy_load: Rails.env.development?)
   # Forces I18n to load available locales from the backend
   I18n.config.available_locales = nil

note that the previous change was safe, simply because the feature was disabled in all environments and this change enables it for dev environments only.

diff --git a/lib/redmine/i18n.rb b/lib/redmine/i18n.rb
index 13b84512f..c3578f34c 100644
--- a/lib/redmine/i18n.rb
+++ b/lib/redmine/i18n.rb
@@ -158,11 +158,9 @@ module Redmine
     # Custom backend based on I18n::Backend::Simple with the following changes:
     # * available_locales are determined by looking at translation file names
     class Backend < ::I18n::Backend::LazyLoadable
-      module Implementation
-        # Get available locales from the translations filenames
-        def available_locales
-          @available_locales ||= ::I18n.load_path.map {|path| File.basename(path, '.*')}.uniq.sort.map(&:to_sym)
-        end
+      # Get available locales from the translations filenames
+      def available_locales
+        @available_locales ||= ::I18n.load_path.map {|path| File.basename(path, '.*')}.uniq.sort.map(&:to_sym)
       end

       # Adds custom pluralization rules

Reading a bit through the PR and the previous issues here on redmine.org, I'd rather tend to switch to the ::I18n::Backend::Simple backend again.

Within Redmine, (skipping the) load of the translations during startup doesn't seem to meaningfully affect the overall startup time. On my system, regardless of which backend or RAILS_ENV is used, full startup takes between 4 and 6 seconds with a plain Redmine.

As such, given the possible issues with the LazyLoadable backend, I think it's just not worth it and we should just use the simpler and more deterministic ::I18n::Backend::Simple.

holger mareck even with the simple backend I18n does load translations after the first call of I18n.t, so you have to measure the first request, not just the startup time.

with these two patches (and lazy loading enabled), all tests passed, but I understand your concerns about complexity. Thanks for considering it!

I'd rather just revert r21448 unless someone can show a real tangible upside of this diversion between environments (which I don't see right now).

these diversions between environments are common - auto_load in development vs eager_load in production

but if you think the risk is too high, please revert r21448 thank you.

note that Redmine used to have its own backend based on the same idea for years (originally introduced by JPLang in 2012 :) ), so this isn't something new
https://github.com/redmine/redmine/commit/3739810afa3545e6747a9111185dc8808fff6078

but I agree it's better to keep it as simple as possible. Thank you all for your comments!