Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: POCOObservableForProperty having a race condition #2020

Merged
merged 1 commit into from May 8, 2019

Conversation

Projects
None yet
2 participants
@worldbeater
Copy link
Member

commented May 8, 2019

What kind of change does this PR introduce?

Fixes the POCOObservableForProperty.GetNotificationForProperty concurrent access error discovered at reactiveui/ReactiveUI.Validation#21

What is the current behavior?

When running unit tests in parallel in ReactiveUI.Validation.Tests project, some of the tests fail randomly with the InvalidOperationException. The same thing happened on Azure Pipelines CI, but only on Mac machine with 1 build of 4 total.

System.InvalidOperationException : Operations that change non-concurrent collections must have exclusive access. A concurrent update was performed on this collection and corrupted its state. The collection's state is no longer correct.
   at System.Collections.Generic.Dictionary`2.TryInsert(TKey key, TValue value, InsertionBehavior behavior)
   at ReactiveUI.POCOObservableForProperty.GetNotificationForProperty(Object sender, Expression expression, String propertyName, Boolean beforeChanged) in D:\a\1\s\src\ReactiveUI\ObservableForProperty\POCOObservableForProperty.cs:line 39
   at ReactiveUI.ReactiveNotifyPropertyChangedMixin.NotifyForProperty(Object sender, Expression expression, Boolean beforeChange) in D:\a\1\s\src\ReactiveUI\Mixins\ReactiveNotifyPropertyChangedMixin.cs:line 206
   at ReactiveUI.ReactiveNotifyPropertyChangedMixin.NestedObservedChanges(Expression expression, IObservedChange`2 sourceChange, Boolean beforeChange) in D:\a\1\s\src\ReactiveUI\Mixins\ReactiveNotifyPropertyChangedMixin.cs:line 190
   at System.Reactive.Linq.ObservableImpl.Select`2.Selector._.OnNext(TSource value) in D:\a\1\s\Rx.NET\Source\src\System.Reactive\Linq\Observable\Select.cs:line 39
at System.Collections.Generic.Dictionary`2.FindEntry(TKey key)
   at ReactiveUI.POCOObservableForProperty.GetNotificationForProperty(Object sender, Expression expression, String propertyName, Boolean beforeChanged) in D:\a\1\s\src\ReactiveUI\ObservableForProperty\POCOObservableForProperty.cs:line 33
   at ReactiveUI.ReactiveNotifyPropertyChangedMixin.NestedObservedChanges(Expression expression, IObservedChange`2 sourceChange, Boolean beforeChange) in D:\a\1\s\src\ReactiveUI\Mixins\ReactiveNotifyPropertyChangedMixin.cs:line 190
   at System.Reactive.Linq.ObservableImpl.Select`2.Selector._.OnNext(TSource value) in D:\a\1\s\Rx.NET\Source\src\System.Reactive\Linq\Observable\Select.cs:line 39

The POCOObservableForProperty internal static Dictionary seems to be the cause.

public class POCOObservableForProperty : ICreatesObservableForProperty
{
private static readonly Dictionary<(Type, string), bool> hasWarned = new Dictionary<(Type, string), bool>();

What is the new behavior?

Now, the Dictionary is replaced with ConcurrentDictionary, which can handle the race condition.

What might this PR break?

Nothing.

@worldbeater worldbeater added the bug label May 8, 2019

@worldbeater worldbeater marked this pull request as ready for review May 8, 2019

@worldbeater worldbeater requested a review from reactiveui/core-team as a code owner May 8, 2019

@glennawatson glennawatson changed the title Use ConcurrentDict in POCO observable internals fix: POCOObservableForProperty having a race condition May 8, 2019

@codecov

This comment has been minimized.

Copy link

commented May 8, 2019

Codecov Report

Merging #2020 into master will increase coverage by 0.04%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2020      +/-   ##
==========================================
+ Coverage   60.74%   60.78%   +0.04%     
==========================================
  Files         115      115              
  Lines        4580     4580              
  Branches      656      656              
==========================================
+ Hits         2782     2784       +2     
+ Misses       1617     1614       -3     
- Partials      181      182       +1
Impacted Files Coverage Δ
...ObservableForProperty/POCOObservableForProperty.cs 100% <100%> (ø) ⬆️
src/ReactiveUI.Winforms/RoutedViewHost.cs 90.27% <0%> (+2.77%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a941bda...3e7fe80. Read the comment docs.

@glennawatson glennawatson merged commit f0b78c4 into master May 8, 2019

6 checks passed

ReactiveUI-CI Build #9.14.3+acb917709a succeeded
Details
ReactiveUI-CI (Mac) Mac succeeded
Details
ReactiveUI-CI (Windows) Windows succeeded
Details
codecov/patch 100% of diff hit (target 60.74%)
Details
codecov/project 60.78% (+0.04%) compared to a941bda
Details
license/cla All CLA requirements met.

@delete-merged-branch delete-merged-branch bot deleted the poco-observable-concurrent branch May 8, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.