Cysharp / R3

The new future of dotnet/reactive and UniRx.
MIT License
2.1k stars 92 forks source link

potential Infinity loop with ReactiveProperty #159

Closed PandaHops closed 6 months ago

PandaHops commented 7 months ago

Hello @neuecc . First I would like to thank you for this amazing repository. we are using your old Unirx system and we have noticed a potential Infinity loop with ReactiveProperty. looking at the new code I don't think you have fixed it, so I thought I should let you know about it.

let say you have this code: image

and image

assume you have 3 calls calling to Func(). lets call them t1,t2,t3. (they are not really different threads but I think it will make it clear to understand it if we call them like this)

t1 is setting the _isBusy.Value to true. and waiting 1 sec. t2 and t3 is stuck in "_isBusy.WhenFalse()" (lines 106).

when t1 is changing the value back to false, t1 is looping on all the observables and releasing them (the reactive release while loop) (OnNextCore)

by doing so it first release t2 that change the value of _isBusy back to true. and t2 is now waiting .

t3 is now also release due to the fact that t1 is going over all the observables and releasing them (even thought this is already and error in my opnion)

t3 is now checking the loop condition again, the condition still true so it goes back to calling the "await _isBusy.WhenFalse();" it is now adding another item to the _isbusy ReactiveProperty list. lets call it t4.

since t1 is still in a loop and releasing observables it will release t4. and t4 in the same way that t3 created t4, will create a t5, that will also be released. and we got a busy infinity loop. sinice it is a busy dead lock. t2 will also never be released. and we got a full dead look.

what we did to solve it, is simply send the this.value instead of value in the OnNextCore (or RaiseOnNext as it is called in the old repository). i.e. node.Observer.OnNext(this.value);

let me know if I mist something but looking at the new ReactiveProperty code, it doesn't seems like this is solved.

neuecc commented 7 months ago

Thanks, I would like to solve this problem. But I honestly don't understand the explanation, so please give me the code to reproduce it first.

neuecc commented 6 months ago

I'm not entirely clear on the specific issues you mentioned, but I have made improvements to the iteration of ReactiveProperty. I hope the new implementation doesn't have any problems.