Skip to content

Commit 25c6112

Browse files
rubennortefacebook-github-bot
authored andcommitted
Fix incorrect locking and attempts check in ShadowTree experiment (#52681)
Summary: Pull Request resolved: #52681 Changelog: [internal] In the original change I made in D78418504 / #52645 I made 2 mistakes: 1. Used a lock that would try to re-lock on itself without it being recursive (which would cause a deadlock). I didn't see that because when testing I didn't hit the case where we'd exhaust the options. 2. The `attemps` variable wasn't incremented, so we never left the loop in case of exhaustion. This propagates a flag to `tryCommit` to indicate we've already locked on the commitMutex_ so we don't need to lock again in that case and increases the counter, fixing the issue. Reviewed By: cortinico Differential Revision: D78497509 fbshipit-source-id: 546ccd0c84aed5416ce1aef47d79419b4fe06f66
1 parent 9b30fe0 commit 25c6112

File tree

2 files changed

+14
-5
lines changed

2 files changed

+14
-5
lines changed

packages/react-native/ReactCommon/react/renderer/mounting/ShadowTree.cpp

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -251,11 +251,12 @@ CommitStatus ShadowTree::commit(
251251
if (status != CommitStatus::Failed) {
252252
return status;
253253
}
254+
attempts++;
254255
}
255256

256257
{
257258
std::unique_lock lock(commitMutex_);
258-
return tryCommit(transaction, commitOptions);
259+
return tryCommit(transaction, commitOptions, true);
259260
}
260261
} else {
261262
while (true) {
@@ -275,7 +276,8 @@ CommitStatus ShadowTree::commit(
275276

276277
CommitStatus ShadowTree::tryCommit(
277278
const ShadowTreeCommitTransaction& transaction,
278-
const CommitOptions& commitOptions) const {
279+
const CommitOptions& commitOptions,
280+
bool hasLocked) const {
279281
TraceSection s("ShadowTree::commit");
280282

281283
auto telemetry = TransactionTelemetry{};
@@ -287,7 +289,10 @@ CommitStatus ShadowTree::tryCommit(
287289

288290
{
289291
// Reading `currentRevision_` in shared manner.
290-
std::shared_lock lock(commitMutex_);
292+
std::shared_lock lock(commitMutex_, std::defer_lock);
293+
if (!hasLocked) {
294+
lock.lock();
295+
}
291296
commitMode = commitMode_;
292297
oldRevision = currentRevision_;
293298
}
@@ -328,7 +333,10 @@ CommitStatus ShadowTree::tryCommit(
328333

329334
{
330335
// Updating `currentRevision_` in unique manner if it hasn't changed.
331-
std::unique_lock lock(commitMutex_);
336+
std::unique_lock lock(commitMutex_, std::defer_lock);
337+
if (!hasLocked) {
338+
lock.lock();
339+
}
332340

333341
if (currentRevision_.number != oldRevision.number) {
334342
return CommitStatus::Failed;

packages/react-native/ReactCommon/react/renderer/mounting/ShadowTree.h

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -111,7 +111,8 @@ class ShadowTree final {
111111
*/
112112
CommitStatus tryCommit(
113113
const ShadowTreeCommitTransaction& transaction,
114-
const CommitOptions& commitOptions) const;
114+
const CommitOptions& commitOptions,
115+
bool hasLocked = false) const;
115116

116117
/*
117118
* Calls `tryCommit` in a loop until it finishes successfully.

0 commit comments

Comments
 (0)