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 #1319

Merged
merged 3 commits into from
Oct 27, 2020
Merged

Fix event handling #1319

merged 3 commits into from
Oct 27, 2020

Conversation

Szymon20000
Copy link
Contributor

@Szymon20000 Szymon20000 commented Oct 7, 2020

Copied from #1149

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.

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.

String key = viewTag + eventName;

shouldSaveEvent |= (mCustomEventHandler != null && mNativeProxy.isAnyHandlerWaitingForEvent(key));
mEventQueue.offer(new CopiedEvent(event));
Copy link
Member

Choose a reason for hiding this comment

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

Copied from #1149

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
Contributor Author

Choose a reason for hiding this comment

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

I don't see a need for a CopiedEvent pool.

Copy link
Member

Choose a reason for hiding this comment

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

Not using the pool will strain GC, event objects will be created and removed frequently if events will be coming from other source than the UI thread.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't make sense for me because we still have to create and remove events' payloads.

Copy link
Member

Choose a reason for hiding this comment

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

So maybe @kmagiera will chime in since he suggested this change in the original PR.

@Szymon20000 Szymon20000 marked this pull request as ready for review October 7, 2020 08:28
@Szymon20000 Szymon20000 requested a review from kmagiera October 7, 2020 08:35
@karol-bisztyga karol-bisztyga merged commit c79d27c into master Oct 27, 2020
@karol-bisztyga karol-bisztyga deleted the @szymon/fix_event_handling branch October 27, 2020 11:18
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.

3 participants