[Fabric] Add lock for surface presenter observers #24052
Conversation
facebook-github-bot
added
the
CLA Signed
React/Fabric/RCTSurfacePresenter.mm
Outdated
| @@ -331,7 +331,10 @@ - (void)mountingManager:(RCTMountingManager *)mountingManager willMountComponent | |||
| { | |||
| RCTAssertMainQueue(); | |||
|
|
|||
| for (id<RCTSurfacePresenterObserver> observer in _observers) { | |||
| std::unique_lock<std::mutex> lock(_observerListMutex); | |||
There was a problem hiding this comment.
We have to use { ... } blocks to designate the scope covered by mutex. We must not call .unlock manually.
We also have to use better::shared_mutex and std::shared_lock here (and std::unique_lock where we modify the collection).
There was a problem hiding this comment.
Done, but seems may not give some benefits? 🤔 because reader seems always from main queue which mean only one reader. 😂
|
The concert is totally valid. We must fix it. |
React/Fabric/RCTSurfacePresenter.mm
Outdated
| std::shared_lock<better::shared_mutex> lock(_observerListMutex); | ||
| observers = [_observers copy]; | ||
| } | ||
| for (id<RCTSurfacePresenterObserver> observer in observers) { |
There was a problem hiding this comment.
Why not iterate over _observers while the lock is acquired?
There was a problem hiding this comment.
One thing to watch out for is deadlock if any of the observer delegate methods can add/remove observers unless better::shared_mutex is like recursive_mutex.
There was a problem hiding this comment.
willMountComponentsWithRootTag: is being called quite often and perf sensitive, so please do.
There was a problem hiding this comment.
@shergin is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
This pull request was successfully merged by @zhongwuzw in 946f1a6. When will my fix make it into a release? | Upcoming Releases |
Summary
Add lock for observers when do enumeration.
cc. @shergin .
Changelog
[iOS] [Fixed] - Add lock for surface presenter observers
Test Plan
N/A.