Fix race-condition during render

[piaskowyk]

Summary

This PR fixes a problem with race conditions during updating parent view and mounting children. The problem occurs in some specific circumstances:

• Parent has an animated style
• Parent update style without animation, just assigns new value to shared value (single frame update, without animation)
• Child doesn't have animated style
• Child's styles depend on parent size
• Child is mounting during update of parent style

In this situation (repro in attached example code) may happen race conditions. See at the graph:

Style updating by Reanimated

Mounting new view by React

So when we merge both graph in single one, It should behave in this way:

But because Reanimated is trying to perform update synchronously it looks like that:

In this specific situation, the React Shadow Tree and UI Native Tree are not synchronized. To resolve this issue, I disabled the synchronous style update through Reanimated until those trees can be synchronized once more.

before	after
Screen.Recording.2023-10-05.at.14.43.58.mov	Screen.Recording.2023-10-05.at.14.40.24.mov

Fixes #4932

Test plan

Example code

import React, { useState } from 'react';
import Reanimated, {
  useAnimatedStyle,
  useSharedValue,
} from 'react-native-reanimated';
import {Pressable, View, StyleSheet, Text} from 'react-native';

function App() {
  const [toggle, setToggle] = useState(false);
  const height = useSharedValue(100);

  const animatedStyle = useAnimatedStyle(() => {
    return { height: height.value };
  });

  return (
    <View style={styles.container}>
      <Reanimated.View style={[styles.parent, animatedStyle]}>
      {toggle && <Reanimated.View style={[styles.child]} />}
      </Reanimated.View>

      <Pressable onPress={() => {
        setToggle(!toggle);
        height.value = height.value === 100 ? 200 : 100;
      }} style={styles.button}>
        <Text>Press Me</Text>
      </Pressable>

    </View>
  );
}

const styles = StyleSheet.create({
  container: {
    height: '100%',
  },
  parent: {
    backgroundColor: 'red',
    width: '100%',
  },
  child: {
    backgroundColor: 'green',
    height: '100%',
    width: '50%'
  },
  button: {
    position: 'absolute',
    bottom: 100,
  },
});

export default App;

[tjzel]

Looks good for me, although I'm not that proficient with RN internals.

[tomekzaw]

Looks good to me! Thanks for explaining the idea behind this PR offline. I also don't have any comments in terms of code style ✨

[kmagiera]

I like this approach much more than the previous one. However, I think it's not as intuitive as it could be and introduces back and forth coupling between swizzled uimanager and reanodesmanager.

I added one inline comment where we do a private field lookup that we could easily avoid. But I think if we redesigned this a little bit we could both get rid of that and make the communication between these two classes more easy to follow. Here is what I'd propose:

We add a method to UIManager category that's like: isExecutingUpdatesBatch or something similar. This method can be called from performOperations where we used to call hasEnqueuedUICommands. Everything else should be encapsulated inside swizzled UIManager to make this method function properly. This way we get rid of observer class and all the things around it (instantiating, assigning etc)

The method isExecutingUpdatesBatch should base its result on two variables: hasPendingBlocks and isFlushingBlocks. The first one meaning if there are any ui blocks enqueued in the batch and the second one meaning if there is a flush operations enqueued and not finished.

hasPendingBlocks can be set to true in addUIBlock and perpendUIBlock and reset back in flushUIBlocks. It will only be assigned from UI manager queue but can be accessed from UI queue.

isFlushingBlocks should be a semaphore and should be incremented in flushUIBlocks on the UImanager queue (before we reset hasPendingBlocks) and decremented in the last block that we add using addUIBlock. Note that we don't need to go through this process if hasPendingBlocks is false.

[kmagiera]

This is much cleaner and looks good overall. For completeness I'd add the following things:

• add UIManager queue asserts or comments for the methods that we expect that'll only run on UI manager queue (specifically where we set hasPendingBlocks)
• add a comment in the place where we use isExecutingUpdatesBatch that says we don't need additional synchronization beyond atomics because at the moment the method gets called, both UI manager and UI queues are blocked – this is a necessary condition for this to work w/o race