Synchronize access to System.ComponentModel.DataAnnotations.Validator
#2428
Conversation
|
@dotnet-policy-service agree |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2428 +/- ##
=======================================
Coverage 85.39% 85.39%
=======================================
Files 312 312
Lines 7464 7464
Branches 1121 1121
=======================================
Hits 6374 6374
Misses 905 905
Partials 185 185
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
It would be good to quantify what, if any, impact this has on performance by running the benchmarks before and after the change to see if there's any impact from introducing the lock (I'd hope not as it's on construction not execution).
Apply changes from App-vNext#2428.
|
I ran the benchmarks for main and then this PR on my laptop to get a consistent baseline to compare (diff) and as expected there's no discernible difference, it's just noise. Once some CI-related infrastructure changes are sorted out and merged, then I'll merge this 👍 |
|
That change has been made now - you just need to rebase/merge with main. |
|
Sorry, #2443 is also needed (which this PR flushed out). |
|
Thanks for your contribution @kmcclellan - the changes from this pull request have been published as part of version 8.5.1 📦, which is now available from NuGet.org 🚀 |
| @@ -7,6 +7,9 @@ namespace Polly.Utils; | |||
| [ExcludeFromCodeCoverage] | |||
| internal static class ValidationHelper | |||
| { | |||
| // Workaround for https://github.com/dotnet/runtime/issues/110917 | |||
| private static readonly object ValidatorLock = new(); | |||
There was a problem hiding this comment.
maybe use the Backport of the new System.Threading.Lock in a future update?
Pull Request
The issue or feature being addressed
[Bug]: InvalidCastException when multiple threads retrieve pipeline
Details on the issue fix or feature implementation
.NET team has targeted a fix in v10 of the runtime. In the meantime, I've opted for a simple workaround that synchronizes validation calls for all strategy options models (these are the only ones that use
RangeAttribute). Given this only occurs when a pipeline is first built, I don't feel like we need to be concerned about contention.Confirm the following