Skip to content

Commit

Permalink
Workaround for NativeAnimatedModule queue race condition
Browse files Browse the repository at this point in the history
Summary:
Apparently it's possible for `!isEmpty()` to be true and `peek` to be non-null, but for `poll()` to be false. It doesn't really make sense to me, and I don't have a repro, but that's what logs are showing.

I suspect a teardown race condition or /maybe/ a Fabric/non-Fabric handoff race condition. Neither of those make a ton of sense, though.

The mitigation is fairly straightforward: we are just much more cautious with how we treat the queue and assume everything could be null.

This impacts Fabric and non-Fabric equally.

Changelog: [Internal]

Reviewed By: motiz88

Differential Revision: D22593924

fbshipit-source-id: 7748121951a64941fa6da2bd25ebf070be6dc89c
  • Loading branch information
JoshuaGross authored and facebook-github-bot committed Jul 17, 2020
1 parent 15dda0a commit 980900c
Showing 1 changed file with 32 additions and 4 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -104,10 +104,12 @@ public long getFrameNumber() {
private final ReactChoreographer mReactChoreographer;

@NonNull
private ConcurrentLinkedQueue<UIThreadOperation> mOperations = new ConcurrentLinkedQueue<>();
private final ConcurrentLinkedQueue<UIThreadOperation> mOperations =
new ConcurrentLinkedQueue<>();

@NonNull
private ConcurrentLinkedQueue<UIThreadOperation> mPreOperations = new ConcurrentLinkedQueue<>();
private final ConcurrentLinkedQueue<UIThreadOperation> mPreOperations =
new ConcurrentLinkedQueue<>();

private @Nullable NativeAnimatedNodesManager mNodesManager;

Expand Down Expand Up @@ -226,8 +228,34 @@ public void didDispatchMountItems(UIManager uiManager) {

private void executeAllOperations(Queue<UIThreadOperation> operationQueue, long maxFrameNumber) {
NativeAnimatedNodesManager nodesManager = getNodesManager();
while (!operationQueue.isEmpty() && operationQueue.peek().getFrameNumber() <= maxFrameNumber) {
operationQueue.poll().execute(nodesManager);
while (true) {
// There is a race condition where `peek` may return a non-null value and isEmpty() is false,
// but `poll` returns a null value - it's not clear why since we only peek and poll on the UI
// thread, but it might be something that happens during teardown or a crash. Regardless, the
// root cause is not currently known so we're extra cautious here.
// It happens equally in Fabric and non-Fabric.
UIThreadOperation peekedOperation = operationQueue.peek();

// This is the same as operationQueue.isEmpty()
if (peekedOperation == null) {
return;
}
// The rest of the operations are for the next frame.
if (peekedOperation.getFrameNumber() > maxFrameNumber) {
return;
}

// Since we apparently can't guarantee that there is still an operation on the queue,
// much less the same operation, we do a poll and another null check. If this isn't
// the same operation as the peeked operation, we can't do anything about it - we still
// need to execute it, we have no mechanism to put it at the front of the queue, and it
// won't cause any errors to execute it earlier than expected (just a bit of UI jank at worst)
// so we just continue happily along.
UIThreadOperation polledOperation = operationQueue.poll();
if (polledOperation == null) {
return;
}
polledOperation.execute(nodesManager);
}
}

Expand Down

0 comments on commit 980900c

Please sign in to comment.