#4649 Feature/4649 thread safe broken rules

[Bowman74]

#4649

@rockfordlhotka This PR is not ready to merge but I think it bears some discussion. The following points should be considered:

• Administratively is Main the right branch for this?
• This fix adds a new method to the BrokenRulesCollection called .ToThreadsafeList() It is behind the lock that is in use in BrokenRulesCollection that returns a copy of the broken rules collection. Because it is protected by the lock the ToList should usually (more on this later) be protected and any subsequent calls into the copy won't get the collection modified error from things like unfinished async rules that are running. That means that the user who calls this will have a pointer to a list that is a point in time representation of the BrokenRulesCollection.
• The public AddRange method has also been put behind the lock that was being used anywhere. It is not being used by CSLA but it will protect use by outside individuals from recreating the error.
• Other methods like GetFirstMessage, GetFirstBrokenRule, ToArray, some of the ToString could conceivably fall to the same problem. They use link statements on the list and could potentially be interrupted by an async rule finishing.
• Because the lock statement is reentrant things happening on the same thread cannot deadlock. It also appears that running async rules is done with .ConfigureAwait(false). This is probably good as it will not allow a completing async rule to interrupt what would potentially be the same thread that is currently calling the ToThreadsafeList method in the middle of the ToList() call. If that is not a safe assumption, then this code will fix the most likely cause of this error but there will still be a situation where the ToList could be interrupted mid stream by a completing async rule and the lock statement would not stop it as it would be the same thread.
• The BrokenRulesColleciton still inherits from the List and the Add and RemoveAt methods are newed up. As far as I know a user could directly start calling methods like Add against the List and completely bypass our lock protection. I don't know if users directly modifying the BrokenRulesColleciton behind CSLA's back is a supported scenario though.
• The tests failed as expected when the ToThreadsafeList() method simply returns a reference to the BrokenRuleCollection without locks as is what happens now when users are using the GetBrokenRules() method.

Right now, this PR is just for discussion on approach and also those Items I listed.

[rockfordlhotka]

Can we use something a little lighter than a lock statement? This contention can't happen that terribly often, and I suspect that reading the list happens way more often than writing. So maybe a SemaphoreSlim, or perhaps a reader-writer lock (if there's a slim version)?

Can the underlying collection be one of the concurrent types now available in .NET?

Merging into main is fine as long as this isn't a breaking change. We're currently building toward 10.1.0, which is a feature+fix release, but can't include breaking changes.

[Copilot]

Pull request overview

This PR addresses issue #4649 by adding thread-safety mechanisms to BrokenRulesCollection to prevent InvalidOperationException when async business rules complete during enumeration operations. The solution introduces a new ToThreadsafeList() method that returns a snapshot of the collection protected by the existing _syncRoot lock, and adds locking to the public AddRange() method.

Changes:

• Added ToThreadsafeList() method to BrokenRulesCollection that creates a thread-safe snapshot
• Added lock protection to the AddRange() method
• Added test to verify thread safety of the new method

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 10 comments.

File	Description
Source/Csla/Rules/BrokenRulesCollection.cs	Implements ToThreadsafeList() method and adds locking to AddRange() to prevent collection modification exceptions
Source/tests/Csla.test/BizRules/BusinessRuleTests.cs	Adds test class and test method to validate thread-safe enumeration with concurrent modifications

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

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.

[Bowman74]

This limitation was explained in the notes for the checkin

[Copilot]

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.

[Bowman74]

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.

[Bowman74]

Can we use something a little lighter than a lock statement? This contention can't happen that terribly often, and I suspect that reading the list happens way more often than writing. So maybe a SemaphoreSlim, or perhaps a reader-writer lock (if there's a slim version)?

Can the underlying collection be one of the concurrent types now available in .NET?

Merging into main is fine as long as this isn't a breaking change. We're currently building toward 10.1.0, which is a feature+fix release, but can't include breaking changes.

@rockfordlhotka,
I originally planned to use the SemephoreSlim but didn't due to the following reasons. I'm happy to do it but wanted to make you aware of this information in case it changes your view on if we should use the lock statement.

• The BrokenRulesCollection already used the lock statement to protect when adding and removing broken rules (inconsistently as noted in my PR notes). Multithreaded operations has clearly been a problem for this class in the past. If I made this change, it would mean changing the preexisting code to use SemephoreSlim instead of lock
• Nowhere in the CSLA codebase is SemephoreSlim used. I counted about 20 classes, including the BrokenRulesCollection, that currently uses the lock statement.

That's not to say I can't use SemephoreSlim but it starts begging the question if that means all the other lock statements should also be examined for the same reason.

The BrokenRulesCollection ultimately inherits from ObservableCollection for what I assume to be binding reasons after going through an inheritance chain running through several CSLA collection classes. That is of course not thread safe. If I changed what BrokenRulesCollection was inheriting from I assume it would be a braking change and also would need a lot of other code like implementing INotifyCollectionChanged and INotifyPropertyChanged. All possible solutions as well. I'm just not sure what would be given up if it no longer inherited from ReadOnlyObservableBindingList. It looks like mobile formatter code is in there and that is just in the first CSLA class down the inheritance chain.

I do have some other ways to do the test that I think may be better and guarantee to cause the error.

[rockfordlhotka]

Can we use something a little lighter than a lock statement? This contention can't happen that terribly often, and I suspect that reading the list happens way more often than writing. So maybe a SemaphoreSlim, or perhaps a reader-writer lock (if there's a slim version)?
Can the underlying collection be one of the concurrent types now available in .NET?
Merging into main is fine as long as this isn't a breaking change. We're currently building toward 10.1.0, which is a feature+fix release, but can't include breaking changes.

@rockfordlhotka, I originally planned to use the SemephoreSlim but didn't due to the following reasons. I'm happy to do it but wanted to make you aware of this information in case it changes your view on if we should use the lock statement.

• The BrokenRulesCollection already used the lock statement to protect when adding and removing broken rules (inconsistently as noted in my PR notes). Multithreaded operations has clearly been a problem for this class in the past. If I made this change, it would mean changing the preexisting code to use SemephoreSlim instead of lock
• Nowhere in the CSLA codebase is SemephoreSlim used. I counted about 20 classes, including the BrokenRulesCollection, that currently uses the lock statement.

That's not to say I can't use SemephoreSlim but it starts begging the question if that means all the other lock statements should also be examined for the same reason.

The BrokenRulesCollection ultimately inherits from ObservableCollection for what I assume to be binding reasons after going through an inheritance chain running through several CSLA collection classes. That is of course not thread safe. If I changed what BrokenRulesCollection was inheriting from I assume it would be a braking change and also would need a lot of other code like implementing INotifyCollectionChanged and INotifyPropertyChanged. All possible solutions as well. I'm just not sure what would be given up if it no longer inherited from ReadOnlyObservableBindingList. It looks like mobile formatter code is in there and that is just in the first CSLA class down the inheritance chain.

I do have some other ways to do the test that I think may be better and guarantee to cause the error.

That all makes sense to me, thank you. Let's stick with lock then.