Fix deadlock in REANodesManager (removes synchronous UI updates)

[cltnschlosser]

Heads up

This is my first attempt to dig in to reanimated, and I'm also new to react-native. So I'd appreciate extra scrutiny and explanation of any concerns.

Description

Fixes #3180
Would fix #2285 by removing the mounting code

Changes

Removes synchronous UI updates in REANodesManager (Added by #1215, to fix #1123). In a second commit here I added to the examples that were specified in that pull request and issue. I was unable to reproduce the issue on my device (iPhone 11 Pro). It's possible that I'm missing something or other conditions have improved the performance issues that were seen before. Would love feedback from @Szymon20000 and/or @kyle-ssg since they originally experienced and fixed #1123.

If we wanted to synchronously perform these updates in a more thread safe way, React-core would need to be updated to include something like the following. This would block the UIManager queue and execute on the main queue. Instead of the current approach of blocking the main queue to execute on the UIManager queue. Blocking the main queue is the current dangerous operation.

void RCTUnsafeExecuteOnMainQueueSyncFromUIManagerQueue(dispatch_block_t block)
{
  RCTAssertUIManagerQueue();

  if (RCTIsMainQueue()) {
    block();
  } else {
    dispatch_sync(dispatch_get_main_queue(), ^{
      pseudoUIManagerQueueFlag = YES;
      block();
      pseudoUIManagerQueueFlag = NO;
    });
  }
}

Test code and steps to reproduce

Included a second commit (which should likely be removed or cleaned up) with some test screens.

Checklist

• Included code example that can be used to test this change
• Ensured that CI passes

[He1nr1chK]

Any updates on this? @piaskowyk @jakub-gonet

[He1nr1chK]

I can confirm that this PR does solve the issue for me. I have tested it by using patch-package, I just do not have write access else I would have reviewed it myself.

[richardo2016x]

Last Update ↓

It seems work, I found the patch WORKs on those npm versions:

     "react-native": "0.68.2",
     "react-native-reanimated": "~2.8.0"

I guess, there're some bugs on communitation between JS thread and UI thread if you use react-native@0.67.x with react-native-reanimated@2.8.0(I can capture the classical mutex lock error in Xcode after RN app launched a little while). the bugs cannot be resolved by this PR's patch ant it causes App crashing.

BUT, just upgrade react-native to 0.68.2, the mutex lock error disappered, you may ONLY see crash by other reasons(Network library, Image decode, etc.), and deadlock caused by react-native-reanimated. THEN use this PR's patch, the deadlock problem resolved.

---

Old Comment ↓

thx the work for this PR by @cltnschlosser, but I must say it's a pity this patch didn't resolve my problem:

my npm versions:

     "react-native": "0.67.4",
     "react-native-reanimated": "~2.8.0", // it's 2.8.0 on my machine

I applied the patch, but I can still crash due to concurrency issue on multiple-thread, which is same as the version without patch:

I will subscribe the change of this PR, looking forward the progress by @cltnschlosser or anyone else, thx a lot again.

[cltnschlosser]

@richardo2016x I think that’s unrelated (#2327). But since you can reproduce it, can you find what mutex is throwing the exception and what the invalid argument being passed is?

[richardo2016x]

@richardo2016x I think that’s unrelated (#2327). But since you can reproduce it, can you find what mutex it throwing the exception and what the invalid argument it’s being passed is?

@cltnschlosser You're right, my screenshot in previous comment is related to #2327, one old, legacy bug. I spent some hours find out where the mutex lock error happend, but I can just track to this._value = value in react-native-reanimated's js function workletValueSetter, I know somewhere triggers the function but I don't know where mutex lock failed and what the invalid argument is.

I can reproduce it on react-native@0.67.4 in my App, there're some other react-native-* libraries dependent on the react-native-reanimated in my App. I' not sure if I could give a minimal sample to reproduce it, If I have spare time, I'd like to make it.

On the other hand, I'm not so sure that whether one sample on react-native@0.67.4 is valueable --- react-native has been 0.69.1 for now, the new architecture FABRIC has been introduced in 0.68. Compared to fix the issue for react-native@<=0.67.x based on react-native-reanimated@2.8.x, maybe it's better to encourage developer to upgrade to react-native@>=0.68.0 and use this patch :)

Thx a lot again 👍 this patch inspired me how to debug/tuning react-native-reanimated for my App

[reichhartd]

@piaskowyk @jakub-gonet When can this pull request be reviewed? We have been using react-native-reanimated 2.2.4 for 6 months because of #3180. 2.2.4 creates some problems in combination with the latest React Native version and Jest 28.

[tomekzaw]

Closing this PR since the issue was fixed in #4239 and backported to v2 in #4283.