Skip to content

Arch Forum 2024-04-04

Participants: Backend devs, EMs and Victor

Agenda

  • Culture/locale

Notes

Culture/locale: This year we have had two serious bugs related to current culture and how that affects decimal numbers, (123.4 vs 123,4). Most recently, it was Risk that (wrongly) parsed a decimal number depending on users culture instead of using invariant culture. The end result was that users got much stricter (up to 100x) remittance limits. Earlier this year we had an issue in remittance when the FX rate got 100x better for a similar reason. It seems likely we will experience more of these kinds of bugs unless we change our culture handling logic, since it's very easy to make mistakes.

How it works in backend currently is that if a culture is set in the Accept-Language header, it will be set as the current culture on the running "thread" and used for all further operations unless explicitly opted out. This culture is further passed along to other services in the same way, propagating to all downstream services. Note that the culture used to parse json request body is the invariant culture, but in our code (i.e. double.Parse(..) or variable.ToString() it will use the "thread" culture by default.

Ideas:

  • Run regression tests with Spanish culture. Will help, but unclear how to best to do it and won't prevent the issue, only increase chance of detection. Care must also be taken to not increase test runtime since it's already long, meaning just running all tests twice with both Spanish and English won't work.
  • Write an analyzer that detects all calls to culture sensitive methods (like double.Parse) and warns if they don't use invariant culture. Can be a bit tricky to implement, but would help as long as all cases are found, which might be difficult to ensure.
  • Stop setting current culture on the "thread" and instead have an explicit way to pass around or get the Accept-Language header culture when something needs to be localized according to users culture. This feels the cleanest, just like other similar cases are handled like always UTC etc., but will require some rework in both platform code and each service.

We could decide that the best would be to stop setting "thread" culture. Victor will investigate some more and estimate time requirement and then bring to EMs. However, it also seems reasonable to default regression testing to use Spanish. Victor will sync with Li on this.

("thread" within quotes since it's not really a single thread used when using async code, but the principle is the same.)