-
-
Notifications
You must be signed in to change notification settings - Fork 96
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
Platform-dependent tests and ReactiveProperty race condition fix #230
Conversation
Fixed [ReactiveProperty race condition](Cysharp#228 (comment))
I understand that using a lock is the most reliable approach. |
That's understandable. It may occur like this:
And so, although the items were pushed like this:
What the observer sees is:
The 'Second' is left missing, and so observer thinks the current value is First. |
I think it is overkill to lock around the entire OnNext. If we put SubscribeCore's observer.OnNext in lock and then minimize the lock, get this. public void OnNext(T value)
{
ObserverNode? node;
ObserverNode? last;
OnValueChanging(ref value);
lock (this)
{
this.currentValue = value;
node = Volatile.Read(ref root);
last = node?.Previous;
}
OnValueChanged(value);
OnNextCore(value, node, last);
}
void OnNextCore(T value, ObserverNode? node, ObserverNode? last)
{
ThrowIfDisposed();
if (IsCompleted) return;
while (node != null)
{
node.Observer.OnNext(value);
if (node == last) return;
node = node.Next;
}
} However, as a worst-case scenario, it will break down if last is deleted by Dispose while OnNextCore is running. If we cannot tolerate that, we can divide the code into two systems: |
A thread-safe version of Our particular use case doesn't care about missing a value so much. We care about not having data tearing in value, or breaking the subscriber list. |
As explained in the Concurrency Policy section, the OnNext of Subject is not thread-safe for subsequent operators. Therefore, when updating Value in a multi-threaded environment, it must first be enclosed in a lock. |
I've updated ReadMe about this policy. |
@neucec I think this still leaves a bug in Would it be possible to add the fully-locked version as |
Yes, I thought about it a lot, but it is hard and difficult to lock everything! |
I'm curious, does |
Okay, I think it is better to implement this conservatively here, so I will also add lock in get. |
Fixed some tests, results of which were dependent on the platform used.
Fixed ReactiveProperty race condition
Also (seems to be) mentioned in #229