Cherry-pick to release agua #10633
Cherry-pick to release agua #10633
Conversation
| private final List<OnCameraMoveCanceledListener> onCameraMoveCanceledListenerList = new ArrayList<>(); | ||
| private final List<OnCameraMoveListener> onCameraMoveListenerList = new ArrayList<>(); | ||
| private final List<OnCameraIdleListener> onCameraIdleListenerList = new ArrayList<>(); | ||
| private final CopyOnWriteArrayList<OnCameraMoveStartedListener> onCameraMoveStarted = new CopyOnWriteArrayList<>(); |
There was a problem hiding this comment.
@tobrun Adding CopyOnWriteArrayList makes it thread-safe, but may result in a rare scenario when user unregisters the listener, yet still gets the callback. Do you think we should add synchronization?
There was a problem hiding this comment.
I haven't ran into any issues with that or haven't heard about any reports. The reason why I don't perceive this as an issue is that the interactions with this list allows occurs on the main thread.
The reason why we have a CopyOnWriteArrayList is to allow to remove the listener as part of it's callback to avoid a ConcurrentModificationException. Imo your remark is a tradeoff of this, happy to look more into this if there is a use-case for it.
There was a problem hiding this comment.
The problem arises when you'd like to register/unregister on a separate thread (which in my scenario would have thrown ConcurrentModificationException before and now will just execute the callback). Since your use-case is concrete and the problem was there before anyway, just in a different form, I think we're good here.
The reason why I don't perceive this as an issue is that the interactions with this list allows occurs on the main thread.
Maybe adding @UiThread annotation when registering in MapboxMap would be nice if we expect users to register/unregister on the main thread?
There was a problem hiding this comment.
We annotate the class with @UiThread, which is the equivalent to adding the annotation to each method of the class. Would love to see if we can improve the implementation in future iterations ;)
There was a problem hiding this comment.
We annotate the class with
@UiThread
Missed that, totally makes sense :)
… handle multiple deactivate scenario in FileSource (#10556)
Because a message we queue from the foreground may cause the background to complete, exit, and tear down the AsyncTask, we have to block queue processing until we've finished our call to AsyncTask::send(). Broadening the scope of a mutex is scary, but I audited the code of our four implementations of AsyncTask and I don't see any way this could cause a deadlock.
This PR cherry picks latest changes from master to the agua release branch:
Before merging we need to validate the following:
Close race condition in runloopfrom [core, ios, qt, android] Close race condition in RunLoop (issue #9620) #10537 (impacts multiple platforms).