-
-
Notifications
You must be signed in to change notification settings - Fork 21
fix: concurrent issue when bulk-generate with db lock retry and change concurrent to for loop for db lock safe thread. #282
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
Conversation
…e concurrent to for loop for db lock safe thread.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces a retry helper to handle database deadlocks and converts parallel batch operations into sequential loops to ensure DB lock safety when bulk-generating URL aliases.
- Adds
withDbLockRetryto automatically retry on common deadlock or lock-timeout errors. - Replaces
Promise.allconcurrency withfor…ofloops, wrapping each DB operation in the retry helper. - Refactors language initialization flow for clarity.
Comments suppressed due to low confidence (2)
packages/core/server/services/bulk-generate.ts:18
- [nitpick] Add JSDoc comments describing the purpose, parameters (
fn,retries,delay), and return value ofwithDbLockRetryto improve readability and maintainability.
async function withDbLockRetry<T>(
packages/core/server/services/bulk-generate.ts:18
- [nitpick] Consider adding unit tests that simulate deadlock or lock-timeout errors to verify that
withDbLockRetrycorrectly retries and eventually throws after max attempts.
async function withDbLockRetry<T>(
boazpoolman
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @mrdhira,
Thank you for submitting this pull request!
I think you're touching an important aspect of the plugin here, which is:
What to do about running large database queries.
There is no issue reported for this matter just yet, but I'm conscious about this limitation. Especially in the bulk generation, but also in upcoming features (like #280) we need a way to run these large tasks without crashing our database or Strapi application as a whole.
Just so we're on the same page here, this is not the same as #268. That was just a bug in the code, which is solved by #276.
That being said, I think we need to make a good plan before we start tackling this problem, as it's not an easy one to solve. While I appreciate your efforts, I don't think implementing a delay when we're facing deadlocks is the way to solve this. We should try to prevent those deadlocks from happening in the first place.
|
I've created an issue to host our discussions. If you have input on how we can solve this problem, please post them here: |
|
I'm going to close this PR as we're still in the RFC phase of solving this problem. If you (or anybody reading this) has any thoughts on the matter, please put your comments here #284. |
Addresses #268
What does it do?
Why is it needed?
There is issue if you have multiple locale and many data that need generated (mine problem when there is around 5000 data) url alias, there will be deadlock issue because strapi manage data using 2 row of data with same id for manage draft and published state
How to test it?
Generate with in one content types with multiple locales active and set prefix for each locales. and then generate it via webtools or API.
Related issue(s)/PR(s)
#268