Skip to content

Fix locale selection in multi-threaded Flask app when force_locale is used#125

Merged
TkTech merged 1 commit into
python-babel:devfrom
lautat:fix/thread-safe-force-locale
Sep 23, 2019
Merged

Fix locale selection in multi-threaded Flask app when force_locale is used#125
TkTech merged 1 commit into
python-babel:devfrom
lautat:fix/thread-safe-force-locale

Conversation

@lautat

@lautat lautat commented Oct 8, 2017

Copy link
Copy Markdown
Contributor

This fixes issue #117 by setting babel_locale in force_locale context manager instead of replacing locale getter. Babel extension instance is shared across threads (through Flask.extensions) which may cause wrong locale to be selected in request that is processed at the same time in one thread while another thread is using force_locale. Setting locale to request context is thread-safe as Flask doesn't share request contexts between threads. I've added a test case to verify that this fix works, and it failed without changes to force_locale.

@lautat lautat changed the title Fix locale selection in multi-threaded Flask app Fix locale selection in multi-threaded Flask app when force_locale is used Oct 8, 2017
@tvuotila

tvuotila commented Nov 2, 2018

Copy link
Copy Markdown

Calling flask_babel.refresh() will revert the force_locale prematurely.

@lautat

lautat commented Nov 2, 2018

Copy link
Copy Markdown
Contributor Author

@tvuotila thanks for the feedback. Since (I assume) your fix works with flask_babel.refresh(), I can close this pull request. This fix is not critical for me since I've applied a workaround to the project which was affected by this bug. I can also try to reproduce the issue with flask_babel.refresh() and fix my patch.

@tvuotila

tvuotila commented Nov 6, 2018

Copy link
Copy Markdown

Since (I assume) your fix works with flask_babel.refresh(), I can close this pull request.

Yes, my fix works with flask_babel.refresh().

I can close this pull request.

I like your solution more. It does not introduce another place for locale_selector_func, like mine does.

I can also try to reproduce the issue with flask_babel.refresh() and fix my patch.

If you could, that would be great.

@lautat

lautat commented Nov 6, 2018

Copy link
Copy Markdown
Contributor Author

Alright, I'll fix the patch.

This fixes issue #117 by setting locale in `force_locale` instead of
replacing locale getter. Babel extension instance is shared across
threads which causes issues when `force_locale` is used in one request
while another starts. `forced_babel_locale` attribute of context is used
to override `babel_locale` in case `refresh` is called within
`force_locale` context.
@lautat

lautat commented Nov 6, 2018

Copy link
Copy Markdown
Contributor Author

I fixed the patch. It is not very pretty, but it works now with flask_babel.refresh().

@Dreamsorcerer

Copy link
Copy Markdown

After spending 3 days trying to figure why our site randomly provides Chinese translations to all our users, we found this PR. This has resolved the issue and should be merged into the next release, so others don't have to go through the same troubles.

@lautat

lautat commented Sep 18, 2019

Copy link
Copy Markdown
Contributor Author

Perhaps @TkTech could take a look? It looks like he has been maintaining this project lately.

@TkTech

TkTech commented Sep 20, 2019

Copy link
Copy Markdown
Contributor

@lautat I can take a look this weekend. Keep in mind of the 4 people with admin access to this repo (which doesn't include me), none of them have ever committed or commented. I can't protect branches and add others as reviewers, nothing. So it's unfortunately just me vs the backlog and I no longer use flask at work. @mitsuhiko has unfortunately never replied to any of my emails over the years :(

@TkTech TkTech changed the base branch from master to dev September 23, 2019 02:54
@TkTech TkTech merged commit d5abdd1 into python-babel:dev Sep 23, 2019
@lautat lautat deleted the fix/thread-safe-force-locale branch September 23, 2019 06:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants