Thread-safety fixes (#53, #26) and .NET 10 build-warning cleanup (#46)#54
Conversation
Snapshot the listener list inside the lock before dispatching, so a concurrent Add/Remove/prune can no longer corrupt the enumeration. Adds a multi-threaded publish/add/remove stress test.
Resolve and cache under a lock so the Contains-then-Add in BuilderFor is atomic, and guard Start with double-checked locking. Fixes the 'An item with the key ... has already been added' crash under concurrent BuilderFor calls. Adds a multi-threaded reproduction test.
- SYSLIB0045: use MD5.Create() instead of HashAlgorithm.Create("MD5")
- SYSLIB0051: remove the dead legacy serialization constructor from OSPSuiteException (nothing in the suite derives from it)
- Replace deprecated PackageIconUrl with PackageIcon and ship logo.png in the package
- Build with 'dotnet build' instead of 'msbuild' in CI to clear the VS 2022 17.14 net10 targeting warning
|
Warning Review limit reached
More reviews will be available in 58 minutes and 29 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughTwo thread-safety bugs are fixed: ChangesThread-safety fixes for BuilderRepository and EventPublisher
Build warning fixes and CI migration
Sequence Diagram(s)sequenceDiagram
participant T1 as Publisher Thread
participant T2 as Mutator Thread
participant EP as EventPublisher
participant LL as _listeners (List)
T1->>EP: PublishEvent(event)
T2->>EP: AddListener(newListener)
EP->>LL: lock(_listeners): pruneReferences()
EP->>LL: ToList() → snapshot
note over EP,LL: lock released
T2->>LL: lock(_listeners): _listeners.Add(newListener)
T2->>EP: RemoveListener(newListener)
T2->>LL: lock(_listeners): _listeners.Remove(newListener)
loop over snapshot (no lock held)
EP->>EP: cast Target as IListener<T> → Handle(event)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/OSPSuite.Utility/FileHelper.cs (1)
190-201:⚠️ Potential issue | 🟡 MinorDispose the
MD5instance after hashing.At Line 190,
MD5.Create()returns a disposable object that is not disposed. With FileStreams properly wrapped inusingstatements in the same method, thehashAlgorithmshould follow the same pattern to release unmanaged crypto resources. This becomes especially relevant when the method is called repeatedly.Suggested fix
- var hashAlgorithm = MD5.Create(); + using var hashAlgorithm = MD5.Create(); byte[] hashFile1; byte[] hashFile2;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/OSPSuite.Utility/FileHelper.cs` around lines 190 - 201, The MD5.Create() call at the beginning returns a disposable object that is not being disposed, which wastes unmanaged crypto resources, especially when the method is called repeatedly. Wrap the hashAlgorithm variable in a using statement similar to how the FileStream instances are already wrapped in the method. Move the hashFile1 and hashFile2 variable declarations inside or before the using block so they remain accessible after the using statement closes, then place both the ComputeHash calls for both files within the using block to ensure the MD5 instance is properly disposed after both hashes are computed.
🧹 Nitpick comments (1)
src/OSPSuite.Utility/Events/EventPublisher.cs (1)
25-34: 💤 Low valueMinor: Redundant reentrant lock acquisition.
pruneReferences()internally callsdoWithinLock(), which acquireslock (_listeners)again while it's already held at line 26. C# locks are reentrant so this is safe, but it adds unnecessary overhead on each publish.Consider extracting the inner pruning logic to a
pruneReferencesCore()method that doesn't acquire the lock:♻️ Optional refactor
lock (_listeners) { - pruneReferences(); + pruneReferencesCore(); listeners = _listeners.ToList(); } ... -private void pruneReferences() -{ - doWithinLock(() => +private void pruneReferencesCore() +{ + for (var i = _listeners.Count - 1; i >= 0; i--) { - for (var i = _listeners.Count - 1; i >= 0; i--) + if (_listeners[i].Target == null) { - if (_listeners[i].Target == null) - { - _listeners.RemoveAt(i); - } + _listeners.RemoveAt(i); } - }); + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/OSPSuite.Utility/Events/EventPublisher.cs` around lines 25 - 34, The pruneReferences() method is being called while already holding the lock on _listeners, but pruneReferences() internally calls doWithinLock() which acquires the same lock again, causing redundant reentrant lock acquisition. Extract the actual pruning logic from the pruneReferences() method into a new pruneReferencesCore() method that does not acquire the lock, update pruneReferences() to call doWithinLock() with pruneReferencesCore() inside, and then change the call site in the lock block (around line 30) to call pruneReferencesCore() directly instead of pruneReferences() since the lock is already held.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/OSPSuite.Utility/FileHelper.cs`:
- Around line 190-201: The MD5.Create() call at the beginning returns a
disposable object that is not being disposed, which wastes unmanaged crypto
resources, especially when the method is called repeatedly. Wrap the
hashAlgorithm variable in a using statement similar to how the FileStream
instances are already wrapped in the method. Move the hashFile1 and hashFile2
variable declarations inside or before the using block so they remain accessible
after the using statement closes, then place both the ComputeHash calls for both
files within the using block to ensure the MD5 instance is properly disposed
after both hashes are computed.
---
Nitpick comments:
In `@src/OSPSuite.Utility/Events/EventPublisher.cs`:
- Around line 25-34: The pruneReferences() method is being called while already
holding the lock on _listeners, but pruneReferences() internally calls
doWithinLock() which acquires the same lock again, causing redundant reentrant
lock acquisition. Extract the actual pruning logic from the pruneReferences()
method into a new pruneReferencesCore() method that does not acquire the lock,
update pruneReferences() to call doWithinLock() with pruneReferencesCore()
inside, and then change the call site in the lock block (around line 30) to call
pruneReferencesCore() directly instead of pruneReferences() since the lock is
already held.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a00d20d6-3ab3-4293-9c4f-1b6c7cfe4118
⛔ Files ignored due to path filters (1)
logo.pngis excluded by!**/*.png
📒 Files selected for processing (9)
.github/workflows/build-and-publish.yml.github/workflows/build-pr.ymlsrc/OSPSuite.Utility/BuilderRepository.cssrc/OSPSuite.Utility/Events/EventPublisher.cssrc/OSPSuite.Utility/Exceptions/OSPSuiteException.cssrc/OSPSuite.Utility/FileHelper.cssrc/OSPSuite.Utility/OSPSuite.Utility.csprojtests/OSPSuite.Utility.Tests/BuilderRepositorySpecs.cstests/OSPSuite.Utility.Tests/EventPublisherSpecs.cs
💤 Files with no reviewable changes (1)
- src/OSPSuite.Utility/Exceptions/OSPSuiteException.cs
Wrap MD5.Create() in a using block so the crypto object is released after both file hashes are computed (addresses PR review).
pruneReferences is only called from PublishEvent, which now holds the lock on _listeners, so its own doWithinLock was a redundant reentrant acquire (addresses PR review).
2 participants
Bundles three independent OSPSuite.Utility fixes. Each is a separate commit.
#53 —
EventPublisher.PublishEventthread-safetyPublishEventpruned_listenersunder a lock but snapshotted it (.ToList()) outside the lock, so a concurrentAdd/Remove/prune could corrupt the enumeration (two concurrent publishers are enough). The fix takes the snapshot inside the lock and dispatches via_context.Sendoutside the lock, so handlers never run while the lock is held._context.Sendis intentionally left as-is — the blocking-marshal concern noted in the issue is explicitly not the core bug.#26 —
BuilderRepositorythread-safetyBuilderFordid a check-then-act across two cache calls (Contains→Add); two threads could both miss and bothAddthe same key, throwingAn item with the key '...' has already been added(root cause of PK-Sim #1260 under parallel qualification export). The fix resolves-and-caches under a private lock so the sequence is atomic, and guardsStart()with double-checked locking (_isInitializedis nowvolatile). No change toCacheis needed — the repository's cache is private and now only ever touched single-threaded.#46 — .NET 10 build warnings
HashAlgorithm.Create("MD5")→MD5.Create().OSPSuiteException. Verified by a tree-wide search that nothing in PK-Sim/MoBi/OSPSuite.Core (all on .NET 10) derives from it, so removal is safe — no suppression needed.PackageIconUrl: replaced withPackageIconand the canonical OSPlogo.pngis now shipped in the package (dotnet packemits<icon>with no NU5048).Targeting .NET 10.0 … in Visual Studio 2022 17.14 is not supported: CI now builds withdotnet buildinstead ofmsbuild(dropped themicrosoft/setup-msbuildstep) in both workflows.Testing
dotnet buildof the library reports 0 warnings;dotnet packproduces a package carryinglogo.pngwith no packaging warnings.Fixes #53
Fixes #26
Fixes #46
Summary by CodeRabbit
Improvements
Chores