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 race condition when ShareableHandle is accessed on multiple threads #5759

Merged
merged 9 commits into from
Mar 6, 2024

Conversation

tjzel
Copy link
Collaborator

@tjzel tjzel commented Mar 4, 2024

Summary

Fixes #5660, a regression introduced in #4300.

The aforementioned race condition happens this way:

  1. An object using a ShareableHandle underneath (e.g. a shared value) is created.
  2. This object is accessed on UI thread.
  3. The initializer of this ShareableHandle get called on the UI thread.
  4. At the same time, JS thread schedules an operation on this object (e.g. setting a value of shared value).
  5. Access on UI thread forces the initialization. The condition of the if clause resolves to true and UI thread tries to access the runtime.
    Screenshot 2024-03-04 at 13 00 07
  6. However, access from JS thread has locked the runtime and causes the UI runtime to wait.
  7. Then, JS thread enters the same if clause body and initializes the whole shareable.
  8. After initializing and releasing the runtime, UI thread gets unblocked.
  9. However, now initializer_ has been nulled and causes memory access issues.

It's difficult to change the whole flow of locking to prevent such scenarios. Therefore we won't null the initializer_ object anymore. However, this won't fix the problem of potential double initialization. Luckily, the code of valueUnpacker already prevents that with its shareable handle cache and the fact that runtime operations must be sequential.

Test plan

Run the following race condition reproduction made - you should re-run the app several times (probably up to 20ish) since it's most likely to happen when the app starts. It's also more likely to happen on Android simulators (at least for me).

Test code
import {useSharedValue} from 'react-native-reanimated';
import {useEffect, useState} from 'react';

const value = 666666;

const Screen = () => {
  const sv = useSharedValue(value);
  useSharedValue(1);
  useSharedValue(2);
  useSharedValue(3);
  sv.value = value;

  useEffect(() => {
    // eslint-disable-next-line @typescript-eslint/no-unused-vars
    const currentValue = sv.value;
  }, [sv]);

  return null;
};
const ReanimatedCrashReproduction = () => {
  const [render, setRender] = useState(false);

  useEffect(() => {
    const interval = setInterval(() => setRender(r => !r), 500);
    return () => clearInterval(interval);
  }, []);

  return render ? <Screen /> : null;
};

export default ReanimatedCrashReproduction;

You can do the following to see that double initialization happens still, but all is well 🚰.

Screenshot 2024-03-06 at 10 06 50

Big thanks to @piaskowyk for the debugging help 🚀

@efstathiosntonas
Copy link
Contributor

thanks @tjzel, this must have been hard to track down!

@tjzel tjzel force-pushed the @tjzel/iterative-crash-debug branch from 18135d8 to cb91e6c Compare March 4, 2024 12:36
@Latropos
Copy link
Contributor

Latropos commented Mar 5, 2024

@tjzel I was looking into linked issue and it seemed that the problem was occurring only in newest release. Could you please track down and link here the exact PR causing the bug?

@callaars
Copy link

callaars commented Mar 5, 2024

@Latropos, it's been occurring since 3.7.0.

@tomekzaw
Copy link
Member

tomekzaw commented Mar 5, 2024

@Latropos It seems like the offending PR was #4300.

@tjzel tjzel requested review from tomekzaw and removed request for tomekzaw March 6, 2024 08:15
Common/cpp/SharedItems/Shareables.h Outdated Show resolved Hide resolved
Common/cpp/SharedItems/Shareables.h Outdated Show resolved Hide resolved
Common/cpp/SharedItems/Shareables.cpp Outdated Show resolved Hide resolved
Common/cpp/SharedItems/Shareables.h Outdated Show resolved Hide resolved
@tjzel tjzel added this pull request to the merge queue Mar 6, 2024
Merged via the queue into main with commit 0f34974 Mar 6, 2024
14 checks passed
@tjzel tjzel deleted the @tjzel/iterative-crash-debug branch March 6, 2024 13:40
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.7.0] Data type not recognized by value unpacker. at valueUnpacker (WorkletRuntime::WorkletRuntime:1:1477)
6 participants