Conversation
kmagiera
left a comment
There was a problem hiding this comment.
looks good, left two small comments inline
| handleEvent(event, false); | ||
| } else { | ||
| mEventQueue.offer(event); | ||
| synchronized (mCopyingEventEmitter) { |
There was a problem hiding this comment.
why is this here? does not seem to be needed, we don't synchronize on mCopyingEventEmitter elsewhere and the code that emitter runs does not seem to be not thread safe?
There was a problem hiding this comment.
If I remember correctly I wanted to synchronize mEventMapping.contains key check with adding to the event queue, so we don't remove mapping while the event is copied and added to the queue.
It may be unnecessary here because we queue events anyway and we should check if a mapping exists at a time when we're popping event from the queue.
There was a problem hiding this comment.
if eventmapping can be modified in a different thread there should be some level of synchronization. But synchronizing on it in only one place is not enough. I see you changed the map to mEventMapping to a concurrent map. This should address the problem of the map internals when dealing with reads writs from multiple threads. IMO this seems enough and the synchronized block isn't needed. If you want to ensure that the mapping stays in the map until we process the event we'd need to add some additional locking, but I don't think it is necessary. The map check is only added as an optimization step so that we don't copy all the events but only ones we want to process.
|
|
||
| @Override | ||
| public void receiveEvent(int targetTag, String eventName, @Nullable WritableMap eventData) { | ||
| CopiedEvent event = new CopiedEvent(); |
There was a problem hiding this comment.
Maybe we could also pool CopiedEvent objects same way we have pools of other events. In this case this should be rather efficient as there typically won't be more than 2-3 events in use.
There was a problem hiding this comment.
Good idea; will implement it.
Co-authored-by: karol-bisztyga <karol.bisztyga@swmansion.com> Copied from #1149 ## Description Previously, we were handling dispatched events like so: ```java @OverRide public void onEventDispatch(Event event) { if (UiThreadUtil.isOnUiThread()) { handleEvent(event); } else { mEventQueue.offer(event); startUpdatingOnAnimationFrame(); } } ``` Event handling in RN works by utilizing `EventDispatcher.dispatchEvent(event)` which 1. runs `onEventDispatch` callback on any registered listener 2. adds `event` to internal event queue 3. adds frame callback which dispatches and disposes events on JS thread This approach introduced timing issues - RN's `EventDispatcher` dispatches events on JS thread and Reanimated handles events on UI thread. There's a possibility that `EventDispatcher` will dispose event (possibly destroying it's state in `onDispose()`) before Reanimated would have chance to handle it. This was found after investigating [pretty popular crash in react-native-gesture-handler](software-mansion/react-native-gesture-handler#1171). # HOW The pull-request adds another method `isAnyHandlerWaitingForEvent` to NativeProxy API which lets us check if an event is important (there is workletHandler listening for the event) or not. The rest part of the pr is very similar to Jakub's pr. However, there are some differences. Instead of saving copied event Object, we save: tag, eventName, and payload in the new class `CopiedEvent`.
Description
Previously, we were handling dispatched events like so:
Event handling in RN works by utilizing
EventDispatcher.dispatchEvent(event)whichonEventDispatchcallback on any registered listenereventto internal event queueThis approach introduced timing issues - RN's
EventDispatcherdispatches events on JS thread and Reanimated handles events on UI thread. There's a possibility thatEventDispatcherwill dispose event (possibly destroying it's state inonDispose()) before Reanimated would have chance to handle it.This was found after investigating pretty popular crash in react-native-gesture-handler.
This PR fixes event handling for Reanimated 1 by copying events that we're subscribed to via event nodes. It's done by creating an intermediate class implementing
RCTEventEmitterwhich receives event data and creates new events with old data.Changes
CopyingEventEmitterwhich copies events by creating new events with almost the same datahandleEvent()getEventNodeTag()Caveats
We can't copy events directly, because
Event's isn't exposed. We do it by usingEvent.init(viewTag)method which will change the event's timestamp andmInitializedfield.Event timestamp is used in event coalescing. Changing it would theoretically prevent copied events from being coalesced if we detach event listener while the stream of events is dispatched. We actually don't use this functionality right now (events coalescing is done after
onEventDispatchcallback had run, to utilize it we would have to change RN core or implement our own coalescing logic) and it doesn't seem like a big problem as event coalescing was primarily introduced to reduce the load in the JS thread.