Skip to content
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 cyclic object detection in makeShareableCloneRecursive #4475

Merged
merged 1 commit into from
May 22, 2023

Conversation

tomekzaw
Copy link
Member

@tomekzaw tomekzaw commented May 18, 2023

Summary

This PR improves the developer experience when playing around with Reanimated's internals takes a very bad turn.

Currently, while cloning shareables, we check for cycles by storing the cloned object at some depth:

processedObjectAtThresholdDepth = value;

Sometimes (not exactly sure when) we clear this variable so we don't hold it forever:
processedObjectAtThresholdDepth = undefined;

However, it may happen that while we're deep down in the tree, the function will be called with depth = 0 which resets the store and thus breaks our cyclic object detection mechanism, resulting in a well-known "Maximum call stack exceeded" error, like here:

Here's the place where we call makeShareableClone recursively without passing depth:

toAdapt.__initData = makeShareableCloneRecursive(
value.__initData,
true
);

This PR adds the missing argument.

Before After
Zrzut ekranu 2023-05-18 o 17 55 30 Zrzut ekranu 2023-05-18 o 17 55 37

Test plan

  1. Add 'worklet'; directive in runOnUI function in threads.ts
  2. Comment out makeShareableCloneRecursive(worklet); inside runOnUI function body
  3. Call the following code:
runOnUI(() => {
  'worklet';
  console.log(_WORKLET, 'Hello from the UI thread!');
  runOnUI;
})();

@tomekzaw tomekzaw requested a review from kmagiera May 18, 2023 16:23
Copy link
Member

@kmagiera kmagiera left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

@tomekzaw tomekzaw added this pull request to the merge queue May 22, 2023
Merged via the queue into main with commit 297bdd0 May 22, 2023
@tomekzaw tomekzaw deleted the @tomekzaw/add-missing-depth branch May 22, 2023 20:57
fluiddot pushed a commit to wordpress-mobile/react-native-reanimated that referenced this pull request Jun 5, 2023
…e-mansion#4475)

## Summary

This PR improves the developer experience when playing around with
Reanimated's internals takes a very bad turn.

Currently, while cloning shareables, we check for cycles by storing the
cloned object at some depth:

https://github.com/software-mansion/react-native-reanimated/blob/4ebd011650e932af12150ed37c27c1cb66d00fd9/src/reanimated2/shareables.ts#L103
Sometimes (not exactly sure when) we clear this variable so we don't
hold it forever:

https://github.com/software-mansion/react-native-reanimated/blob/4ebd011650e932af12150ed37c27c1cb66d00fd9/src/reanimated2/shareables.ts#L110
However, it may happen that while we're deep down in the tree, the
function will be called with `depth = 0` which resets the store and thus
breaks our cyclic object detection mechanism, resulting in a well-known
"Maximum call stack exceeded" error, like here:


![](https://github.com/software-mansion/react-native-reanimated/assets/20516055/6558d757-6fd1-4da5-8db2-3048fbca76f8)

Here's the place where we call `makeShareableClone` recursively without
passing `depth`:


https://github.com/software-mansion/react-native-reanimated/blob/4ebd011650e932af12150ed37c27c1cb66d00fd9/src/reanimated2/shareables.ts#L157-L160

This PR adds the missing argument.

| Before | After |
|:-:|:-:|
| <img width="503" alt="Zrzut ekranu 2023-05-18 o 17 55 30"
src="https://github.com/software-mansion/react-native-reanimated/assets/20516055/bc54b12d-a6cb-4ae3-ad00-18242d85f2c7">
| <img width="503" alt="Zrzut ekranu 2023-05-18 o 17 55 37"
src="https://github.com/software-mansion/react-native-reanimated/assets/20516055/033646e9-6370-49a3-89b1-17adb7833c17">
|

## Test plan

1. Add `'worklet';` directive in `runOnUI` function in `threads.ts`
2. Comment out `makeShareableCloneRecursive(worklet);` inside `runOnUI`
function body
3. Call the following code:
```ts
runOnUI(() => {
  'worklet';
  console.log(_WORKLET, 'Hello from the UI thread!');
  runOnUI;
})();
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants