[Fabric] Add enumerateWithBlock: for surface registry

[zhongwuzw]

Summary

To ensure all methods in surface registry thread safe, add copy to enumerator method.
cc. @shergin .

Changelog

[iOS] [Fixed] - Add copy for surface registry when return enumerator

Test Plan

N/A.

[shergin]

I think we should fix it a bit differently.
Instead of copying the collection on every enumerator call, we should change the signature of the method. Instead of returning the enumerator the method should accept a block with the enumerator as a parameter. Internally, the method should acquire the lock, call the block with enumerator and release lock.

[shergin]

... that way we can ensure that we iterate over the actual representation of the collection and avoid races. And that's a bit more efficient.

[zhongwuzw]

@shergin , seems we should make some trade-off, IMO, if we should release lock after finished enumeration, maybe it's worse than copy collection, for example, currently, we use it in _startAllSurfaces, in the time of lock, we need to call _startSurface for each surface, I think it may not a simple function, needs do some works. So I may still think copy is more efficient(copy only copy the pointer).

- (void)_startAllSurfaces
{
  for (RCTFabricSurface *surface in _surfaceRegistry.enumerator) {
    [self _startSurface:surface];
  }
}

[shergin]

Which exact trade-off do you mean? copy does not just copy a pointer, it clones the whole collection.
Yes, in real life it's just several Surfaces but still conceptually it's better to lock the collection during iterating over.

LMK if you need a detailed explanation of proposed method.

[zhongwuzw]

Yeah, copy would clone whole collection, but it's shallow copy. If we choose provide the enumerator parameter by block, it may has the risks to dead-lock, for example, what if _startSurface: function also access _surfaceRegistry ? maybe call surfaceForRootTag:, we don't release the lock, but we may try to lock again. So my opinion is still just copy the collection, that we always provide immutable data to external.

- (void)_startAllSurfaces
{
  for (RCTFabricSurface *surface in _surfaceRegistry.enumerator) {
    [self _startSurface:surface]; // what if _startSurface internal access `surfaceRegistry`?
  }
}

[shergin]

Well, if we'll have a dead lock in that case, it will indicate a much more severe logical problem.

[zhongwuzw]

@shergin 👌 , I changed the method signature and add the block parameter.

[facebook-github-bot]

@shergin is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[react-native-bot]

This pull request was successfully merged by @zhongwuzw in 27e7279.

^{When will my fix make it into a release? | Upcoming Releases}