プロジェクト

全般

プロフィール

Vote #81912

完了

Revert lazy loading of i18n files introduced in Redmine 5.0

Admin Redmine さんが3年以上前に追加. 3年以上前に更新.

ステータス:
Closed
優先度:
通常
担当者:
-
カテゴリ:
I18n_37
対象バージョン:
開始日:
2022/05/09
期日:
進捗率:

0%

予定工数:
category_id:
37
version_id:
178
issue_org_id:
36998
author_id:
1565
assigned_to_id:
107353
comments:
12
status_id:
5
tracker_id:
1
plus1:
0
affected_version:
closed_on:
affected_version_id:
155
ステータス-->[Closed]

説明

Starting with 5.0.0, Redmine subclasses the @::I18n::Backend::LazyLoadable@ backend (#36728). I haven't looked deeply into the inner workings of this particular backend, but I did notice the following notes/recommendations in the "upstream PR":https://github.com/ruby-i18n/i18n/pull/612:

Most notably, a local test environment.

This backend should only be enabled in test environments!

It's designed for the local test environment [...]

Now I wonder: has the above been considered at all before it was committed? If so: what were the reasons this was nevertheless deemed fit for production use in the Redmine project?


journals

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?

<pre><code class="diff">
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

</code></pre>

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.
--------------------------------------------------------------------------------
<pre>
<code class="diff">
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
</code>
</pre>
--------------------------------------------------------------------------------

--------------------------------------------------------------------------------
Reading a bit through "the PR":https://github.com/ruby-i18n/i18n/pull/612 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 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!
--------------------------------------------------------------------------------
Is it enough to revert r21448 or we should allow lazy loading on dev environment?
--------------------------------------------------------------------------------
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.
--------------------------------------------------------------------------------
Reverted r21448 in r21556.
--------------------------------------------------------------------------------

--------------------------------------------------------------------------------
Reverted in r21556 and merged to 5.0-stable branch. Thank you all for your reviews!

Pavel, I agree with Holger, we can discuss this again for non production environments if we can obtain a meaningfully improvement.
--------------------------------------------------------------------------------
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!
--------------------------------------------------------------------------------


related_issues

relates,Closed,36728,Reintroduce lazy loading of i18n files

Admin Redmine さんが3年以上前に更新

  • カテゴリI18n_37 にセット
  • 対象バージョン5.0.1_178 にセット

他の形式にエクスポート: Atom PDF

いいね!0
いいね!0