osu-framework
osu-framework copied to clipboard
Bindable.BindTo() is not thread-safe
Calling BindTo
on multiple threads can create the Bindings
list multiple times:
https://github.com/ppy/osu-framework/blob/ec4ba87c49f9a7b6756f62f7e1f664259123999a/osu.Framework/Bindables/Bindable.cs#L223-L227
~~At one point this used to be a Lazy
, but I think there was a reason for not using that, possibly performance related?~~ Turns out this was never a Lazy
o_O
Why should binables be thread safe? I think making it will only cause more trouble (and worse performance) because people will not take care to do multithreading properly - and I dont think binables are a tool suited to send data across threads.
Everything else about them is already thread-safe. We use them across thread occasionally and don't want them to fail in weird ways (like this case, where bindings could potentially be completely lost).
Being able to read data while another thread writes to it is not thread safe by definition. This is already met by the bindable being available on more than one thread. If the ValueChanged event is invoked on a thread that is not the subscribing thread, you will potentially mess with values that shouldn't be available to that thread. How is that thread safe?
If you have another scenario which is of concern to you please open a new issue.
Creating the Bindings
list unconditionally (before a BindTo
call) doesn't seem feasible.. The obvious solution would be lock
ing or maybe a SpinLock, since the lock times are short.
Is there some sort of big picture on how many bindings Bindables usually have? If the number is small in most cases, it could be feasible to use a fixed-size buffer before creating an expensive WeakList to try and reduce the performance impact the thread synchronization will have.