React-native-reanimated: Exception when unmounting component with Animated Views

[peacechen]

Description

I refactored and added confetti animations to React Native "Make It Rain". It creates 100 nodes by default, but the user can specify more through props. The component throws an exception in some cases, or logs a warning in other cases.

What is the recommended way to clean up nodes to avoid issues? I couldn't find mention of that in the docs at https://software-mansion.github.io/react-native-reanimated. It calls stopClock() on unmount. I've experimented with additional guards in the useCode hook to prevent the clock from restarting, and returning an empty block on unmount.

Screenshots

Steps To Reproduce

Start the example project in the Make It Rain repo.
https://github.com/peacechen/react-native-make-it-rain#sample-application

Toggle the switch from Arrive to Rain.

Expected behavior

No errors or warnings. Switch should take effect immediately, but the animations continue to run for 8+ seconds.

Actual behavior

The example project spews Excessive number of pending callbacks: 501 when the animation type is switched.
Another user has observed an exception when the component unmounts:

*** Assertion failure in -[REANodesManager disconnectNodes:childID:](), <path-to-project>/node_modules/react-native-reanimated/ios/REANodesManager.m:286

[error][tid:main][RCTUIManager.m:1166] Exception thrown while executing UI block: 'childNode' is a required parameter

Snack or minimal code example

Code is available in the example project and the root of the repo.
https://github.com/peacechen/react-native-make-it-rain/tree/master/Example

Package versions

• React: 16.9.0
• React Native: 0.61.4
• React Native Reanimated: 1.8.0

[jakub-gonet]

This is related to call to the native module in __detach() which is called when node should be removed from view. #87 can provide more context.

I'll investigate this further and maybe batching those updates may be better for performance and if not, we can provide an option to disable this JS<=>Native<=>JS roundtrip

[peacechen]

Thanks for investigating this, @jakub-gonet . Will the fix be in the next release of Reanimated?

[jakub-gonet]

Yeah, probably.
You can use patch-package to disable this behavior until then.

[jakub-gonet]

Related to #654.

[jakub-gonet]

This is an issue that we can't really solve - when detaching and reattaching node to view it should maintain its state. We can't update this state in any other way than calling a method in the native module.

Reanimated has an upper limit of nodes it can manage and exceeding this limit can trigger this warning.

[peacechen]

What is the upper limit of nodes so we can set the appropriate restriction?

What is the appropriate way to destroy nodes? The documentation isn't clear on this, providing only a few tools such as stopClock() which don't address this issue. Components will be unmounted, and there should be a way to clean up properly.

[jakub-gonet]

What is the upper limit of nodes so we can set the appropriate restriction?

I haven't tested that, this depends on the device performance.

What is the appropriate way to destroy nodes?

Nodes are created on the native side by __attach() method and destroyed by __detach() method of AnimatedNode. The simplified model is like that:

• every animated component is wrapper in createAnimatedComponent() which manages the lifecycle of nodes, binding it to the passed component.
• when the wrapped component is mounted (or updated) we call _attachProps() which attaches nodes bound to every animated property and style.
• when wrapped component is unmounting, we call __detach() to remove nodes from the native side.
  This calls native method synchronizing JS value with native node value (this call causes Excessive number of pending callbacks: 501 when unmounting large tree of nodes)

We can't really disable this native call, because many animations rely on that behavior. You can disable it by yourself (e.g. by using patch-package or forking Reanimated).

[peacechen]

That sounds like Reanimated is unsuitable for real world animations which are complex and have more than ~20 nodes. Until a solution is found for that architectural limitation, it's only usable for simple demo projects.

[jakub-gonet]

Well, if you had more than 501 callbacks pending then you used more than 501 nodes and that's a bit more than 20 :>

You may be interested in Reanimated v2 (now in alpha) which doesn't rely on nodes and uses turbo modules instead, reducing bridge traffic to 0.

[peacechen]

The demo app in the OP triggers the error even when the number of nodes is reduced to ~50. I forget the exact number, but I tested with fewer nodes than that and it still caused the unexpected callbacks error. Do the pending callbacks map 1:1 with number of nodes?

How certain are you that Reanimated v2 doesn't have these types of bugs with high number of nodes? Having been burned once refactoring to Reanimated v1, I'm wary of going through that again.

[jakub-gonet]

The demo app in the OP triggers the error even when the number of nodes is reduced to ~50. I forget the exact number, but I tested with fewer nodes than that and it still caused the unexpected callbacks error.

If you're saying about this then there's 50 nodes per one item if I understand code correctly. Even with 10 animated items that's quite a lot of nodes.

Do the pending callbacks map 1:1 with number of nodes?

Not really, but with a high number of nodes, it's a good approximation — basically, React Native keeps a queue of callbacks that should be dispatched to JS. If you overflow this queue you'll get this warning.

How certain are you that Reanimated v2 doesn't have these types of bugs with high number of nodes? Having been burned once refactoring to Reanimated v1, I'm wary of going through that again.

Because Reanimated 2 doesn't use nodes at all — we're using JSI's shared references. You create so-called 'worklets' which are JS functions with annotation. You can also create SharedValues to share data between JS and the native side (asynchronously). Worklets are registered as turbo module functions and because arguments passed are copied (if they're not shared values) you don't have any traffic on the bridge. Additionally, worklets are executed on the thread pool, so they don't block UI thread if they're executing some heavy code not related to the rendering.

I understand your frustration and as I said, you can create a patch to the reanimated, disabling this node update behavior, and use patch-package to apply it automatically.

[davidbiedenbach]

Hey @jakub-gonet , I'm probably going to locally patch AnimatedValue to optionally skip the synchronization call via a flag (i.e. for AnimatedValues that are not shared and only need to exist within the lifecycle of a particular component). Is that the sort of thing you'd accept a PR on?

[jakub-gonet]

I'm a little hesitant to merge such implementation details fixes, but if it helps some folks then why not. I'd want to know @kmagiera's opinion on that though.

[kmagiera]

hey @davidbiedenbach – happy to accept a patch that'd optionally disable value synchronization upon unmount as long as it does not incur any change of behavior for the cases when the flag isn't set.

Also, at this point I think you may be interested in looking into Reanimated 2 API where we take a completely different approach for managing animated value and hence Reanimated 2 is free of the problem with too many callbacks.

[davidbiedenbach]

@kmagiera
Sounds good. Right now, I've locally patched to add a 3rd param, default false, to the InternalAnimatedValue constructor. It behaves just like _constant, only it doesn't fail the invariant check in AnimatedSet. Suits my purposes fine; not sure if you'd prefer to have to manually set a flag on the Value at cleanup-time inside the owning component's willUnmount. I can get a PR up for either of those options once it's cleared through our OSS contributions process.

As for Reanimated 2, I'd love to try it, but we're blocked on the Hermes dependency by Realm. :(