Ensure unmounted components are updated correctly after mounting on iOS#3374
Merged
piaskowyk merged 1 commit intosoftware-mansion:mainfrom Jul 11, 2022
Merged
Conversation
piaskowyk
approved these changes
Jul 11, 2022
Member
piaskowyk
left a comment
There was a problem hiding this comment.
Thanks a lot for the fix! ❤️ Everything works great!
piaskowyk
pushed a commit
that referenced
this pull request
Jul 11, 2022
…OS (#3374) ## Description Hiya 👋 We stumbled on an issue where an animation was queued on an unmounted component (to fade in), and it would often either a) not fade in at all, or b) fade in after a second or two. This only occurred on iOS (simulator + device), and was introduced in 2.5.0. I narrowed the changeset to this PR: #2580 The problem is [this](https://github.com/software-mansion/react-native-reanimated/blob/03bdda7560d3fc89564df1a838fbb42c87026546/ios/REANodesManager.mm#L398) line. If the component is mounted but has a snapshot, it remains in the queue. There's no guarantee of the next time `-maybeFlushUpdateBuffer` will get called - it comes from `-[REAModule uiManagerWillPerformMounting:]`, so another component has to be mounted to flush the queue. As a result, the animation may appear halted forever, or randomly continue if you attach a component elsewhere in your tree. In a small repro video, you can see how it sometimes appears "stuck", but we can "update" to get it back on track: https://user-images.githubusercontent.com/33126/177866145-a25cad9c-047e-4755-8497-589c56c3c41b.mp4 ## Changes This PR updates the logic to separate the handling of "is the view mounted?" and "handle the snapshot". When the view is not mounted, it will continue to bail early (after updating the snapshot). However, if the view is mounted, and the snapshot exists, it will now proceed in the current update. ## Test code and steps to reproduce The repro relies on a race condition, so it's kinda tricky to reliably cause it. I use a utility called stress (`brew install stress`) and run `stress --cpu 6 --timeout 60` to increase likelihood of threading issues. The button "Start" will (re)start the animation. Press this over and over until you see the failure. When you get a failure, press the "force update" button. It's a no-op, but it causes `UIManager` (I guess?) to fire an event to clear the queue. <details> <summary>App.tsx</summary> ```typescript import React, {useCallback, useEffect, useState} from 'react'; import { Button, SafeAreaView, StatusBar, StyleSheet, Text, View, } from 'react-native'; import Animated, { Easing, interpolate, useAnimatedStyle, useSharedValue, withTiming, } from 'react-native-reanimated'; const App = () => { const [shouldStart, setShouldStart] = useState(false); const animateContent = useSharedValue(0); const start = useCallback(() => { animateContent.value = 0; animateContent.value = withTiming(1, { duration: 3000, easing: Easing.bounce, }); }, [animateContent]); useEffect(() => { if (shouldStart) { start(); setShouldStart(false); } }, [start, shouldStart, setShouldStart]); const onContentLayout = useCallback(() => {}, []); const contentStyle = useAnimatedStyle(() => ({ transform: [ { translateX: interpolate(animateContent.value, [0, 1], [50, 200]), }, ], opacity: interpolate(animateContent.value, [0, 1], [0.2, 1]), })); return ( <SafeAreaView style={{ backgroundColor: 'white', }}> <StatusBar barStyle={'light-content'} /> <Button title="Start" onPress={() => { setShouldStart(true); }} /> {!shouldStart ? ( <View style={[styles.container]}> <Button title="force update" /> <Animated.View onLayout={onContentLayout} style={[contentStyle]}> <Text>Bonjour! 👋</Text> </Animated.View> </View> ) : null} </SafeAreaView> ); }; const styles = StyleSheet.create({ container: { height: 200, width: '100%', top: 200, backgroundColor: 'rgba(127,127,127,0.5)', }, }); export default App; ``` </details> React Native 0.69.1 (also tested down to 0.66.x) Reanimated 2.9.1 (also tested 2.5.0, 2.8.0. however 2.4.1 is fine) ## Checklist - [x] Included code example that can be used to test this change - [ ] Updated TS types - [ ] Added TS types tests - [ ] Added unit / integration tests - [ ] Updated documentation - [x] Ensured that CI passes
3 tasks
fluiddot
pushed a commit
to wordpress-mobile/react-native-reanimated
that referenced
this pull request
Jun 5, 2023
…OS (software-mansion#3374) ## Description Hiya 👋 We stumbled on an issue where an animation was queued on an unmounted component (to fade in), and it would often either a) not fade in at all, or b) fade in after a second or two. This only occurred on iOS (simulator + device), and was introduced in 2.5.0. I narrowed the changeset to this PR: software-mansion#2580 The problem is [this](https://github.com/software-mansion/react-native-reanimated/blob/03bdda7560d3fc89564df1a838fbb42c87026546/ios/REANodesManager.mm#L398) line. If the component is mounted but has a snapshot, it remains in the queue. There's no guarantee of the next time `-maybeFlushUpdateBuffer` will get called - it comes from `-[REAModule uiManagerWillPerformMounting:]`, so another component has to be mounted to flush the queue. As a result, the animation may appear halted forever, or randomly continue if you attach a component elsewhere in your tree. In a small repro video, you can see how it sometimes appears "stuck", but we can "update" to get it back on track: https://user-images.githubusercontent.com/33126/177866145-a25cad9c-047e-4755-8497-589c56c3c41b.mp4 ## Changes This PR updates the logic to separate the handling of "is the view mounted?" and "handle the snapshot". When the view is not mounted, it will continue to bail early (after updating the snapshot). However, if the view is mounted, and the snapshot exists, it will now proceed in the current update. ## Test code and steps to reproduce The repro relies on a race condition, so it's kinda tricky to reliably cause it. I use a utility called stress (`brew install stress`) and run `stress --cpu 6 --timeout 60` to increase likelihood of threading issues. The button "Start" will (re)start the animation. Press this over and over until you see the failure. When you get a failure, press the "force update" button. It's a no-op, but it causes `UIManager` (I guess?) to fire an event to clear the queue. <details> <summary>App.tsx</summary> ```typescript import React, {useCallback, useEffect, useState} from 'react'; import { Button, SafeAreaView, StatusBar, StyleSheet, Text, View, } from 'react-native'; import Animated, { Easing, interpolate, useAnimatedStyle, useSharedValue, withTiming, } from 'react-native-reanimated'; const App = () => { const [shouldStart, setShouldStart] = useState(false); const animateContent = useSharedValue(0); const start = useCallback(() => { animateContent.value = 0; animateContent.value = withTiming(1, { duration: 3000, easing: Easing.bounce, }); }, [animateContent]); useEffect(() => { if (shouldStart) { start(); setShouldStart(false); } }, [start, shouldStart, setShouldStart]); const onContentLayout = useCallback(() => {}, []); const contentStyle = useAnimatedStyle(() => ({ transform: [ { translateX: interpolate(animateContent.value, [0, 1], [50, 200]), }, ], opacity: interpolate(animateContent.value, [0, 1], [0.2, 1]), })); return ( <SafeAreaView style={{ backgroundColor: 'white', }}> <StatusBar barStyle={'light-content'} /> <Button title="Start" onPress={() => { setShouldStart(true); }} /> {!shouldStart ? ( <View style={[styles.container]}> <Button title="force update" /> <Animated.View onLayout={onContentLayout} style={[contentStyle]}> <Text>Bonjour! 👋</Text> </Animated.View> </View> ) : null} </SafeAreaView> ); }; const styles = StyleSheet.create({ container: { height: 200, width: '100%', top: 200, backgroundColor: 'rgba(127,127,127,0.5)', }, }); export default App; ``` </details> React Native 0.69.1 (also tested down to 0.66.x) Reanimated 2.9.1 (also tested 2.5.0, 2.8.0. however 2.4.1 is fine) ## Checklist - [x] Included code example that can be used to test this change - [ ] Updated TS types - [ ] Added TS types tests - [ ] Added unit / integration tests - [ ] Updated documentation - [x] Ensured that CI passes
This file contains hidden or 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
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
Hiya 👋
We stumbled on an issue where an animation was queued on an unmounted component (to fade in), and it would often either a) not fade in at all, or b) fade in after a second or two.
This only occurred on iOS (simulator + device), and was introduced in 2.5.0. I narrowed the changeset to this PR: #2580
The problem is this line. If the component is mounted but has a snapshot, it remains in the queue. There's no guarantee of the next time
-maybeFlushUpdateBufferwill get called - it comes from-[REAModule uiManagerWillPerformMounting:], so another component has to be mounted to flush the queue.As a result, the animation may appear halted forever, or randomly continue if you attach a component elsewhere in your tree.
In a small repro video, you can see how it sometimes appears "stuck", but we can "update" to get it back on track:
07-30-2fk6k-yp6so.mp4
Changes
This PR updates the logic to separate the handling of "is the view mounted?" and "handle the snapshot".
When the view is not mounted, it will continue to bail early (after updating the snapshot). However, if the view is mounted, and the snapshot exists, it will now proceed in the current update.
Test code and steps to reproduce
The repro relies on a race condition, so it's kinda tricky to reliably cause it. I use a utility called stress (
brew install stress) and runstress --cpu 6 --timeout 60to increase likelihood of threading issues.The button "Start" will (re)start the animation. Press this over and over until you see the failure.
When you get a failure, press the "force update" button. It's a no-op, but it causes
UIManager(I guess?) to fire an event to clear the queue.App.tsx
React Native 0.69.1 (also tested down to 0.66.x)
Reanimated 2.9.1 (also tested 2.5.0, 2.8.0. however 2.4.1 is fine)
Checklist