{"action":"created","issue":{"url":"https://api.github.com/repos/facebook/componentkit/issues/71","labels_url":"https://api.github.com/repos/facebook/componentkit/issues/71/labels{/name}","comments_url":"https://api.github.com/repos/facebook/componentkit/issues/71/comments","events_url":"https://api.github.com/repos/facebook/componentkit/issues/71/events","html_url":"https://github.com/facebook/componentkit/pull/71","id":66925123,"number":71,"title":"Replace CKComponentDataSource queue with atomic counter","user":{"login":"kastiglione","id":43429,"avatar_url":"https://avatars.githubusercontent.com/u/43429?v=3","gravatar_id":"","url":"https://api.github.com/users/kastiglione","html_url":"https://github.com/kastiglione","followers_url":"https://api.github.com/users/kastiglione/followers","following_url":"https://api.github.com/users/kastiglione/following{/other_user}","gists_url":"https://api.github.com/users/kastiglione/gists{/gist_id}","starred_url":"https://api.github.com/users/kastiglione/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/kastiglione/subscriptions","organizations_url":"https://api.github.com/users/kastiglione/orgs","repos_url":"https://api.github.com/users/kastiglione/repos","events_url":"https://api.github.com/users/kastiglione/events{/privacy}","received_events_url":"https://api.github.com/users/kastiglione/received_events","type":"User","site_admin":false},"labels":[],"state":"closed","locked":false,"assignee":null,"milestone":null,"comments":10,"created_at":"2015-04-07T16:09:35Z","updated_at":"2015-04-08T17:43:22Z","closed_at":"2015-04-07T20:01:14Z","pull_request":{"url":"https://api.github.com/repos/facebook/componentkit/pulls/71","html_url":"https://github.com/facebook/componentkit/pull/71","diff_url":"https://github.com/facebook/componentkit/pull/71.diff","patch_url":"https://github.com/facebook/componentkit/pull/71.patch"},"body":"This change would make this aspect of `CKComponentDataSource` thread safe.\r\n\r\nFrom my reading, the queue here currently fills two needs:\r\n\r\n1. Assert that `-enqueueBatch:block:` processes batches serially.\r\n2. Track whether changes are actively being processed (`isComputingChanges`)\r\n\r\nThe first item isn't necessary, `-enqueueBatch:` uses a serial dispatch queue. Unless there are plans/desires to make the preparation queue operate in parallel, then the first item is essentially asserting that dispatch queues work.\r\n\r\nThe second item can be implemented with an atomic counter."},"comment":{"url":"https://api.github.com/repos/facebook/componentkit/issues/comments/90985292","html_url":"https://github.com/facebook/componentkit/pull/71#issuecomment-90985292","issue_url":"https://api.github.com/repos/facebook/componentkit/issues/71","id":90985292,"user":{"login":"benlodotcom","id":313959,"avatar_url":"https://avatars.githubusercontent.com/u/313959?v=3","gravatar_id":"","url":"https://api.github.com/users/benlodotcom","html_url":"https://github.com/benlodotcom","followers_url":"https://api.github.com/users/benlodotcom/followers","following_url":"https://api.github.com/users/benlodotcom/following{/other_user}","gists_url":"https://api.github.com/users/benlodotcom/gists{/gist_id}","starred_url":"https://api.github.com/users/benlodotcom/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/benlodotcom/subscriptions","organizations_url":"https://api.github.com/users/benlodotcom/orgs","repos_url":"https://api.github.com/users/benlodotcom/repos","events_url":"https://api.github.com/users/benlodotcom/events{/privacy}","received_events_url":"https://api.github.com/users/benlodotcom/received_events","type":"User","site_admin":false},"created_at":"2015-04-08T17:43:22Z","updated_at":"2015-04-08T17:43:22Z","body":"> What are the arguments against behaving like arrays and other mutable data structures: not inherently thread safe, but can be safely used across multiple threads when the callers guarantees potential multithreaded changes are serial. Is that too risky of an API?\r\n\r\nThis is definitely defensive measures enqueueChangeset is not safe for concurrent access, and more things needs to happen for it to be, the counter would be a first step but the main one would be the mutation of the inputChangeset. Also making this API concurrent wouldn't make much sense because changesets are not independent (or at least not guaranteed to be) and hence need to be enqueued in a specific order. So I wouldn't bother changing the counter to be atomic this code path shouldn't be accessed concurrently anyway.\r\n\r\nI agree with you that enforcing a call on the main thread is more than what we are looking for (which is just serialization of the inputs), happy to move to a model where we don't enforce main thread anymore but  it would still be nice to enforce that enqueues are done serially and I like @adamjernst suggestion there.\r\n\r\nSmall digression but moving away from a changesets API and just pass the whole state of the world + do diffing (basically automatically generate a changeset from the current state to the other) behind the scenes would alleviate those issues as well, in that case concurrency just becomes an internal versioning problem as each input is independent from the others and the only constraint for eventual consistency is to make sure we never apply a less \"recent\" state than the one currently displayed. IMO all of this doesn't belong in this layer though and could be proposed as an adapter in front of the datasources that also handles serialization of the \"generated\" changesets."}}