Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix event handling for reanimated v1 #1149

Closed
wants to merge 1 commit into from

Conversation

jakub-gonet
Copy link
Member

Description

Previously, we were handling dispatched events like so:

@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.

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 RCTEventEmitter which receives event data and creates new events with old data.

Changes

  • created CopyingEventEmitter which copies events by creating new events with almost the same data
  • added logic for copied events disposal in handleEvent()
  • extracted logic creating internal event nodes tags to getEventNodeTag()

Caveats

We can't copy events directly, because Event's isn't exposed. We do it by using Event.init(viewTag) method which will change the event's timestamp and mInitialized field.

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 onEventDispatch callback 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.

Copy link
Member

@kmagiera kmagiera left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good, left two small comments inline

} else {
mEventQueue.offer(event);
synchronized (mCopyingEventEmitter) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

@jakub-gonet jakub-gonet Sep 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea; will implement it.

@jakub-gonet jakub-gonet mentioned this pull request Oct 7, 2020
@jakub-gonet jakub-gonet closed this Oct 9, 2020
@jakub-gonet jakub-gonet deleted the @kuba/fix-event-handling branch October 22, 2020 12:56
karol-bisztyga pushed a commit that referenced this pull request Oct 27, 2020
Co-authored-by: karol-bisztyga <[email protected]>
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`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants