https://www.redmine.org/https://www.redmine.org/favicon.ico?16793021292022-04-21T17:12:29ZRedmineRedmine - Defect #36998: Revert lazy loading of i18n files introduced in Redmine 5.0https://www.redmine.org/issues/36998?journal_id=1064712022-04-21T17:12:29ZPavel Rosický
<ul></ul><p>Hi Mischa!<br />thank you for catching this. Yes, there're potential thread-safety issues. Especially in production environments, everything should be eager-loaded.</p>
<p>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</p>
<p>I propose this patch to fix the issue, what do you think?</p>
<pre><code class="diff syntaxhl"><span class="gh">diff --git a/config/initializers/30-redmine.rb b/config/initializers/30-redmine.rb
index fc36977f6..887fcc8ee 100644
</span><span class="gd">--- a/config/initializers/30-redmine.rb
</span><span class="gi">+++ b/config/initializers/30-redmine.rb
</span><span class="p">@@ -4,7 +4,7 @@</span> require 'redmine/configuration'
require 'redmine/plugin_loader'
Rails.application.config.to_prepare do
<span class="gd">- I18n.backend = Redmine::I18n::Backend.new
</span><span class="gi">+ I18n.backend = Redmine::I18n::Backend.new(lazy_load: Rails.env.development?)
</span> # Forces I18n to load available locales from the backend
I18n.config.available_locales = nil
</code></pre>
<p>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.</p> Redmine - Defect #36998: Revert lazy loading of i18n files introduced in Redmine 5.0https://www.redmine.org/issues/36998?journal_id=1064732022-04-21T19:58:48ZPavel Rosický
<ul></ul><pre>
<code class="diff syntaxhl"><span class="gh">diff --git a/lib/redmine/i18n.rb b/lib/redmine/i18n.rb
index 13b84512f..c3578f34c 100644
</span><span class="gd">--- a/lib/redmine/i18n.rb
</span><span class="gi">+++ b/lib/redmine/i18n.rb
</span><span class="p">@@ -158,11 +158,9 @@</span> 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
<span class="gd">- 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
</span><span class="gi">+ # 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)
</span> end
# Adds custom pluralization rules
</code><br /></pre> Redmine - Defect #36998: Revert lazy loading of i18n files introduced in Redmine 5.0https://www.redmine.org/issues/36998?journal_id=1064812022-04-23T08:06:35ZGo MAEDA
<ul><li><strong>Related to</strong> <i><a class="issue tracker-2 status-5 priority-4 priority-default closed" href="/issues/36728">Feature #36728</a>: Reintroduce lazy loading of i18n files</i> added</li></ul> Redmine - Defect #36998: Revert lazy loading of i18n files introduced in Redmine 5.0https://www.redmine.org/issues/36998?journal_id=1065262022-04-27T18:23:48ZHolger Just
<ul></ul><p>Reading a bit through <a href="https://github.com/ruby-i18n/i18n/pull/612" class="external">the PR</a> and the previous issues here on redmine.org, I'd rather tend to switch to the <code>::I18n::Backend::Simple</code> backend again.</p>
<p>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 <code>RAILS_ENV</code> is used, full startup takes between 4 and 6 seconds with a plain Redmine.</p>
<p>As such, given the possible issues with the <code>LazyLoadable</code> backend, I think it's just not worth it and we should just use the simpler and more deterministic <code>::I18n::Backend::Simple</code>.</p> Redmine - Defect #36998: Revert lazy loading of i18n files introduced in Redmine 5.0https://www.redmine.org/issues/36998?journal_id=1065382022-04-27T21:06:59ZPavel Rosický
<ul></ul><p><a class="user active" href="https://www.redmine.org/users/13581">holger mareck</a> even with the simple backend I18n does load translations after the first call of I18n.t, so you have to measure <strong>the first request</strong>, not just the startup time.</p>
<p>with these two patches (and lazy loading enabled), all tests passed, but I understand your concerns about complexity. Thanks for considering it!</p> Redmine - Defect #36998: Revert lazy loading of i18n files introduced in Redmine 5.0https://www.redmine.org/issues/36998?journal_id=1065402022-04-27T21:21:36ZMarius BĂLTEANU
<ul><li><strong>Target version</strong> set to <i>5.0.1</i></li></ul><p>Is it enough to revert <a class="changeset" title="Lazy load locales (#36728). Patch by Pavel Rosický." href="https://www.redmine.org/projects/redmine/repository/svn/revisions/21448">r21448</a> or we should allow lazy loading on dev environment?</p> Redmine - Defect #36998: Revert lazy loading of i18n files introduced in Redmine 5.0https://www.redmine.org/issues/36998?journal_id=1065542022-04-28T11:04:42ZHolger Just
<ul></ul><p>I'd rather just revert <a class="changeset" title="Lazy load locales (#36728). Patch by Pavel Rosický." href="https://www.redmine.org/projects/redmine/repository/svn/revisions/21448">r21448</a> unless someone can show a real tangible upside of this diversion between environments (which I don't see right now).</p> Redmine - Defect #36998: Revert lazy loading of i18n files introduced in Redmine 5.0https://www.redmine.org/issues/36998?journal_id=1065562022-04-28T14:45:21ZPavel Rosický
<ul></ul><p>these diversions between environments are common - auto_load in development vs eager_load in production</p>
<p>but if you think the risk is too high, please revert <a class="changeset" title="Lazy load locales (#36728). Patch by Pavel Rosický." href="https://www.redmine.org/projects/redmine/repository/svn/revisions/21448">r21448</a> thank you.</p> Redmine - Defect #36998: Revert lazy loading of i18n files introduced in Redmine 5.0https://www.redmine.org/issues/36998?journal_id=1065822022-05-02T20:30:30ZMarius BĂLTEANU
<ul><li><strong>Resolution</strong> set to <i>Fixed</i></li></ul><p>Reverted <a class="changeset" title="Lazy load locales (#36728). Patch by Pavel Rosický." href="https://www.redmine.org/projects/redmine/repository/svn/revisions/21448">r21448</a> in <a class="changeset" title="Reverts r21448 (#36998)." href="https://www.redmine.org/projects/redmine/repository/svn/revisions/21556">r21556</a>.</p> Redmine - Defect #36998: Revert lazy loading of i18n files introduced in Redmine 5.0https://www.redmine.org/issues/36998?journal_id=1065832022-05-02T20:30:42ZMarius BĂLTEANU
<ul><li><strong>Status</strong> changed from <i>New</i> to <i>Resolved</i></li><li><strong>Assignee</strong> set to <i>Marius BĂLTEANU</i></li></ul> Redmine - Defect #36998: Revert lazy loading of i18n files introduced in Redmine 5.0https://www.redmine.org/issues/36998?journal_id=1065882022-05-03T16:41:43ZMarius BĂLTEANU
<ul><li><strong>Subject</strong> changed from <i>::I18n::Backend::LazyLoadable is not deemed fit for production use</i> to <i>Revert lazy loading of i18n files introduced in Redmine 5.0</i></li><li><strong>Status</strong> changed from <i>Resolved</i> to <i>Closed</i></li></ul><p>Reverted in <a class="changeset" title="Reverts r21448 (#36998)." href="https://www.redmine.org/projects/redmine/repository/svn/revisions/21556">r21556</a> and merged to 5.0-stable branch. Thank you all for your reviews!</p>
<p>Pavel, I agree with Holger, we can discuss this again for non production environments if we can obtain a meaningfully improvement.</p> Redmine - Defect #36998: Revert lazy loading of i18n files introduced in Redmine 5.0https://www.redmine.org/issues/36998?journal_id=1065902022-05-03T17:22:04ZPavel Rosický
<ul></ul><p>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<br /><a class="external" href="https://github.com/redmine/redmine/commit/3739810afa3545e6747a9111185dc8808fff6078">https://github.com/redmine/redmine/commit/3739810afa3545e6747a9111185dc8808fff6078</a></p>
<p>but I agree it's better to keep it as simple as possible. Thank you all for your comments!</p>