Skip to content
22 changes: 20 additions & 2 deletions Source/Csla/Rules/BrokenRulesCollection.cs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
// <summary>A collection of currently broken rules.</summary>
//-----------------------------------------------------------------------

using System.Collections.ObjectModel;
using Csla.Properties;
using Csla.Serialization.Mobile;

Expand Down Expand Up @@ -446,8 +447,11 @@ public void AddRange(List<BrokenRule> list)
{
if (list is null)
throw new ArgumentNullException(nameof(list));
foreach (var item in list)
Add(item);
lock (_syncRoot)
{
foreach (var item in list)
Add(item);
}
Comment on lines +450 to +454
Copy link

Copilot AI Feb 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The AddRange method now acquires the _syncRoot lock and then calls Add() in a loop. However, the Add() method is declared with the 'new' keyword (line 144) rather than 'override', which means callers using the base class reference type could bypass this method and the associated counter updates. Additionally, since Add() is not protected by any lock internally, calling it within the lock doesn't actually provide thread safety for the Add operation itself - it only ensures AddRange iterations aren't interrupted. Consider reviewing whether Add() should also be protected by the lock or if there are scenarios where the base class Add() could be called directly.

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This limitation was explained in the notes for the checkin

}

/// <summary>
Expand Down Expand Up @@ -479,5 +483,19 @@ protected override void OnSetState(SerializationInfo info)
InformationCount = info.GetValue<int>("_infoCount");
base.OnSetState(info);
}

/// <summary>
/// Returns a thread-safe copy of the current broken rules.
/// This method should be used instead of ToList() when a copy of the
/// broken rules list is desired that
/// </summary>
/// <returns>A new <see cref="ReadOnlyCollection{BrokenRule}"/> containing the broken rules at the time of the call.</returns>
public ReadOnlyCollection<BrokenRule> ToThreadSafeList()
{
lock (_syncRoot)
{
return this.ToList().AsReadOnly();
}
Comment on lines +495 to +498
Copy link

Copilot AI Feb 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The method creates a complete copy of the collection using ToList() while holding the lock. For collections with many broken rules, this could hold the lock for a non-trivial duration, potentially blocking async rule completion callbacks. Consider whether this is acceptable for the use case, or if a more efficient snapshot mechanism (such as using an immutable collection internally) would be better for performance.

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bit outdated but I find it unlikely a broken rules collection would ever have enough items in here to make this an issue.

}
}
}
50 changes: 50 additions & 0 deletions Source/tests/Csla.test/BizRules/BusinessRuleTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,31 @@ public void DisableDataAnnotationsScan()
obj.IsValid.Should().BeTrue();
}

[TestMethod]
[TestCategory("ThreadSafety")]
public async Task ToThreadSafeList_WithConcurrentModification_DoesNotThrowException()
{
SetScanForDataAnnotations(true);

var portal = _testDIContext.ServiceProvider.GetRequiredService<IDataPortal<TestBusinessRule3>>();
var obj = portal.Create();

obj.FirstName = "";
obj.LastName = "";
var list = obj.GetBrokenRules().ToThreadSafeList();

foreach (var item in list)
{
await Task.Run(() =>
{
obj.FirstName = "Drop Dead";
obj.LastName = "Fred";
}).ConfigureAwait(false);
}

Assert.AreEqual(2, list.Count);
}

private void SetScanForDataAnnotations(bool enable)
{
var options = _testDIContext.ServiceProvider.GetRequiredService<CslaOptions>();
Expand Down Expand Up @@ -96,4 +121,29 @@ private void Create()
BusinessRules.CheckRules();
}
}

public class TestBusinessRule3 : BusinessBase<TestBusinessRule3>
{
public static readonly PropertyInfo<string> FirstNameProperty = RegisterProperty<string>(nameof(FirstName));
[Required]
public string FirstName
{
get => GetProperty(FirstNameProperty);
set => SetProperty(FirstNameProperty, value);
}

public static readonly PropertyInfo<string> LastNameProperty = RegisterProperty<string>(nameof(LastName));
[Required]
public string LastName
{
get => GetProperty(LastNameProperty);
set => SetProperty(LastNameProperty, value);
}

[Create]
private void Create()
{
BusinessRules.CheckRules();
}
}
}
Loading