Skip to content

Make RCTUnsafeExecuteOnMainQueueSync safer #51425

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

RSNara
Copy link
Contributor

@RSNara RSNara commented May 17, 2025

Problem

React native's new list component is doing synchronous render. That means it makes synchronous dispatches from main thread to the js thread. (To capture the runtime so that it can execute js on the main thread).

But, the js thread already as a bunch of synchronous calls to the main thread. So, if any of those js -> ui sync calls happen concurrently with a synchronous render, the application will deadlock.

This diff is an attempt to mitigate all those deadlocks.

Context

How js execution from the main thread works:

  • Main thread puts a block on the js thread, to capture the js runtime.
  • Main thread goes to sleep until that "runtime capture" block executes.
  • Js thread executes "runtime capture block". The runtime is captured for the main thread. The js thread is put to sleep, until the runtime is released.
  • Main thread wakes up, noticing that the runtime is captured. It executes its js code with the captured runtime. Then, it releases the runtime, and wakes up the js thread. Both the main and js thread move on to other tasks.

How synchronous js -> main thread calls work:

  • Js thread puts a ui block on the main queue.
  • Js thread goes to sleep until that ui block executes on the main thread.

Deadlock #1

Main thread: execute js now:

  • Main thread puts a block on the js queue, to capture the runtime.
  • Main thread then then goes to sleep, waiting for runtime to be captured

JS thread: execute ui code synchronously:

  • Js thread schedules a block on the ui thread
  • Js thread then goes to sleep, waiting for that block to execute.

Result: The application deadlocks

| {F1978009555} | {F1978009612} |

image

Deadlock #2

JS thread: execute ui code synchronously:

  • Js thread schedules a block on the ui thread
  • Js thread then goes to sleep waiting for that block to execute.

Main thread: execute js now:

  • Main thread puts a block on the js queue, to capture the runtime.
  • Main thread then then goes to sleep, waiting for runtime to be captured

Result: The application deadlocks

| {F1978009690} | {F1978009701} |

image

Changes

This diff attempts to fix those deadlocks. How:

  • In "execute ui code synchronously" (js thread):
    • Before going to sleep, the js thread schedules the ui work on the main queue, and it posts the ui work to "execute js now".
  • In "execute js now" (main thread):
    • This diff makes "execute js now" stateful: it keeps a "pending ui block."
    • Before capturing the runtime, the "execute js now" executes "pending ui work", if it exists.
    • While sleeping waiting for runtime capture, "execute js now" can wake up, and execute "pending ui work." It goes back to sleep afterwards, waiting for runtime capture.

Mitigation: Deadlock #1

Main thread: execute js now:

  • Main thread puts a block on the js queue, to capture the runtime.
  • Main thread then then goes to sleep, waiting for runtime capture

JS Thread: execute ui code synchronously:

  • Js thread puts its ui block on the ui queue.
  • New: Js thread also posts that ui block to "execute js now". Main thread was sleeping waiting for runtime to be captured. It now wakes up.
  • Js thread goes to sleep.

The main thread wakes up in "execute js now":

  • Main thread sees that a "pending ui block" is posted. It executes the "pending ui block." The block, also scheduled on the main thread, noops henceforth.
  • Main thread goes back to sleep, waiting for runtime capture.
  • The js thread wakes up, moves on to the next task.

Result: The runtime is captured by the main thread.

| {F1978010383} | {F1978010363} | {F1978010371} | {F1978010379} |

image

Mitigation: Deadlock #2

JS Thread: execute ui code synchronously:

  • Js thread puts its ui block on the ui queue.
  • New: Js thread also posts that ui block to "execute js now". Main thread was sleeping waiting for runtime to be captured. It now wakes up.
  • Js thread goes to sleep.

Main thread: execute js now

  • Main thread sees that a "pending ui block" is posted. It executes the "pending ui block" immediately. The block, also scheduled on the main thread, noops henceforth.
  • Js thread wakes up and moves onto the next task.

Result: Main thread captures the runtime.

| {F1978010525} | {F1978010533} | {F1978010542} |

image

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. p: Facebook Partner: Facebook Partner labels May 17, 2025
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D74769326

@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D74769326

@RSNara RSNara force-pushed the export-D74769326 branch from 8a578e3 to 167f7f2 Compare May 17, 2025 17:46
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D74769326

RSNara added a commit to RSNara/react-native that referenced this pull request May 17, 2025
Summary:
Pull Request resolved: facebook#51425

# Problem

Sync rendering/events (i.e: execute js now below), when used in conjunction with js -> ui sync calls, can deadlock react native:

## Deadlock facebook#1
**Main thread**: execute js now:
  * Main thread puts a block on the js queue, to capture the runtime.
 * Main thread then then goes to sleep, waiting for runtime to be captured

**JS thread**: execute ui code synchronously:
* Js thread schedules a block on the ui thread
* Js thread then goes to sleep, waiting for that block to execute.
* **The application deadlocks**

| {F1978009555} |  {F1978009612} |

## Deadlock facebook#2
**JS thread**: execute ui code synchronously:
* Js thread schedules a block on the ui thread
* Js thread then goes to sleep waiting for that block to execute.

**Main thread**: execute js now:
* Main thread puts a block on the js queue, to capture the runtime.
* Main thread then then goes to sleep, waiting for runtime to be captured
* **The application deadlocks.**

|  {F1978009690}  | {F1978009701} |

# Changes
This diff attempts to fix those deadlocks. How:
* This diff introduces a stateful "execute js now" coordinator.
* In "execute ui code synchronously" (js thread):
   * Before going to sleep, the js thread posts its ui work to the "execute js now" coordinator.
* In "execute js now" (main thread):
   * Before trying to capture the runtime, the main thread executes "pending ui work", if it exists.
   * While the main thread is sleeping, waiting for runtime capture, it can be woken up, and asked to execute "pending ui work."

## Mitigation: Deadlock facebook#1
**Main thread**: execute js now:
* Main thread puts a block on the js queue, to capture the runtime.
* Main thread then then goes to sleep, waiting for runtime to be captured

**JS Thread**: execute ui code synchronously:
* Js thread schedules this block on the ui thread.
* ***New***: Js thread also assigns this block to the coordinator. *And wakes up the main thread*.
* Js thread goes to sleep.

The main thread wakes up:
* Main thread **executes** the ui block assigned to the coordinator. **This cancels the ui block scheduled on the main queue.**
* Main thread goes back to sleep.
* The js thread wakes up, moves on to the next task.

The runtime is captured by the main thread.

| {F1978010383} | {F1978010363} |  {F1978010371} |  {F1978010379}

## Mitigation: Deadlock facebook#2
**JS Thread**: execute ui code synchronously:
* Js thread schedules this block on the ui thread.
* ***New***: Js thread also assigns this block to the coordinator. *And wakes up the main thread*.
* Js thread goes to sleep.

**Main thread**: execute js now
* Main thread executes the ui block immediately. (This cancels the ui block on the main queue).
* Js thread wakes up and moves onto the next task.

Main thread captures the runtime.

|  {F1978010525}  |  {F1978010533} |  {F1978010542} |

Differential Revision: D74769326
@RSNara RSNara force-pushed the export-D74769326 branch from 167f7f2 to 82fdfbc Compare May 17, 2025 18:18
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D74769326

@RSNara RSNara force-pushed the export-D74769326 branch from 82fdfbc to 3b9e538 Compare May 17, 2025 22:01
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D74769326

@RSNara RSNara force-pushed the export-D74769326 branch from 3b9e538 to 9f7b4cf Compare May 18, 2025 16:30
@react-native-bot
Copy link
Collaborator

react-native-bot commented May 18, 2025

Fails
🚫

📋 Missing Changelog - Please add a Changelog to your PR description. See Changelog format

Warnings
⚠️ 📋 Missing Test Plan - Can you add a Test Plan? To do so, add a "## Test Plan" section to your PR description. A Test Plan lets us know how these changes were tested.

Generated by 🚫 dangerJS against 07040ce

@RSNara RSNara changed the title RuntimeExecutor: Fix sync js execution from main thread [react-native][c++] Make RCTUnsafeExecuteOnMainQueueSync safer May 18, 2025
@RSNara RSNara changed the title [react-native][c++] Make RCTUnsafeExecuteOnMainQueueSync safer Make RCTUnsafeExecuteOnMainQueueSync safer May 18, 2025
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D74769326

@RSNara RSNara force-pushed the export-D74769326 branch from 9f7b4cf to baf8aaa Compare May 19, 2025 01:07
RSNara added 4 commits May 18, 2025 18:19
Summary:
Just refactoring the control flow in these functions (in a separate diff). So, that the logic in subsequent diffs is easier to read: D74769326.

Note: I made the second argument to RCTUnsafeExecuteOnMainQueueSyncWithError nullable.

Changelog: [Internal]

Differential Revision: D74940681
Summary:
Just refactoring an renaming the variables inside RuntimeExecutor again (in a separate diff). Just so that the logic in the subsequent diffs is easier to read: D74769326

Changelog: [Internal]

Differential Revision: D74941734
Summary:
We want the ios implementation of this api, to be implemented differently. Because, we need to build in deadlock mitigation for sync renders and events, for ios only.

Changelog: [Internal]

Differential Revision: D74901907
Summary:
Gating for "safer main queue sync dispatch": D74769326

Changelog: [Internal]

Differential Revision: D74940621
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D74769326

@RSNara RSNara force-pushed the export-D74769326 branch from baf8aaa to d99483e Compare May 19, 2025 01:53
Summary:
# Problem

React native's new list component is doing synchronous render. That means it makes synchronous dispatches from main thread to the js thread. (To capture the runtime so that it can execute js on the main thread).

But, the js thread already as a bunch of synchronous calls to the main thread. So, if any of those js -> ui sync calls happen concurrently with a synchronous render, the application will deadlock.

This diff is an attempt to mitigate all those deadlocks.

## Context
How js execution from the main thread works:

* Main thread puts a block on the js thread, to capture the js runtime.
* Main thread goes to sleep until that "runtime capture" block executes.
* Js thread executes "runtime capture block". The runtime is captured for the main thread. The js thread is put to sleep, until the runtime is released.
* Main thread wakes up, noticing that the runtime is captured. It executes its js code with the captured runtime. Then, it releases the runtime, and wakes up the js thread. Both the main and js thread move on to other tasks.

How synchronous js -> main thread calls work:
* Js thread puts a ui block on the main queue.
* Js thread goes to sleep until that ui block executes on the main thread.

## Deadlock facebook#1
**Main thread**: execute js now:
  * Main thread puts a block on the js queue, to capture the runtime.
 * Main thread then then goes to sleep, waiting for runtime to be captured

**JS thread**: execute ui code synchronously:
* Js thread schedules a block on the ui thread
* Js thread then goes to sleep, waiting for that block to execute.

**Result:** The application deadlocks

| {F1978009555} |  {F1978009612} |

![image](https://github.com/user-attachments/assets/325a62f4-d5b7-492d-a114-efb738556239)

## Deadlock facebook#2
**JS thread**: execute ui code synchronously:
* Js thread schedules a block on the ui thread
* Js thread then goes to sleep waiting for that block to execute.

**Main thread**: execute js now:
* Main thread puts a block on the js queue, to capture the runtime.
* Main thread then then goes to sleep, waiting for runtime to be captured

**Result:** The application deadlocks

|  {F1978009690}  | {F1978009701} |

![image](https://github.com/user-attachments/assets/13a6ea17-a55d-453d-9291-d1c8007ecffa)

# Changes
This diff attempts to fix those deadlocks. How:
* In "execute ui code synchronously" (js thread):
   * Before going to sleep, the js thread schedules the ui work on the main queue, **and** it  posts the ui work to "execute js now".
* In "execute js now" (main thread):
   * This diff makes "execute js now" stateful: it keeps a "pending ui block."
   * Before capturing the runtime, the "execute js now" executes "pending ui work", if it exists.
   * While sleeping waiting for runtime capture, "execute js now" can wake up, and execute "pending ui work." It goes back to sleep afterwards, waiting for runtime capture.

## Mitigation: Deadlock facebook#1
**Main thread**: execute js now:
* Main thread puts a block on the js queue, to capture the runtime.
* Main thread then then goes to sleep, waiting for runtime capture

**JS Thread**: execute ui code synchronously:
* Js thread puts its ui block on the ui queue.
* ***New***: Js thread also posts that ui block to "execute js now". Main thread was sleeping waiting for runtime to be captured. It now wakes up.
* Js thread goes to sleep.

The main thread wakes up in "execute js now":
* Main thread sees that a "pending ui block" is posted. It executes the "pending ui block." The block, also scheduled on the main thread, noops henceforth.
* Main thread goes back to sleep, waiting for runtime capture.
* The js thread wakes up, moves on to the next task.

**Result:** The runtime is captured by the main thread.

| {F1978010383} | {F1978010363} |  {F1978010371} |  {F1978010379} |

![image](https://github.com/user-attachments/assets/f53cb10c-7801-46be-934a-96af7d5f5fab)

## Mitigation: Deadlock facebook#2
**JS Thread**: execute ui code synchronously:
* Js thread puts its ui block on the ui queue.
* ***New***: Js thread also posts that ui block to "execute js now". Main thread was sleeping waiting for runtime to be captured. It now wakes up.
* Js thread goes to sleep.

**Main thread**: execute js now
* Main thread sees that a "pending ui block" is posted. It executes the "pending ui block" immediately. The block, also scheduled on the main thread, noops henceforth.
* Js thread wakes up and moves onto the next task.

**Result:** Main thread captures the runtime.

|  {F1978010525}  |  {F1978010533} |  {F1978010542} |

![image](https://github.com/user-attachments/assets/9e0ca5ef-fab6-4a26-bcca-d79d36624d5d)

Differential Revision: D74769326
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D74769326

@RSNara RSNara force-pushed the export-D74769326 branch from d99483e to 07040ce Compare May 19, 2025 02:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. fb-exported p: Facebook Partner: Facebook Partner
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants