Skip to content

Commit

Permalink
Allow recursive calls to schedulerShouldRenderTransactions without de…
Browse files Browse the repository at this point in the history
…adlocks (facebook#44725)

Summary:

Changelog: [internal]

## Context

We're currently testing synchronous state updates in Fabric (committing shadow trees for state updates synchronously in the thread where they were dispatched, instead of always scheduling it in the JS thread).

In these experiments we saw a problem caused by a recent change in the Android mounting layer (done to fix a problem with the Event Loop) where we were doing the mount operations inside a mutex lock. The problem is that we didn't have recursive commit+mount operations (because we were dispatching state updates in the JS thread) but now that we do we get a deadlock here.

 {F1659804385} 

These recursive commit+mount operations happen because it's possible to trigger state updates while we mount changes in the host platform (e.g.: we create the scroll view and we update the state to set the content offset). Those state updates trigger more mount operations, which deadlock in the mentioned place.

## Changes

This fixes the described issue by restricting the lock only to access the list of pending operations, but not to apply them. In the current implementation, `mountingManager->executeMount` is protected by the lock, whereas in the new version it isn't (so it can be safely called recursively). The synchronization of the mount operations is done directly at the mounting layer on Android.

Differential Revision: D57968936
  • Loading branch information
rubennorte authored and facebook-github-bot committed May 30, 2024
1 parent 07219a9 commit 08d972d
Show file tree
Hide file tree
Showing 20 changed files with 160 additions and 41 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*
* @generated SignedSource<<84f45a19bf93a62da7ce904a2682fc51>>
* @generated SignedSource<<4e30b8a42dfe7e041ecb30a386b54e50>>
*/

/**
Expand Down Expand Up @@ -40,6 +40,12 @@ public object ReactNativeFeatureFlags {
@JvmStatic
public fun allowCollapsableChildren(): Boolean = accessor.allowCollapsableChildren()

/**
* Adds support for recursively processing commits that mount synchronously (Android only).
*/
@JvmStatic
public fun allowRecursiveCommitsWithSynchronousMountOnAndroid(): Boolean = accessor.allowRecursiveCommitsWithSynchronousMountOnAndroid()

/**
* When enabled, the RuntimeScheduler processing the event loop will batch all rendering updates and dispatch them together at the end of each iteration of the loop.
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*
* @generated SignedSource<<2f84a5455d747e14324f7b79cbbdc763>>
* @generated SignedSource<<944fabed27a371ce2e1a86e367312c1f>>
*/

/**
Expand All @@ -22,6 +22,7 @@ package com.facebook.react.internal.featureflags
public class ReactNativeFeatureFlagsCxxAccessor : ReactNativeFeatureFlagsAccessor {
private var commonTestFlagCache: Boolean? = null
private var allowCollapsableChildrenCache: Boolean? = null
private var allowRecursiveCommitsWithSynchronousMountOnAndroidCache: Boolean? = null
private var batchRenderingUpdatesInEventLoopCache: Boolean? = null
private var destroyFabricSurfacesInReactInstanceManagerCache: Boolean? = null
private var enableBackgroundExecutorCache: Boolean? = null
Expand Down Expand Up @@ -59,6 +60,15 @@ public class ReactNativeFeatureFlagsCxxAccessor : ReactNativeFeatureFlagsAccesso
return cached
}

override fun allowRecursiveCommitsWithSynchronousMountOnAndroid(): Boolean {
var cached = allowRecursiveCommitsWithSynchronousMountOnAndroidCache
if (cached == null) {
cached = ReactNativeFeatureFlagsCxxInterop.allowRecursiveCommitsWithSynchronousMountOnAndroid()
allowRecursiveCommitsWithSynchronousMountOnAndroidCache = cached
}
return cached
}

override fun batchRenderingUpdatesInEventLoop(): Boolean {
var cached = batchRenderingUpdatesInEventLoopCache
if (cached == null) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*
* @generated SignedSource<<c82dca47362f4798dfcebda31b2fe2d6>>
* @generated SignedSource<<d2ccaef4c75979c26327963aeee0b0ad>>
*/

/**
Expand Down Expand Up @@ -32,6 +32,8 @@ public object ReactNativeFeatureFlagsCxxInterop {

@DoNotStrip @JvmStatic public external fun allowCollapsableChildren(): Boolean

@DoNotStrip @JvmStatic public external fun allowRecursiveCommitsWithSynchronousMountOnAndroid(): Boolean

@DoNotStrip @JvmStatic public external fun batchRenderingUpdatesInEventLoop(): Boolean

@DoNotStrip @JvmStatic public external fun destroyFabricSurfacesInReactInstanceManager(): Boolean
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*
* @generated SignedSource<<1ea3b574063dca0256494361423ce63e>>
* @generated SignedSource<<c74b5434945f63cf42551b188b9f7a48>>
*/

/**
Expand All @@ -27,6 +27,8 @@ public open class ReactNativeFeatureFlagsDefaults : ReactNativeFeatureFlagsProvi

override fun allowCollapsableChildren(): Boolean = true

override fun allowRecursiveCommitsWithSynchronousMountOnAndroid(): Boolean = false

override fun batchRenderingUpdatesInEventLoop(): Boolean = false

override fun destroyFabricSurfacesInReactInstanceManager(): Boolean = false
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*
* @generated SignedSource<<a66642e4f6027ab0f5280baa2169d901>>
* @generated SignedSource<<3b94757d58e6adb2bccf4ddfb86e2080>>
*/

/**
Expand All @@ -26,6 +26,7 @@ public class ReactNativeFeatureFlagsLocalAccessor : ReactNativeFeatureFlagsAcces

private var commonTestFlagCache: Boolean? = null
private var allowCollapsableChildrenCache: Boolean? = null
private var allowRecursiveCommitsWithSynchronousMountOnAndroidCache: Boolean? = null
private var batchRenderingUpdatesInEventLoopCache: Boolean? = null
private var destroyFabricSurfacesInReactInstanceManagerCache: Boolean? = null
private var enableBackgroundExecutorCache: Boolean? = null
Expand Down Expand Up @@ -65,6 +66,16 @@ public class ReactNativeFeatureFlagsLocalAccessor : ReactNativeFeatureFlagsAcces
return cached
}

override fun allowRecursiveCommitsWithSynchronousMountOnAndroid(): Boolean {
var cached = allowRecursiveCommitsWithSynchronousMountOnAndroidCache
if (cached == null) {
cached = currentProvider.allowRecursiveCommitsWithSynchronousMountOnAndroid()
accessedFeatureFlags.add("allowRecursiveCommitsWithSynchronousMountOnAndroid")
allowRecursiveCommitsWithSynchronousMountOnAndroidCache = cached
}
return cached
}

override fun batchRenderingUpdatesInEventLoop(): Boolean {
var cached = batchRenderingUpdatesInEventLoopCache
if (cached == null) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*
* @generated SignedSource<<7a63b1fc14ccc86efb0929452755744d>>
* @generated SignedSource<<68ec2c046868a2cb1d2d88e4fdd9d993>>
*/

/**
Expand All @@ -27,6 +27,8 @@ public interface ReactNativeFeatureFlagsProvider {

@DoNotStrip public fun allowCollapsableChildren(): Boolean

@DoNotStrip public fun allowRecursiveCommitsWithSynchronousMountOnAndroid(): Boolean

@DoNotStrip public fun batchRenderingUpdatesInEventLoop(): Boolean

@DoNotStrip public fun destroyFabricSurfacesInReactInstanceManager(): Boolean
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -487,11 +487,28 @@ void Binding::schedulerShouldRenderTransactions(
return;
}

std::unique_lock<std::mutex> lock(pendingTransactionsMutex_);
for (auto& transaction : pendingTransactions_) {
mountingManager->executeMount(transaction);
if (ReactNativeFeatureFlags::
allowRecursiveCommitsWithSynchronousMountOnAndroid()) {
std::vector<MountingTransaction> pendingTransactions;

{
// Retain the lock to access the pending transactions but not to execute
// the mount operations because that method can call into this method
// again.
std::unique_lock<std::mutex> lock(pendingTransactionsMutex_);
pendingTransactions_.swap(pendingTransactions);
}

for (auto& transaction : pendingTransactions) {
mountingManager->executeMount(transaction);
}
} else {
std::unique_lock<std::mutex> lock(pendingTransactionsMutex_);
for (auto& transaction : pendingTransactions_) {
mountingManager->executeMount(transaction);
}
pendingTransactions_.clear();
}
pendingTransactions_.clear();
}

void Binding::schedulerDidRequestPreliminaryViewAllocation(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*
* @generated SignedSource<<82d20aa409496b17df21a077088894a6>>
* @generated SignedSource<<318ea35c5b58c968d7026dbe547200e4>>
*/

/**
Expand Down Expand Up @@ -51,6 +51,12 @@ class ReactNativeFeatureFlagsProviderHolder
return method(javaProvider_);
}

bool allowRecursiveCommitsWithSynchronousMountOnAndroid() override {
static const auto method =
getReactNativeFeatureFlagsProviderJavaClass()->getMethod<jboolean()>("allowRecursiveCommitsWithSynchronousMountOnAndroid");
return method(javaProvider_);
}

bool batchRenderingUpdatesInEventLoop() override {
static const auto method =
getReactNativeFeatureFlagsProviderJavaClass()->getMethod<jboolean()>("batchRenderingUpdatesInEventLoop");
Expand Down Expand Up @@ -173,6 +179,11 @@ bool JReactNativeFeatureFlagsCxxInterop::allowCollapsableChildren(
return ReactNativeFeatureFlags::allowCollapsableChildren();
}

bool JReactNativeFeatureFlagsCxxInterop::allowRecursiveCommitsWithSynchronousMountOnAndroid(
facebook::jni::alias_ref<JReactNativeFeatureFlagsCxxInterop> /*unused*/) {
return ReactNativeFeatureFlags::allowRecursiveCommitsWithSynchronousMountOnAndroid();
}

bool JReactNativeFeatureFlagsCxxInterop::batchRenderingUpdatesInEventLoop(
facebook::jni::alias_ref<JReactNativeFeatureFlagsCxxInterop> /*unused*/) {
return ReactNativeFeatureFlags::batchRenderingUpdatesInEventLoop();
Expand Down Expand Up @@ -286,6 +297,9 @@ void JReactNativeFeatureFlagsCxxInterop::registerNatives() {
makeNativeMethod(
"allowCollapsableChildren",
JReactNativeFeatureFlagsCxxInterop::allowCollapsableChildren),
makeNativeMethod(
"allowRecursiveCommitsWithSynchronousMountOnAndroid",
JReactNativeFeatureFlagsCxxInterop::allowRecursiveCommitsWithSynchronousMountOnAndroid),
makeNativeMethod(
"batchRenderingUpdatesInEventLoop",
JReactNativeFeatureFlagsCxxInterop::batchRenderingUpdatesInEventLoop),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*
* @generated SignedSource<<08463d4f487df81332c79c8e85b4dac8>>
* @generated SignedSource<<4d557dc4399d46888205165ee7ae4ab2>>
*/

/**
Expand Down Expand Up @@ -36,6 +36,9 @@ class JReactNativeFeatureFlagsCxxInterop
static bool allowCollapsableChildren(
facebook::jni::alias_ref<JReactNativeFeatureFlagsCxxInterop>);

static bool allowRecursiveCommitsWithSynchronousMountOnAndroid(
facebook::jni::alias_ref<JReactNativeFeatureFlagsCxxInterop>);

static bool batchRenderingUpdatesInEventLoop(
facebook::jni::alias_ref<JReactNativeFeatureFlagsCxxInterop>);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*
* @generated SignedSource<<c51ddd57ad10e748d6e2c5f444ea3765>>
* @generated SignedSource<<2a748a0a1b92cb0514fec83668ed8b0f>>
*/

/**
Expand All @@ -29,6 +29,10 @@ bool ReactNativeFeatureFlags::allowCollapsableChildren() {
return getAccessor().allowCollapsableChildren();
}

bool ReactNativeFeatureFlags::allowRecursiveCommitsWithSynchronousMountOnAndroid() {
return getAccessor().allowRecursiveCommitsWithSynchronousMountOnAndroid();
}

bool ReactNativeFeatureFlags::batchRenderingUpdatesInEventLoop() {
return getAccessor().batchRenderingUpdatesInEventLoop();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*
* @generated SignedSource<<705dba656bfb228227399bcda638c404>>
* @generated SignedSource<<a75464f590c4f55e64ad2e33cfc52989>>
*/

/**
Expand Down Expand Up @@ -47,6 +47,11 @@ class ReactNativeFeatureFlags {
*/
RN_EXPORT static bool allowCollapsableChildren();

/**
* Adds support for recursively processing commits that mount synchronously (Android only).
*/
RN_EXPORT static bool allowRecursiveCommitsWithSynchronousMountOnAndroid();

/**
* When enabled, the RuntimeScheduler processing the event loop will batch all rendering updates and dispatch them together at the end of each iteration of the loop.
*/
Expand Down
Loading

0 comments on commit 08d972d

Please sign in to comment.