Fixed scoping issue with multiple itterations on the foreach #20100
Conversation
|
Hi there @TimoTielens, thank you for this contribution! 👍 While we wait for one of the Core Collaborators team to have a look at your work, we wanted to let you know about that we have a checklist for some of the things we will consider during review:
Don't worry if you got something wrong. We like to think of a pull request as the start of a conversation, we're happy to provide guidance on improving your contribution. If you realize that you might want to make some changes then you can do that by adding new commits to the branch you created for this work and pushing new commits. They should then automatically show up as updates to this pull request. Thanks, from your friendly Umbraco GitHub bot 🤖 🙂 |
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a scoping issue in the ContentVersionService where multiple iterations of document version cleanup would fail due to attempting to acquire a write lock multiple times within nested scopes.
- Moved the scope creation and write lock outside the foreach loop to prevent lock acquisition conflicts
- Reorganized the code structure to have a single scope encompassing all batch operations
- Cleaned up indentation and formatting of the notification publishing call
| using (ICoreScope scope = _scopeProvider.CreateCoreScope()) | ||
| { | ||
| scope.WriteLock(Constants.Locks.ContentTree); | ||
| var groupEnumerated = group.ToList(); | ||
| _documentVersionRepository.DeleteVersions(groupEnumerated.Select(x => x.VersionId)); | ||
|
|
||
| foreach (ContentVersionMeta version in groupEnumerated) | ||
| foreach (IEnumerable<ContentVersionMeta> group in versionsToDelete.InGroupsOf(Constants.Sql.MaxParameterCount)) | ||
| { | ||
| EventMessages messages = _eventMessagesFactory.Get(); | ||
| var groupEnumerated = group.ToList(); | ||
| _documentVersionRepository.DeleteVersions(groupEnumerated.Select(x => x.VersionId)); | ||
|
|
||
| scope.Notifications.Publish( | ||
| new ContentDeletedVersionsNotification(version.ContentId, messages, version.VersionId)); | ||
| } | ||
| foreach (ContentVersionMeta version in groupEnumerated) | ||
| { | ||
| EventMessages messages = _eventMessagesFactory.Get(); | ||
|
|
||
| scope.Notifications.Publish(new ContentDeletedVersionsNotification(version.ContentId, messages, version.VersionId)); | ||
| } | ||
| } | ||
| scope.Complete(); | ||
| } |
There was a problem hiding this comment.
The current implementation processes all version groups within a single scope, which could lead to very long-running transactions. For large datasets, this approach may cause database lock contention and timeout issues. Consider keeping the original per-batch scoping but fixing the lock acquisition issue differently, such as using a higher-level coordination mechanism.
Copilot uses AI. Check for mistakes.
|
I haven't been able to replicate the issue that the PR is proposing to fix I'm afraid @TimoTielens - I took a database with a few hundred versions and looped in groups of 3, just ensure there are multiple passes. Also, I note there's some discussion in the file above about deliberately using multiple scopes and locks to not prevent other activity going on in the CMS. Given that I'm reluctant to take this further. Are you able to provide clearer steps for replication that can show there's an issue with the |
Fixed scoping issues for cleaning up older content versions. When having large datasets (more then 2000 items)
Description
The CleanupDocumentVersions methods tries to clean up older versions of the content. The last step that actually performs the removal of the the contents versions is in an foreach. This foreach makes use of the scopelock. However there is an issue if we iterate multiple times over the logic. The second time we try to write the lock an exceptions occurs. By moving the lock outside the loop we fixed this.
Testing