-
-
Notifications
You must be signed in to change notification settings - Fork 983
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 threading issue in RNGestureHandlerStateChangeEvent #1171
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was referenced Aug 20, 2020
kmagiera
approved these changes
Aug 21, 2020
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job on tracking down this longstanding issue 🎉 PR description is great too 🚀
jakub-gonet
added a commit
that referenced
this pull request
Aug 24, 2020
## Description Fixes #1169. Another instance of changing the handler state outside the UI thread. In this case, calling `handler.cancel()` which calls `mOrchestrator.onHandlerStateChange()` interfered with `scheduleFinishedHandlersCleanup()` method which calls `handler.reset()` and sets `mOrchestrator` to `null`. This may happen when threads interleave in the way described below: 1. `RNGestureHandlerModule.dropGestureHandler` is called (directly or in `attachGestureHandler()`; it's called on native modules thread) 2. RNGH dispatches first `onStateChange` event on UI thread 3. after dispatching event `mHandlingChangeSemaphore` is zero and call to `scheduleFinishedHandlersCleanup()` causes `cleanupFinishedHandlers()` to reset handlers and clear `mOrchestrator` 4. Native modules thread continues with execution and tries to call `moveToState` which crashes the app This may show when `RNGestureHandlerModule.attachGestureHandler()` or `RNGestureHandlerModule.dropGestureHandler()` is rapidly called from JS (e.g. when frequently unmounting or updating handler component). After checking the call hierarchy of `GestureHandler.moveToState()` it seems like this is the last occurrence of a native-module-calling-UI-only-method problem. ## Example Fast clicking on the TapGestureHandler crashes with error "Expected to run on UI thread", introduced in #1171. After applying this patch, the error disappears. ```jsx import React, { useState } from 'react'; import { View } from 'react-native'; import { TapGestureHandler } from 'react-native-gesture-handler'; export default () => { const [i, setI] = useState(0); const updateState = () => setI(i + 1); return ( <View style={{ flex: 1, alignItems: 'center', justifyContent: 'center' }}> <TapGestureHandler onHandlerStateChange={updateState} key={i}> <View style={{ height: 100, width: 100, backgroundColor: 'pink' }} /> </TapGestureHandler> </View> ); }; ```
Looking forward to a new release with this commit included! Great work! |
karol-bisztyga
pushed a commit
to software-mansion/react-native-reanimated
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`.
braincore
pushed a commit
to braincore/react-native-gesture-handler
that referenced
this pull request
Mar 4, 2021
…sion#1171) ## Description This commit fixes a threading issue connected with `enabled` property of gesture handlers. Changing this property in JS called `updateGestureHandler` in the RNGH Java module which in turn called `setEnabled`. `setEnabled` cancels handler by using `cancel()` method if it was in an active state previously. This method was mistakenly called directly from the native modules thread - state transition methods are intended to be called from the UI thread. This made GH orchestrator call `handler.dispatchStateChange()` on the wrong thread. This caused event listeners to receive the event on a non-UI thread (`NodeManager.onEventDispatch()` from Reanimated) via `EventDispatcher`. Reanimated handles non-UI events in `onEventDispatch` (e.g. `onLayout` event) by adding them to the internal queue and posting frame callback if it wasn't posted previously (`onAnimationFrame()` wasn't called). Then any queued event is handled on UI thread in the next frame. Problem is, `EventDispatcher` first calls `onEventDispatch()` of any registered listeners and then runs `maybePostFrameCallbackFromNonUI()` which tries to post frame callback dispatching and disposing events from JS thread. So there was a possibility that we: 1. queue event in `NodeManager.onEventDispatch` in native modules thread 2. handle and **dispose** event in `EventDispatcher`, setting extra data to `null` 3. take the event from the queue and try handling it in the `NodeManager.onAnimationFrame`, raising exception. The solution to that problem is to always run `cancel()` (and any other `stateChange` method) on UI thread. - Fixes react-navigation/react-navigation/issues/6403 - Fixes satya164/react-native-tab-view/issues/976 - Fixes software-mansion/react-native-reanimated/issues/704 ## Test plan Huge thanks to the @midoushitongtong who provided small enough code example which reproduced this issue. ```jsx import * as React from 'react'; import { View, StyleSheet, Dimensions, Text } from 'react-native'; import { TabView, SceneMap } from 'react-native-tab-view'; const FirstRoute = () => ( <View style={[styles.scene, { backgroundColor: '#ff4081' }]}> <Text>aaaaa</Text> <Text>aaaaa</Text> <Text>aaaaa</Text> <Text>aaaaa</Text> <Text>aaaaa</Text> </View> ); const initialLayout = { width: Dimensions.get('window').width }; const InnerTab = () => { const [index, setIndex] = React.useState(0); const [routes] = React.useState([{ key: 'first', title: 'First' }]); const renderScene = SceneMap({ first: FirstRoute, }); return ( <TabView lazy navigationState={{ index, routes }} renderScene={renderScene} onIndexChange={setIndex} initialLayout={initialLayout} /> ); }; export default () => { const [index, setIndex] = React.useState(0); const [routes] = React.useState([ { key: 'a', title: 'a' }, { key: 'b', title: 'b' }, { key: 'c', title: 'c' }, ]); const renderScene = SceneMap({ a: InnerTab, b: InnerTab, c: InnerTab, }); return ( <TabView lazy navigationState={{ index, routes }} renderScene={renderScene} onIndexChange={setIndex} initialLayout={initialLayout} /> ); }; const styles = StyleSheet.create({ scene: { flex: 1, }, }); ``` When swiping rapidly in the first or last tab app crashed. After running `cancel()` on UI thread it stopped crashing. Also made sure that the Example app still works correctly.
braincore
pushed a commit
to braincore/react-native-gesture-handler
that referenced
this pull request
Mar 4, 2021
…on#1173) ## Description Fixes software-mansion#1169. Another instance of changing the handler state outside the UI thread. In this case, calling `handler.cancel()` which calls `mOrchestrator.onHandlerStateChange()` interfered with `scheduleFinishedHandlersCleanup()` method which calls `handler.reset()` and sets `mOrchestrator` to `null`. This may happen when threads interleave in the way described below: 1. `RNGestureHandlerModule.dropGestureHandler` is called (directly or in `attachGestureHandler()`; it's called on native modules thread) 2. RNGH dispatches first `onStateChange` event on UI thread 3. after dispatching event `mHandlingChangeSemaphore` is zero and call to `scheduleFinishedHandlersCleanup()` causes `cleanupFinishedHandlers()` to reset handlers and clear `mOrchestrator` 4. Native modules thread continues with execution and tries to call `moveToState` which crashes the app This may show when `RNGestureHandlerModule.attachGestureHandler()` or `RNGestureHandlerModule.dropGestureHandler()` is rapidly called from JS (e.g. when frequently unmounting or updating handler component). After checking the call hierarchy of `GestureHandler.moveToState()` it seems like this is the last occurrence of a native-module-calling-UI-only-method problem. ## Example Fast clicking on the TapGestureHandler crashes with error "Expected to run on UI thread", introduced in software-mansion#1171. After applying this patch, the error disappears. ```jsx import React, { useState } from 'react'; import { View } from 'react-native'; import { TapGestureHandler } from 'react-native-gesture-handler'; export default () => { const [i, setI] = useState(0); const updateState = () => setI(i + 1); return ( <View style={{ flex: 1, alignItems: 'center', justifyContent: 'center' }}> <TapGestureHandler onHandlerStateChange={updateState} key={i}> <View style={{ height: 100, width: 100, backgroundColor: 'pink' }} /> </TapGestureHandler> </View> ); }; ```
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This commit fixes a threading issue connected with
enabled
property of gesture handlers. Changing this property in JS calledupdateGestureHandler
in the RNGH Java module which in turn calledsetEnabled
.setEnabled
cancels handler by usingcancel()
method if it was in an active state previously.This method was mistakenly called directly from the native modules thread - state transition methods are intended to be called from the UI thread.
This made GH orchestrator call
handler.dispatchStateChange()
on the wrong thread. This caused event listeners to receive the event on a non-UI thread (NodeManager.onEventDispatch()
from Reanimated) viaEventDispatcher
.Reanimated handles non-UI events in
onEventDispatch
(e.g.onLayout
event) by adding them to the internal queue and posting frame callback if it wasn't posted previously (onAnimationFrame()
wasn't called). Then any queued event is handled on UI thread in the next frame.Problem is,
EventDispatcher
first callsonEventDispatch()
of any registered listeners and then runsmaybePostFrameCallbackFromNonUI()
which tries to post frame callback dispatching and disposing events from JS thread.So there was a possibility that we:
NodeManager.onEventDispatch
in native modules threadEventDispatcher
, setting extra data tonull
NodeManager.onAnimationFrame
, raising exception.The solution to that problem is to always run
cancel()
(and any otherstateChange
method) on UI thread.Test plan
Huge thanks to the @midoushitongtong who provided small enough code example which reproduced this issue.
When swiping rapidly in the first or last tab app crashed. After running
cancel()
on UI thread it stopped crashing. Also made sure that the Example app still works correctly.