Bindable.BindTo() is not thread-safe

[smoogipoo]

trafficstars

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

[Flutterish]

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.

[peppy]

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).

[Flutterish]

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?

[peppy]

If you have another scenario which is of concern to you please open a new issue.

[FreezyLemon]

Creating the Bindings list unconditionally (before a BindTo call) doesn't seem feasible.. The obvious solution would be locking 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.