Skip to content

Commit 980900c

Browse files
JoshuaGrossfacebook-github-bot
authored andcommitted
Workaround for NativeAnimatedModule queue race condition
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
1 parent 15dda0a commit 980900c

File tree

1 file changed

+32
-4
lines changed

1 file changed

+32
-4
lines changed

ReactAndroid/src/main/java/com/facebook/react/animated/NativeAnimatedModule.java

Lines changed: 32 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -104,10 +104,12 @@ public long getFrameNumber() {
104104
private final ReactChoreographer mReactChoreographer;
105105

106106
@NonNull
107-
private ConcurrentLinkedQueue<UIThreadOperation> mOperations = new ConcurrentLinkedQueue<>();
107+
private final ConcurrentLinkedQueue<UIThreadOperation> mOperations =
108+
new ConcurrentLinkedQueue<>();
108109

109110
@NonNull
110-
private ConcurrentLinkedQueue<UIThreadOperation> mPreOperations = new ConcurrentLinkedQueue<>();
111+
private final ConcurrentLinkedQueue<UIThreadOperation> mPreOperations =
112+
new ConcurrentLinkedQueue<>();
111113

112114
private @Nullable NativeAnimatedNodesManager mNodesManager;
113115

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

227229
private void executeAllOperations(Queue<UIThreadOperation> operationQueue, long maxFrameNumber) {
228230
NativeAnimatedNodesManager nodesManager = getNodesManager();
229-
while (!operationQueue.isEmpty() && operationQueue.peek().getFrameNumber() <= maxFrameNumber) {
230-
operationQueue.poll().execute(nodesManager);
231+
while (true) {
232+
// There is a race condition where `peek` may return a non-null value and isEmpty() is false,
233+
// but `poll` returns a null value - it's not clear why since we only peek and poll on the UI
234+
// thread, but it might be something that happens during teardown or a crash. Regardless, the
235+
// root cause is not currently known so we're extra cautious here.
236+
// It happens equally in Fabric and non-Fabric.
237+
UIThreadOperation peekedOperation = operationQueue.peek();
238+
239+
// This is the same as operationQueue.isEmpty()
240+
if (peekedOperation == null) {
241+
return;
242+
}
243+
// The rest of the operations are for the next frame.
244+
if (peekedOperation.getFrameNumber() > maxFrameNumber) {
245+
return;
246+
}
247+
248+
// Since we apparently can't guarantee that there is still an operation on the queue,
249+
// much less the same operation, we do a poll and another null check. If this isn't
250+
// the same operation as the peeked operation, we can't do anything about it - we still
251+
// need to execute it, we have no mechanism to put it at the front of the queue, and it
252+
// won't cause any errors to execute it earlier than expected (just a bit of UI jank at worst)
253+
// so we just continue happily along.
254+
UIThreadOperation polledOperation = operationQueue.poll();
255+
if (polledOperation == null) {
256+
return;
257+
}
258+
polledOperation.execute(nodesManager);
231259
}
232260
}
233261

0 commit comments

Comments
 (0)