Prevent deadlocks on components and locale updates

[petersg83]

What does it do?

It uses sequential updates instead of Promise.all to avoid deadlocks

Why is it needed?

Because otherwise it can prevent to create a new locale for an entity or to save an entry with components

How to test it?

It's very difficult to test it as the deadlocks are random. Contact @Marc-Roig so he can show you how to :p

Related issue(s)/PR(s)

#15202

[codecov]

Codecov Report

Base: 61.29% // Head: 50.62% // Decreases project coverage by -10.68% ⚠️

Coverage data is based on head (70f188c) compared to base (62e3f93).
Patch coverage: 0.00% of modified lines in pull request are covered.

❗ Current head 70f188c differs from pull request most recent head c02061d. Consider uploading reports for the commit c02061d to get more accurate results

Additional details and impacted files

@@             Coverage Diff             @@
##             main   #15535       +/-   ##
===========================================
- Coverage   61.29%   50.62%   -10.68%     
===========================================
  Files        1355      297     -1058     
  Lines       33498    10474    -23024     
  Branches     6455     2324     -4131     
===========================================
- Hits        20532     5302    -15230     
+ Misses      11143     4272     -6871     
+ Partials     1823      900      -923     

Flag	Coverage Δ
back	50.62% <0.00%> (-0.01%)	⬇️
front	?
unit_back	50.62% <0.00%> (-0.01%)	⬇️
unit_front	?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...r/server/services/utils/configuration/metadatas.js	9.80% <0.00%> (-0.20%)	⬇️
...admin/src/content-manager/components/Hint/index.js
...n/src/content-manager/components/InputUID/index.js
...ntent-manager/components/PreviewWysiwyg/Wrapper.js
...in/src/content-manager/components/Wysiwyg/index.js
...min/src/utils/isAllowedContentTypesForRelations.js
...ges/MarketplacePage/components/SortSelect/index.js
...rc/content-manager/hooks/useKeyboardDragAndDrop.js
...ore/upload/admin/src/pages/SettingsPage/reducer.js
...dmin/src/content-manager/pages/ListView/actions.js
... and 1049 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[Boegie19]

users and permisisons also has one don't know if this should be in this pr or a different one?

strapi/packages/plugins/users-permissions/server/services/role.js

Lines 138 to 143 in 70f188c

	await Promise.all(
	toCreate.map((permissionInfo) =>
	strapi.query('plugin::users-permissions.permission').create({ data: permissionInfo })
	)
	);
	},

the #12346 pr also would fix this

[jhoward1994]

There are also some other uses of Promise.all() in the entity-service and i18n plugin:

strapi/packages/core/strapi/lib/services/entity-service/index.js

Lines 253 to 263 in 62e3f93

	const componentsToDelete = await Promise.all(
	entitiesToDelete.map((entityToDelete) => getComponents(uid, entityToDelete))
	);
	const deletedEntities = await db.query(uid).deleteMany(query);
	await Promise.all(
	componentsToDelete.map((compos) => deleteComponents(uid, compos, { loadComponents: false }))
	);
	// Trigger webhooks. One for each entity
	await Promise.all(entitiesToDelete.map((entity) => this.emitEvent(uid, ENTRY_DELETE, entity)));

strapi/packages/plugins/i18n/server/migrations/field/migrate.js

Lines 9 to 23 in bd201cd

	const migrateBatch = async (entries, { model, attributesToMigrate }, { transacting }) => {
	const { copyNonLocalizedAttributes } = getService('content-types');
	const updatePromises = entries.map((entity) => {
	const updateValues = pick(attributesToMigrate, copyNonLocalizedAttributes(model, entity));
	const entriesIdsToUpdate = entity.localizations.map(prop('id'));
	return Promise.all(
	entriesIdsToUpdate.map((id) =>
	strapi.query(model.uid).update({ id }, updateValues, { transacting })
	)
	);
	});
	await Promise.all(updatePromises);
	};

Could these lead to deadlocks? And if not should we change the syntax anyway for consistency?

[nathan-pichon]

LGTM as a quick fix.
But I'm persuaded that we should find a way to do ops in parallel as this can decrease performances over huge sets of data. Also, I think we need to mention this point in the release patch note.
Maybe we can put this element to be addressed in the v5 architecture spec process.

[petersg83]

Thank you for your reviews!

Yes, we let some Promise.all here and there in the code as we think they are not causing deadlocks. We prefer to wait for users to tell us if they still see deadlocks rather than slowing everything.

We just did another PR on the same part of the code to fix performance issues and it appears... it may also fix the deadlocks! So we would like to first merge #15554 and see if it fixes the deadlocks issues for users before merging this PR :)
In the meantime this PR is on hold then