Content/Media: Fix deadlock when performing certain operations in parallel (closes #21125) (#21526)

* Move MediaTree write lock before MediaSavingNotification to prevent deadlock

Fixes a deadlock that could occur when saving multiple media items in parallel
when a MediaSavingNotification handler acquires a MediaTree read lock. The
previous ordering allowed two threads to each acquire read locks in their
notification handlers, then both attempt to upgrade to write locks, causing
a classic lock upgrade deadlock in SQL Server.

By acquiring the write lock before publishing the notification, the deadlock
scenario is avoided. Since the write lock is lazy, it only materializes at the
database level when actual queries are made, so notification handlers doing
in-memory work won't hold the lock.

* Apply same fix to MediaService.Delete method

* Apply same fix to DeleteVersions, DeleteVersion, and Sort methods

* Apply same fix to ContentService methods

Move WriteLock before notifications in:
- Save (single and batch)
- Delete
- DeleteVersions
- DeleteVersion
- Copy

* Apply the same pattern to MemberService.

* Add integration tests to verify the fix.

---------

Co-authored-by: Andy Butland <abutland73@gmail.com>
This commit is contained in:
Laura Neto
2026-01-27 11:08:35 +01:00
committed by GitHub
co-authored by Andy Butland
parent 138818acde
commit 7d813667c3
4 changed files with 229 additions and 64 deletions
+12 -8
View File
@@ -1111,6 +1111,8 @@ public class ContentService : RepositoryService, IContentService
using (ICoreScope scope = ScopeProvider.CreateCoreScope())
{
scope.WriteLock(Constants.Locks.ContentTree);
var savingNotification = new ContentSavingNotification(content, eventMessages);
if (scope.Notifications.PublishCancelable(savingNotification))
{
@@ -1118,7 +1120,6 @@ public class ContentService : RepositoryService, IContentService
return OperationResult.Cancel(eventMessages);
}
scope.WriteLock(Constants.Locks.ContentTree);
userId ??= Constants.Security.SuperUserId;
if (content.HasIdentity == false)
@@ -1178,6 +1179,8 @@ public class ContentService : RepositoryService, IContentService
using (ICoreScope scope = ScopeProvider.CreateCoreScope())
{
scope.WriteLock(Constants.Locks.ContentTree);
var savingNotification = new ContentSavingNotification(contentsA, eventMessages);
if (scope.Notifications.PublishCancelable(savingNotification))
{
@@ -1185,7 +1188,6 @@ public class ContentService : RepositoryService, IContentService
return OperationResult.Cancel(eventMessages);
}
scope.WriteLock(Constants.Locks.ContentTree);
foreach (IContent content in contentsA)
{
if (content.HasIdentity == false)
@@ -2297,14 +2299,14 @@ public class ContentService : RepositoryService, IContentService
using (ICoreScope scope = ScopeProvider.CreateCoreScope())
{
scope.WriteLock(Constants.Locks.ContentTree);
if (scope.Notifications.PublishCancelable(new ContentDeletingNotification(content, eventMessages)))
{
scope.Complete();
return OperationResult.Cancel(eventMessages);
}
scope.WriteLock(Constants.Locks.ContentTree);
// if it's not trashed yet, and published, we should unpublish
// but... Unpublishing event makes no sense (not going to cancel?) and no need to save
// just raise the event
@@ -2368,6 +2370,8 @@ public class ContentService : RepositoryService, IContentService
using (ICoreScope scope = ScopeProvider.CreateCoreScope())
{
scope.WriteLock(Constants.Locks.ContentTree);
var deletingVersionsNotification =
new ContentDeletingVersionsNotification(id, evtMsgs, dateToRetain: versionDate);
if (scope.Notifications.PublishCancelable(deletingVersionsNotification))
@@ -2376,7 +2380,6 @@ public class ContentService : RepositoryService, IContentService
return;
}
scope.WriteLock(Constants.Locks.ContentTree);
_documentRepository.DeleteVersions(id, versionDate);
scope.Notifications.Publish(
@@ -2402,6 +2405,8 @@ public class ContentService : RepositoryService, IContentService
using (ICoreScope scope = ScopeProvider.CreateCoreScope())
{
scope.WriteLock(Constants.Locks.ContentTree);
var deletingVersionsNotification = new ContentDeletingVersionsNotification(id, evtMsgs, versionId);
if (scope.Notifications.PublishCancelable(deletingVersionsNotification))
{
@@ -2415,7 +2420,6 @@ public class ContentService : RepositoryService, IContentService
DeleteVersions(id, content?.UpdateDate ?? DateTime.UtcNow, userId);
}
scope.WriteLock(Constants.Locks.ContentTree);
IContent? c = _documentRepository.Get(id);
// don't delete the current or published version
@@ -2738,6 +2742,8 @@ public class ContentService : RepositoryService, IContentService
using (ICoreScope scope = ScopeProvider.CreateCoreScope())
{
scope.WriteLock(Constants.Locks.ContentTree);
TryGetParentKey(parentId, out Guid? parentKey);
if (scope.Notifications.PublishCancelable(new ContentCopyingNotification(content, copy, parentId, parentKey, eventMessages)))
{
@@ -2750,8 +2756,6 @@ public class ContentService : RepositoryService, IContentService
// meaning that the event has to trigger for every copied content including descendants
var copies = new List<Tuple<IContent, IContent>>();
scope.WriteLock(Constants.Locks.ContentTree);
// a copy is not published (but not really unpublishing either)
// update the create author and last edit author
if (copy.Published)
+16 -18
View File
@@ -802,6 +802,8 @@ namespace Umbraco.Cms.Core.Services
using (ICoreScope scope = ScopeProvider.CreateCoreScope())
{
scope.WriteLock(Constants.Locks.MediaTree);
var savingNotification = new MediaSavingNotification(media, eventMessages);
if (scope.Notifications.PublishCancelable(savingNotification))
{
@@ -829,7 +831,6 @@ namespace Umbraco.Cms.Core.Services
new OperationResult(OperationResultType.FailedInvalidKey, eventMessages));
}
scope.WriteLock(Constants.Locks.MediaTree);
if (media.HasIdentity == false)
{
if (_entityRepository.Get(media.Key, UmbracoObjectTypes.Media.GetGuid()) is not null)
@@ -869,6 +870,8 @@ namespace Umbraco.Cms.Core.Services
using (ICoreScope scope = ScopeProvider.CreateCoreScope())
{
scope.WriteLock(Constants.Locks.MediaTree);
var savingNotification = new MediaSavingNotification(mediasA, messages);
if (scope.Notifications.PublishCancelable(savingNotification))
{
@@ -877,8 +880,6 @@ namespace Umbraco.Cms.Core.Services
}
IEnumerable<TreeChange<IMedia>> treeChanges = mediasA.Select(x => new TreeChange<IMedia>(x, TreeChangeTypes.RefreshNode));
scope.WriteLock(Constants.Locks.MediaTree);
foreach (IMedia media in mediasA)
{
if (media.HasIdentity == false)
@@ -915,14 +916,14 @@ namespace Umbraco.Cms.Core.Services
using (ICoreScope scope = ScopeProvider.CreateCoreScope())
{
scope.WriteLock(Constants.Locks.MediaTree);
if (scope.Notifications.PublishCancelable(new MediaDeletingNotification(media, messages)))
{
scope.Complete();
return OperationResult.Attempt.Cancel(messages);
}
scope.WriteLock(Constants.Locks.MediaTree);
DeleteLocked(scope, media, messages);
scope.Notifications.Publish(new MediaTreeChangeNotification(media, TreeChangeTypes.Remove, messages));
@@ -983,17 +984,17 @@ namespace Umbraco.Cms.Core.Services
{
EventMessages evtMsgs = EventMessagesFactory.Get();
if (wlock)
{
scope.WriteLock(Constants.Locks.MediaTree);
}
var deletingVersionsNotification = new MediaDeletingVersionsNotification(id, evtMsgs, dateToRetain: versionDate);
if (scope.Notifications.PublishCancelable(deletingVersionsNotification))
{
return;
}
if (wlock)
{
scope.WriteLock(Constants.Locks.MediaTree);
}
_mediaRepository.DeleteVersions(id, versionDate);
scope.Notifications.Publish(new MediaDeletedVersionsNotification(id, evtMsgs, dateToRetain: versionDate).WithStateFrom(deletingVersionsNotification));
@@ -1013,6 +1014,8 @@ namespace Umbraco.Cms.Core.Services
EventMessages evtMsgs = EventMessagesFactory.Get();
using ICoreScope scope = ScopeProvider.CreateCoreScope();
scope.WriteLock(Constants.Locks.MediaTree);
var deletingVersionsNotification = new MediaDeletingVersionsNotification(id, evtMsgs, specificVersion: versionId);
if (scope.Notifications.PublishCancelable(deletingVersionsNotification))
{
@@ -1025,13 +1028,9 @@ namespace Umbraco.Cms.Core.Services
IMedia? media = GetVersion(versionId);
if (media is not null)
{
DeleteVersions(scope, true, id, media.UpdateDate, userId);
DeleteVersions(scope, false, id, media.UpdateDate, userId);
}
}
else
{
scope.WriteLock(Constants.Locks.MediaTree);
}
_mediaRepository.DeleteVersion(versionId);
@@ -1275,6 +1274,8 @@ namespace Umbraco.Cms.Core.Services
using (ICoreScope scope = ScopeProvider.CreateCoreScope())
{
scope.WriteLock(Constants.Locks.MediaTree);
var savingNotification = new MediaSavingNotification(itemsA, messages);
if (scope.Notifications.PublishCancelable(savingNotification))
{
@@ -1283,8 +1284,6 @@ namespace Umbraco.Cms.Core.Services
}
var saved = new List<IMedia>();
scope.WriteLock(Constants.Locks.MediaTree);
var sortOrder = 0;
foreach (IMedia media in itemsA)
@@ -1509,6 +1508,5 @@ namespace Umbraco.Cms.Core.Services
}
#endregion
}
}
+6 -5
View File
@@ -818,6 +818,8 @@ namespace Umbraco.Cms.Core.Services
EventMessages evtMsgs = EventMessagesFactory.Get();
using ICoreScope scope = ScopeProvider.CreateCoreScope();
scope.WriteLock(Constants.Locks.MemberTree);
MemberSavingNotification? savingNotification = null;
if (publishNotificationSaveOptions.HasFlag(PublishNotificationSaveOptions.Saving))
{
@@ -836,8 +838,6 @@ namespace Umbraco.Cms.Core.Services
var previousUsername = _memberRepository.Get(member.Id)?.Username;
scope.WriteLock(Constants.Locks.MemberTree);
_memberRepository.Save(member);
if (publishNotificationSaveOptions.HasFlag(PublishNotificationSaveOptions.Saved))
@@ -876,6 +876,8 @@ namespace Umbraco.Cms.Core.Services
EventMessages evtMsgs = EventMessagesFactory.Get();
using ICoreScope scope = ScopeProvider.CreateCoreScope();
scope.WriteLock(Constants.Locks.MemberTree);
var savingNotification = new MemberSavingNotification(membersA, evtMsgs);
if (scope.Notifications.PublishCancelable(savingNotification))
{
@@ -883,8 +885,6 @@ namespace Umbraco.Cms.Core.Services
return OperationResult.Attempt.Cancel(evtMsgs);
}
scope.WriteLock(Constants.Locks.MemberTree);
foreach (IMember member in membersA)
{
//trimming username and email to make sure we have no trailing space
@@ -958,6 +958,8 @@ namespace Umbraco.Cms.Core.Services
EventMessages evtMsgs = EventMessagesFactory.Get();
using ICoreScope scope = ScopeProvider.CreateCoreScope();
scope.WriteLock(Constants.Locks.MemberTree);
var deletingNotification = new MemberDeletingNotification(member, evtMsgs);
if (scope.Notifications.PublishCancelable(deletingNotification))
{
@@ -965,7 +967,6 @@ namespace Umbraco.Cms.Core.Services
return OperationResult.Attempt.Cancel(evtMsgs);
}
scope.WriteLock(Constants.Locks.MemberTree);
DeleteLocked(scope, member, evtMsgs, deletingNotification.State);
Audit(AuditType.Delete, userId, member.Id);
@@ -1,15 +1,16 @@
// Copyright (c) Umbraco.
// See LICENSE for more details.
using System.Linq;
using System.Reflection;
using NUnit.Framework;
using Umbraco.Cms.Core;
using Umbraco.Cms.Core.Events;
using Umbraco.Cms.Core.Models;
using Umbraco.Cms.Core.Notifications;
using Umbraco.Cms.Core.Services;
using Umbraco.Cms.Tests.Common.Builders;
using Umbraco.Cms.Tests.Common.Testing;
using Umbraco.Cms.Tests.Integration.Attributes;
using Umbraco.Cms.Tests.Integration.Testing;
namespace Umbraco.Cms.Tests.Integration.Umbraco.Infrastructure.Services;
@@ -61,10 +62,10 @@ internal sealed class MediaServiceTests : UmbracoIntegrationTest
}
[Test]
public void Can_Update_Media_Property_Values()
public async Task Can_Update_Media_Property_Values()
{
IMediaType mediaType = MediaTypeBuilder.CreateSimpleMediaType("test", "Test");
MediaTypeService.Save(mediaType);
await MediaTypeService.CreateAsync(mediaType, Constants.Security.SuperUserKey);
IMedia media = MediaBuilder.CreateSimpleMedia(mediaType, "hello", -1);
media.SetValue("title", "title of mine");
media.SetValue("bodyText", "hello world");
@@ -122,12 +123,12 @@ internal sealed class MediaServiceTests : UmbracoIntegrationTest
}
[Test]
public void Get_Paged_Children_With_Media_Type_Filter()
public async Task Get_Paged_Children_With_Media_Type_Filter()
{
var mediaType1 = MediaTypeBuilder.CreateImageMediaType("Image2");
MediaTypeService.Save(mediaType1);
await MediaTypeService.CreateAsync(mediaType1, Constants.Security.SuperUserKey);
var mediaType2 = MediaTypeBuilder.CreateImageMediaType("Image3");
MediaTypeService.Save(mediaType2);
await MediaTypeService.CreateAsync(mediaType2, Constants.Security.SuperUserKey);
for (var i = 0; i < 10; i++)
{
@@ -214,38 +215,22 @@ internal sealed class MediaServiceTests : UmbracoIntegrationTest
}
[Test]
public void Cannot_Save_Media_With_Empty_Name()
public async Task Cannot_Save_Media_With_Empty_Name()
{
// Arrange
var mediaType = MediaTypeBuilder.CreateNewMediaType();
MediaTypeService.Save(mediaType);
await MediaTypeService.CreateAsync(mediaType, Constants.Security.SuperUserKey);
var media = MediaService.CreateMedia(string.Empty, -1, Constants.Conventions.MediaTypes.VideoAlias);
// Act & Assert
Assert.Throws<ArgumentException>(() => MediaService.Save(media));
}
// [Test]
// public void Ensure_Content_Xml_Created()
// {
// var mediaType = MediaTypeBuilder.CreateVideoMediaType();
// MediaTypeService.Save(mediaType);
// var media = MediaService.CreateMedia("Test", -1, Constants.Conventions.MediaTypes.VideoAlias);
//
// MediaService.Save(media);
//
// using (var scope = ScopeProvider.CreateScope())
// {
// Assert.IsTrue(scope.Database.Exists<ContentXmlDto>(media.Id));
// }
// }
[Test]
public void Can_Get_Media_By_Path()
public async Task Can_Get_Media_By_Path()
{
var mediaType = MediaTypeBuilder.CreateImageMediaType("Image2");
MediaTypeService.Save(mediaType);
await MediaTypeService.CreateAsync(mediaType, Constants.Security.SuperUserKey);
var media = MediaBuilder.CreateMediaImage(mediaType, -1);
MediaService.Save(media);
@@ -258,10 +243,10 @@ internal sealed class MediaServiceTests : UmbracoIntegrationTest
}
[Test]
public void Can_Get_Media_With_Crop_By_Path()
public async Task Can_Get_Media_With_Crop_By_Path()
{
var mediaType = MediaTypeBuilder.CreateImageMediaTypeWithCrop("Image2");
MediaTypeService.Save(mediaType);
await MediaTypeService.CreateAsync(mediaType, Constants.Security.SuperUserKey);
var media = MediaBuilder.CreateMediaImageWithCrop(mediaType, -1);
MediaService.Save(media);
@@ -274,10 +259,10 @@ internal sealed class MediaServiceTests : UmbracoIntegrationTest
}
[Test]
public void Can_Get_Paged_Children()
public async Task Can_Get_Paged_Children()
{
var mediaType = MediaTypeBuilder.CreateImageMediaType("Image2");
MediaTypeService.Save(mediaType);
await MediaTypeService.CreateAsync(mediaType, Constants.Security.SuperUserKey);
for (var i = 0; i < 10; i++)
{
var c1 = MediaBuilder.CreateMediaImage(mediaType, -1);
@@ -295,10 +280,10 @@ internal sealed class MediaServiceTests : UmbracoIntegrationTest
}
[Test]
public void Can_Get_Paged_Children_Dont_Get_Descendants()
public async Task Can_Get_Paged_Children_Dont_Get_Descendants()
{
var mediaType = MediaTypeBuilder.CreateImageMediaType("Image2");
MediaTypeService.Save(mediaType);
await MediaTypeService.CreateAsync(mediaType, Constants.Security.SuperUserKey);
// Only add 9 as we also add a folder with children.
for (var i = 0; i < 9; i++)
@@ -308,7 +293,7 @@ internal sealed class MediaServiceTests : UmbracoIntegrationTest
}
var mediaTypeForFolder = MediaTypeBuilder.CreateImageMediaType("Folder2");
MediaTypeService.Save(mediaTypeForFolder);
await MediaTypeService.CreateAsync(mediaTypeForFolder, Constants.Security.SuperUserKey);
var mediaFolder = MediaBuilder.CreateMediaFolder(mediaTypeForFolder, -1);
MediaService.Save(mediaFolder);
for (var i = 0; i < 10; i++)
@@ -364,4 +349,181 @@ internal sealed class MediaServiceTests : UmbracoIntegrationTest
return new Tuple<IMedia, IMedia, IMedia, IMedia, IMedia>(folder, folder2, image, folderTrashed, imageTrashed);
}
#region Concurrency Tests
public static void ConfigureConcurrencyTest(IUmbracoBuilder builder) =>
builder.AddNotificationHandler<MediaSavingNotification, ReadLockAcquiringMediaSavingHandler>();
/// <summary>
/// Verifies that parallel media saves don't deadlock when a notification handler acquires a read lock.
/// </summary>
/// <remarks>
/// Before the fix (issue #21125), this test would deadlock because:
/// 1. Thread A publishes MediaSavingNotification, handler calls GetById (acquires read lock).
/// 2. Thread B publishes MediaSavingNotification, handler calls GetById (acquires read lock).
/// 3. Thread A tries to acquire write lock - blocked waiting for Thread B's read lock.
/// 4. Thread B tries to acquire write lock - blocked waiting for Thread A's read lock.
/// = Deadlock
/// After the fix, write locks are acquired before publishing notifications, so the deadlock cannot occur.
/// </remarks>
[Test]
[Timeout(10000)]
[ConfigureBuilder(ActionName = nameof(ConfigureConcurrencyTest))]
public async Task Parallel_Media_Save_Does_Not_Deadlock_When_Notification_Handler_Acquires_Read_Lock()
{
// Arrange
var mediaType = MediaTypeBuilder.CreateSimpleMediaType("testMedia", "Test Media");
await MediaTypeService.CreateAsync(mediaType, Constants.Security.SuperUserKey);
const int numberOfMediaItems = 5;
var mediaItems = new List<IMedia>();
// Create media items first so they have an identity and will be handled by the notification handler.
for (var i = 0; i < numberOfMediaItems; i++)
{
var media = MediaBuilder.CreateSimpleMedia(mediaType, $"Test Media {i}", Constants.System.Root);
MediaService.Save(media);
mediaItems.Add(media);
}
var exceptions = new List<Exception>();
var lockObj = new Lock();
// Act - update all media items in parallel.
var tasks = mediaItems.Select(media => RunWithSuppressedExecutionContext(() =>
{
try
{
media.Name += " Updated";
MediaService.Save(media);
}
catch (Exception ex)
{
lock (lockObj)
{
exceptions.Add(ex);
}
}
return Task.CompletedTask;
})).ToList();
await Task.WhenAll(tasks);
// Assert
Assert.IsEmpty(
exceptions,
$"Expected no exceptions but got {exceptions.Count}: {string.Join(", ", exceptions.Select(e => e.Message))}");
// Verify all media items were updated successfully
foreach (var media in mediaItems)
{
var retrieved = MediaService.GetById(media.Id);
Assert.That(retrieved, Is.Not.Null, $"Media '{media.Name}' should be retrievable after save");
Assert.That(retrieved!.Name, Does.EndWith("Updated"), $"Media should have been updated");
}
}
/// <summary>
/// Verifies that parallel media deletes don't deadlock when a notification handler is registered.
/// </summary>
[Test]
[Timeout(10000)]
[ConfigureBuilder(ActionName = nameof(ConfigureConcurrencyTest))]
public async Task Parallel_Media_Delete_Does_Not_Deadlock()
{
// Arrange
var mediaType = MediaTypeBuilder.CreateSimpleMediaType("testMedia", "Test Media");
await MediaTypeService.CreateAsync(mediaType, Constants.Security.SuperUserKey);
const int numberOfMediaItems = 5;
var mediaItems = new List<IMedia>();
// Create media items.
for (var i = 0; i < numberOfMediaItems; i++)
{
var media = MediaBuilder.CreateSimpleMedia(mediaType, $"Test Media {i}", Constants.System.Root);
MediaService.Save(media);
mediaItems.Add(media);
}
var exceptions = new List<Exception>();
var lockObj = new Lock();
// Act - delete all media items in parallel.
var tasks = mediaItems.Select(media => RunWithSuppressedExecutionContext(() =>
{
try
{
MediaService.Delete(media);
}
catch (Exception ex)
{
lock (lockObj)
{
exceptions.Add(ex);
}
}
return Task.CompletedTask;
})).ToList();
await Task.WhenAll(tasks);
// Assert
Assert.IsEmpty(
exceptions,
$"Expected no exceptions but got {exceptions.Count}: {string.Join(", ", exceptions.Select(e => e.Message))}");
// Verify all media items were deleted
foreach (var media in mediaItems)
{
var retrieved = MediaService.GetById(media.Id);
Assert.That(retrieved, Is.Null, $"Media '{media.Name}' should have been deleted");
}
}
private static Task RunWithSuppressedExecutionContext(Func<Task> action)
{
using (ExecutionContext.SuppressFlow())
{
return Task.Run(action);
}
}
/// <summary>
/// A notification handler that acquires a read lock by calling MediaService.GetById.
/// This simulates real-world scenarios where handlers need to read related data.
/// </summary>
/// <remarks>
/// Before the fix for issue #21125, this handler would cause deadlocks when multiple
/// media items are saved in parallel because:
/// 1. The notification is published BEFORE the write lock is acquired
/// 2. This handler calls GetById which acquires a read lock
/// 3. Multiple threads each hold read locks and then try to upgrade to write locks
/// 4. SQL Server detects this as a deadlock; SQLite hangs indefinitely
/// </remarks>
internal sealed class ReadLockAcquiringMediaSavingHandler : INotificationHandler<MediaSavingNotification>
{
private readonly IMediaService _mediaService;
public ReadLockAcquiringMediaSavingHandler(IMediaService mediaService) =>
_mediaService = mediaService;
public void Handle(MediaSavingNotification notification)
{
foreach (var media in notification.SavedEntities)
{
// This call acquires a read lock on MediaTree.
// Before the fix, this could cause deadlocks when combined with parallel saves.
if (media.HasIdentity)
{
_mediaService.GetById(media.Id);
}
}
}
}
#endregion
}