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.

Sign up for GitHub

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

Jump to bottom

Fix issue causing ANR when performing sync layout updates #4239

Merged
merged 3 commits into from
Mar 21, 2023
Merged

Fix issue causing ANR when performing sync layout updates #4239

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d4de467
Fix issue causing ANR when performing sync layout updates on iOS
kmagiera Mar 17, 2023
eaea90d
Add timeout for semaphore wait fort sync layout updates on Android
kmagiera Mar 20, 2023
8daa5ac
Fix lint
kmagiera Mar 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 6 additions & 7 deletions android/src/main/java/com/swmansion/reanimated/NodesManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
import java.util.Set;
import java.util.concurrent.ConcurrentLinkedQueue;
import java.util.concurrent.Semaphore;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.atomic.AtomicBoolean;
import javax.annotation.Nullable;

Expand Down Expand Up @@ -218,13 +219,11 @@ public void runGuarded() {
}
});
if (trySynchronously) {
while (true) {
try {
semaphore.acquire();
break;
} catch (InterruptedException e) {
// noop
}
try {
semaphore.tryAcquire(16, TimeUnit.MILLISECONDS);
} catch (InterruptedException e) {
// if the thread is interruped we just continue and let the layout update happen
// asynchronously
}
}
}
Expand Down
27 changes: 23 additions & 4 deletions ios/REANodesManager.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,8 @@ @implementation REANodesManager {
BOOL _tryRunBatchUpdatesSynchronously;
REAEventHandler _eventHandler;
volatile void (^_mounting)(void);
NSObject *_syncLayoutUpdatesWaitLock;
volatile BOOL _syncLayoutUpdatesWaitTimedOut;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. We should set initial value for _syncLayoutUpdatesWaitTimedOut - just for sure.
  2. Maybe the usage of atomic boolean instead of volatile BOOL + lock would be better approach here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. we always reset that bool prior to the place where we read from it
  2. this won't work as we want to synchronize more changes, specifically setting _mounting property also happens inside of the synchronized block and has to be synchronized for this to work. Using atomic bool won't suffice

NSMutableDictionary<NSNumber *, ComponentUpdate *> *_componentUpdateBuffer;
NSMutableDictionary<NSNumber *, UIView *> *_viewRegistry;
#ifdef RCT_NEW_ARCH_ENABLED
Expand Down Expand Up @@ -123,6 +125,7 @@ - (nonnull instancetype)initWithModule:(REAModule *)reanimatedModule
_operationsInBatch = [NSMutableDictionary new];
_componentUpdateBuffer = [NSMutableDictionary new];
_viewRegistry = [_uiManager valueForKey:@"_viewRegistry"];
_syncLayoutUpdatesWaitLock = [NSObject new];
}

_displayLink = [CADisplayLink displayLinkWithTarget:self selector:@selector(onAnimationFrame:)];
Expand Down Expand Up @@ -241,8 +244,14 @@ - (void)onAnimationFrame:(CADisplayLink *)displayLink
- (BOOL)uiManager:(RCTUIManager *)manager performMountingWithBlock:(RCTUIManagerMountingBlock)block
{
RCTAssert(_mounting == nil, @"Mouting block is expected to not be set");
_mounting = block;
return YES;
@synchronized(_syncLayoutUpdatesWaitLock) {
if (_syncLayoutUpdatesWaitTimedOut) {
return NO;
} else {
_mounting = block;
return YES;
}
}
}

- (void)performOperations
Expand All @@ -260,6 +269,7 @@ - (void)performOperations

__weak __typeof__(self) weakSelf = self;
dispatch_semaphore_t semaphore = dispatch_semaphore_create(0);
_syncLayoutUpdatesWaitTimedOut = NO;
RCTExecuteOnUIManagerQueue(^{
__typeof__(self) strongSelf = weakSelf;
if (strongSelf == nil) {
Expand All @@ -276,15 +286,24 @@ - (void)performOperations
}

if (canUpdateSynchronously) {
[strongSelf.uiManager runSyncUIUpdatesWithObserver:self];
[strongSelf.uiManager runSyncUIUpdatesWithObserver:strongSelf];
dispatch_semaphore_signal(semaphore);
}
// In case canUpdateSynchronously=true we still have to send uiManagerWillPerformMounting event
// to observers because some components (e.g. TextInput) update their UIViews only on that event.
[strongSelf.uiManager setNeedsLayout];
});
if (trySynchronously) {
dispatch_semaphore_wait(semaphore, DISPATCH_TIME_FOREVER);
// The 16ms timeout here aims to match the frame duration. It may make sense to read that parameter
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about 120 fps mode?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was trying to keep this change minimal as may want to port it back to Rea 2. We could pass expected frame duration here but we don't always have it available (e.g. we have it when we run operations from CADisplayLink callback but when handling events display link instance could be paused). In my opinion the consequences of this timeout being longer than one frame on 120fps devices isn't very serious. This deadlock happens very randomly and not that often. If the timeout takes two frames we will just drop one extra frame on such device.

// from CADisplayLink but it is easier to hardcode it for the time being.
// The reason why we use frame duration here is that if takes longer than one frame to complete layout tasks
// there is no point of synchronizing layout with the UI interaction as we get that one frame delay anyways.
long result = dispatch_semaphore_wait(semaphore, dispatch_time(DISPATCH_TIME_NOW, 16 * NSEC_PER_MSEC));
if (result != 0) {
@synchronized(_syncLayoutUpdatesWaitLock) {
_syncLayoutUpdatesWaitTimedOut = YES;
}
}
}

if (_mounting) {
Expand Down