Threading

[ashitikov]

Hello, @plivesey, thank you for the library!
Why does any work with addModelUpdatesListener and removeModelUpdatesListener should be done on main thread?
https://github.com/plivesey/ConsistencyManager-iOS/blob/master/ConsistencyManager/ConsistencyManager.swift#L416

I ask because I have an intention to write through the RocketData's provider setData(...) using background thread, and there is some logic around llisteningToModelIdentifier property. And so I have some crashes because array modelUpdatesListeners out of sync https://github.com/plivesey/ConsistencyManager-iOS/blob/master/ConsistencyManager/ConsistencyManager.swift#L102 .
On the other hand there is similar listeners array, and ConsistencyManager synchronize access through internal queue using dispatchTask(...).

Is there some valuable reason to access model updates listeners on main thread only?
If not I propose to use dispatchTask in similar way as work with listeners array. I can do a PR.

Thank you!

[plivesey]

The reason that it's on the main thread is because it's updated on the main thread. So, the model modelUpdatesListeners is read on the main thread, so should be written there too.

This is only used by the DataProvider for open var modelIdentifier: String?, so shouldn't affect data providers much.

However, I think there are more threading problems with data providers. Specifically, setData is not thread safe. The consistency manager always calls back on the main thread, so if you're calling setData from a background thread - there could be problems.

The library was mostly intended for UI to listen to changes to data. If you're doing any heavy processing on the data, you can always DispatchQueue.async from the delegate method since the models will be thread safe (because they're immutable).

Maybe one question is - why do you need to set data on a background thread vs just going back to the main thread for that?

[plivesey]

Yeah, looking back through the docs, I found: https://plivesey.github.io/RocketData/pages/110_threading.html

I think there's significant work to make data providers thread safe. Also, I'd want to make sure that code didn't affect performance for anyone who is using it on the main thread.

[ashitikov]

Well, in my case I have different points in application, which can only write (no read). For example when I get an update from network (say by subscription, which maintained by subscription manager), I just write from the same background thread, and expect an update on the other data provider on main thread. This works perfectly with CollectionDataProvider, but sometimes crashes for DataProvider by the out of sync reason. I guess you when you are talking about reading modelUpdatesListeners on main thread you mean this place: https://github.com/plivesey/ConsistencyManager-iOS/blob/master/ConsistencyManager/ConsistencyManager.swift#L685. I think we can read listeners here https://github.com/plivesey/ConsistencyManager-iOS/blob/master/ConsistencyManager/ConsistencyManager.swift#L662 and then use them inside main.async block to dispatch updates. What do you think?

[plivesey]

I think the fact that CollectionDataProvider is chance and is not fully correct. There are several points in the code which assumes the main thread and you're introducing several race conditions. These race conditions won't show up for most runs of the application though.

Reading them first and then dispatching also introduces a race condition. Consider (this pseudocode):

consistencyManager.addListener(self)
// ... later
// consistencyManager reads the current listeners
consistencyManager.removeListeners(self)
// consistencyManager gets the main thread and calls update
// Now, self gets a notification after removing themself

Maybe this race condition highlights the difficulty of supporting asynchronous work. It's not just about accessing on the right thread, it's also ensuring there are no race conditions.

[ashitikov]

Thank you for detailed explanation. Well, to me this would be a great feature to change read and write thread. Like an option, I can offer to pass desired queue for reading and writing in this way DataProvider -> ConsistencyManager. It seems there will not be many changes in code, but will be more flexible.