Skip to content

Conversation

@Bonias
Copy link

@Bonias Bonias commented Jan 28, 2019

Fixes #126

The bug is caused by the presence of two things: Rails code reloading and Lit caching (storing models in globals).

Basically Lit stores instance of Lit::locale(#111111) class in the Lit.loader.cache (it's like a global). Then code is changed by developer and on the next request rails reloads it. Lit::Locale is also reloaded, so we have new Lit::Locale(#222222)class now but instance of old class is still stored in the cache.
And now we have code localization.locale = locale, where locale on the right side is outdated instance from the cache. localization on the left side is using new reloaded code so is expecting new Lit::Locale instance.


It's easy to reproduce:

# Generate fresh app
rails new lit-bug
cd lit-bug
echo 'gem "lit"' >> Gemfile
bundle install
bin/rails g lit:install # choose hash or redis as key_value_engine - doesn't matter
echo 'Rails.application.config.i18n.available_locales = [:en, :de]' > config/initializers/available_locales.rb
# You can also use your existing app, but reset SQL DB and redis DB first
bin/rake db:drop && bin/rake db:create && bin/rake db:migrate && redis-cli flushdb
# run rails server
bin/rails s
# in the other terminal window make first request (which populates cache)
curl "http://localhost:3000/lit/localization_keys/not_translated?key=errors.messages.invalid"
# trigger code reload
touch app/controllers/application_controller.rb
# make another request (important - with other param because we want lit to try update DB) and see the error
curl "http://localhost:3000/lit/localization_keys/not_translated?key=errors.messages.accepted"

Actually there is easier way to reproduce the problem. Run rails console and then:

I18n.t(:non_existing_key_1) && reload! && I18n.t(:non_existing_key_2)

I can think of 3 ways to fix it:

  1. do not assign instances of ActiveRecord::Base to any global - it's rather not doable
  2. force rails somehow to cache Lit related classes (so it don't reload them) - but I don't know how to do that
  3. clear Lit cache on each code reload. And this is what this PR do.

You may be wondering why there weren't such errors before Thu Nov 22. On that date 1478d85 was introduced which added localization.locale = locale line. So before that time outdated Lit::Locale object was stored as well but it wasn't used to create Lit::Localization objects so there were not any conflicts.


BTW

I'm also wondering, shouldn't Lit.loader.cache be considered a memory leak in the current form? AR objects are pushed there but never removed in the whole process lifetime. I'm talking mainly about @localization_object_cache and @localization_key_object_cache.
Why are they needed? It seems they are added only when translation doesn't exist yet. For example:

Add to PostsController#index:

1000.times { |i| I18n.t("key_#{i}") }
  1. run rails server
  2. navigate to /posts - @localization_object_cache and @localization_key_object_cache will have 1000 AR objects each.
  3. stop rails server and rerun it
  4. navigate to /posts - now @localization_object_cache and @localization_key_object_cache are empty.

So why these cache vars are even needed if they don't seem to be used after persisting?

If this is needed for some reason then shouldn't it be stored per request or something like that?

@Bonias
Copy link
Author

Bonias commented Jan 28, 2019

I've just realized I shouldn't clear localizations and localization_keys because it deletes redis entries. I'll fix that.

Update: PR updated.

Lit cache needs to be cleared on each code reload. Otherwise we can get
error "like Locale(#84661580) expected, got (...) which is an instance
of Lit::Locale(#75155120)".
@Bonias Bonias force-pushed the fix-problem-with-rails-reloading branch from 3b2ff4c to e355063 Compare January 28, 2019 19:15
@mlitwiniuk mlitwiniuk merged commit 20403f6 into prograils:master Jan 31, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

3 participants