Summary:
`addObserver` and `removeObserver` now accepts const references instead of pointers which indicates the intent (non-nullability and non-owning) clearly. The delegate methods are also marked as `const` to designate the possible concurrent execution (`const` means "thread-safe" here).
All changes are pure syntactical, nothing really changes (besides the fact overall code quality and redability).
Reviewed By: JoshuaGross
Differential Revision: D17535395
fbshipit-source-id: b0c6c872d44fee22e38fd067ccd3320e7231c94a
Summary:
# A race condition
The practical thing of this diff is fixing a data race.
Imagine a case where a thread A calls `addObserver` and thread B calls `nativeImageResponseFailed` at the same time.
Thread A might read `status_` exclusively and store result as a local variable and then go sleep.
Then thread B starts and finishes `nativeImageResponseFailed`, it writes `status_` and notifies all observers.
Then thread B wakes up. It adds an observer to a collection of observers and finishes.
As a result, the observer from `addObserver` will never be called.
To fix this, we changed a logic a bit to lock only once per method. During the lock, we read and/or write to storage and then perform side-effects.
In contrast, previously we often locked only around the access of a particular instance variable (several times per method).
The challenge here is that idiomatic/fancy to C++/STL ways to lock mutexes don't work in our case.
# C++ idioms and readability, multiple locks for the same transaction
STL has tools to avoid calling `lock` and `unlock` methods manually (std::lock_guard<> and lamdas). Unfortunately, using that in our use case is quite problematic. That's probably possible but will lead to much less readable code and some copy-pasta in `addObserver`.
Therefore we replaced using `std::lock_guard` with simple `lock` and `unlock` where using `std::lock_guard` was problematic.
# Why we changed `shared_mutex` to a normal one?
After consolidating the locks we found that we have an only case where we can use shared lock (in `nativeImageResponseProgress`). Calling this method in real life is not concurrent, so it makes sense to replace a shared lock with a more simple and performant regular one.
Reviewed By: sammy-SC
Differential Revision: D17368739
fbshipit-source-id: 61d66fb737d8c2dc73001a80a31edaa59a16d886
Summary:
This diff contains some small improvements in `ImageResponseObserverCoordinator` which are pretty minor:
* Now we use `small_vector` instead of a normal one. In the vast majority of cases, the coordinator has only one observer, so having `small_vector` with default size `1` saves us a memory allocation (which is a dozen of allocations for a screen, not bad).
* Empty constructor and destructor were removed.
* Unnecessary copying of ImageResponse was removed. ImageResponse is a practically a shared_pointer, it has value semantic and does not need to be copied. We don't need to acquire mutex to access that.
Reviewed By: sammy-SC
Differential Revision: D17368740
fbshipit-source-id: 828e27a72b9c8ac0063c5fbda00f83ddb255309c
Summary: Folly promises/futures have been replaced by an observer model which keeps track of loading state. This resolves at least one crash that I can no longer repro and simplifies the code a bit (IMO).
Reviewed By: shergin
Differential Revision: D13743393
fbshipit-source-id: 2b650841525db98b2f67add85f2097f24259c6cf
Valentin Shergin
zhongwuzw
Joshua Gross