Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: multi threading compliance #110

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Quentinchampenois
Copy link

@Quentinchampenois Quentinchampenois commented Nov 9, 2023

Description

The current implementation isn't thread-safe. On multi-threaded decidim application users may find inconsistencies on the modified translations.
I was able to reproduce a race condition on the proposals index view. Modifying the translation of decidim.proposals.application_helper.filter_state_values.evaluating and using a scraper inspecting the label text with 4 threads and 100 requests per thread.

Related to

#107

Testing

  • Modify a translation
  • Activate cache
  • Enable multi-threading on application
  • Make multiple concurrent requests on application

Additional information

I tested with different environments :

  • single thread puma
  • 5 threads puma
  • 4 workers and 5 threads per worker puma

I am not sure the current PR is fully thread-safe but I notice a reduction of inconsistencies.

I can share the application used with different configuration, and also the scraper used if wanted

@Quentinchampenois Quentinchampenois marked this pull request as ready for review November 9, 2023 15:20
@BigSnicker
Copy link

Is there a timeline for this getting pulled into master?

@Quentinchampenois
Copy link
Author

I share with you a feedback about this solution,
I notice a reduction of inconsistencies but I don't think this is completely fixed with this solution

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.

2 participants