Amber: Current locale mechanism isn't "thread safe"

[bigforcegun]

Description

Technically, this applies to other shards.
But since the locale is now included in the generator of the new application, I'm writing here.

Citrine::I18n::Handler use I18n.locale to set current locale for request
But this locale sets globally for all application.
if I'm not mistaken it is called race condition.

Example - we have two requests long and short

• locale_fast - really fast
• locale_long - lasts three seconds

If we call locale_long with one Accept-Language, then locale_fast with another Accept-Language - locale_long respond with wrong locale

Steps to Reproduce

class LocaleTestController < ApplicationController
  def locale_fast
    I18n.locale
  end

  def locale_long
    sleep(3)
    respond_with do
      text I18n.locale.to_s
    end
  end
end

Expected behavior:

After paralel call locale_long and locale_fast with differend Accept-Language all respond with own locale key

Actual behavior:

locale_long respods with last called locale

Reproduces how often: [What percentage of the time does it reproduce?]

100% times

Versions

Amber CLI (amberframework.org) - v0.7.2

Crystal 0.24.2 [4f9ed8d03] (2018-03-08)

LLVM: 4.0.0
Default target: x86_64-unknown-linux-gnu

Additional Information

I have some suggestions for resolve this issue.

First - we need new instance of I18n store in context for each request.
And in helpers use this instance for translations
But now it's impossible - shard I18n use class (@@) variables for store config and locale value

Second (i use it for now) - store in context i18n_key and use it in trasnslations with force_locale argument.

[faustinoaq]

/cc @docelic

[docelic]

Hey Mark,

Thanks for the report. Correct, it is not very useful that locale is stored in a class variable in I18n.

There were suggestions to store it in context in Amber, but back then it was deemed as unnecessary because we were discussing it from some other perspective, not thread safety specifically. (We assumed it was thread safe.)

I certainly agree that context is the right way to go about it, and force_locale is a nice way to automatically pass the desired/context locale in t() and l() helpers.

Unless there will be comments suggesting a different course of action, I will implement the method you suggest over the next 2 days.

Thanks!

[docelic]

The changes needed in the "citrine-i18n" shard have been made:
https://github.com/amberframework/citrine-i18n/commit/dad0ce33a2479ffee7ff2f44c25dbde9dfa8ce91

Now the other remaining part is to slightly modify the t() and l() helpers within Amber.