Allow recursive calls to schedulerShouldRenderTransactions without deadlocks #44725
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
facebook-github-bot
added
the
CLA Signed
|
This pull request was exported from Phabricator. Differential Revision: D57968936 |
rubennorte
added a commit
to rubennorte/react-native
that referenced
this pull request
May 30, 2024
…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
rubennorte
force-pushed
the
export-D57968936
branch
from
2fc41cd
to
08d972d
Compare
|
This pull request was exported from Phabricator. Differential Revision: D57968936 |
rubennorte
added a commit
to rubennorte/react-native
that referenced
this pull request
May 30, 2024
…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
rubennorte
added a commit
to rubennorte/react-native
that referenced
this pull request
May 30, 2024
…without deadlocks (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
…ached (facebook#44724) Summary: Changelog: [internal] Batching operations at this layer was wrong because these are the operations that were already flushed by the mounting layer but were accumulated in `SurfaceMountingManager` because the root view wasn't created. These operations should be executed before anything else that's scheduled in the `MountItemDispatcher`, so we should never batch them. The problem this was trying to solve is solved in a different way in D57968937. This was gated so this shouldn't affect any current usages. Reviewed By: sammy-SC Differential Revision: D57968939
…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. Reviewed By: sammy-SC Differential Revision: D57968936
rubennorte
force-pushed
the
export-D57968936
branch
from
08d972d
to
d08ba0f
Compare
|
This pull request was exported from Phabricator. Differential Revision: D57968936 |
Base commit: 32b5c96 |
|
This pull request has been merged in d1c7201. |
kosmydel
pushed a commit
to kosmydel/react-native
that referenced
this pull request
Jun 11, 2024
…adlocks (facebook#44725) Summary: Pull Request resolved: facebook#44725 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. Reviewed By: sammy-SC Differential Revision: D57968936 fbshipit-source-id: 52f996d212cad691646610632b03b5223e7e90ca
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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->executeMountis 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