Skip to content

Commit 2f9a2e9

Browse files
Fabio Carballofacebook-github-bot
Fabio Carballo
authored andcommitted
Add more fine-grainted mechanism to understand if TreeFuture was cancelled by Policy
Summary: This is a bit of a defensive approach to try to avoid the issues with the cancellation experiment. The steps are the following: 1. Adding a flag in `TreeFuture` that marks it as "cancelled" whenever we call cancel due to our policy. 2. When we cancel the Future, it can happen that an exception is triggered. If so, we verify if it is one of the expected exceptions **and** if it was marked cancelled by us. If that is the case, we throw a specific `FutureCancelledByPolicyException` and this should be caught in the `CancellationHelper` and then a `TreeFutureResult.cancelled()` should be returned. Reviewed By: adityasharat Differential Revision: D44495245 fbshipit-source-id: 29d0d02d9d6433e3a58ace9e5e82d0334fa17e5a
1 parent f67c80a commit 2f9a2e9

9 files changed

+170
-39
lines changed

litho-core/src/main/java/com/facebook/litho/ResolveTreeFuture.java

+2-1
Original file line numberDiff line numberDiff line change
@@ -107,7 +107,8 @@ public ResolveTreeFuture(
107107
resolveVersion,
108108
component.getId(),
109109
mComponentContext.getTreeProps(),
110-
ExecutionModeKt.getExecutionMode(source));
110+
ExecutionModeKt.getExecutionMode(source),
111+
source);
111112

112113
// Allow interrupt to happen during tryRegisterForResponse when config is enabled.
113114
mEnableEarlyInterrupt = ComponentsConfiguration.isInterruptEarlyWithSplitFuturesEnabled;

litho-core/src/main/java/com/facebook/litho/TreeFuture.java

+35-7
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@
2626
import android.os.Process;
2727
import androidx.annotation.Nullable;
2828
import androidx.annotation.VisibleForTesting;
29+
import com.facebook.litho.cancellation.FutureCancelledByPolicyException;
2930
import com.facebook.litho.config.ComponentsConfiguration;
3031
import com.facebook.rendercore.Systracer;
3132
import com.facebook.rendercore.instrumentation.FutureInstrumenter;
@@ -102,6 +103,13 @@ public TreeFutureResult<T> call() {
102103
/** Returns an integer that identifies uniquely the version of this {@link TreeFuture}. */
103104
public abstract int getVersion();
104105

106+
/**
107+
* This indicates if this {@link TreeFuture} was canceleled by the framework. If so, we can
108+
* consider that any {@link InterruptedException} or {@link CancellationException} was actively
109+
* triggered by us.
110+
*/
111+
private boolean mCancelled = false;
112+
105113
/**
106114
* This method will forcefully cancel any possible execution or remaining execution of the task
107115
* associated to this {@link TreeFuture}.
@@ -118,8 +126,15 @@ public TreeFutureResult<T> call() {
118126
* @return false if the task could not be cancelled, typically because it has already completed
119127
* normally; true otherwise
120128
*/
121-
public boolean forceCancellation() {
122-
return mFutureTask.cancel(true);
129+
public boolean cancel(boolean useInterruption) {
130+
mCancelled = true;
131+
132+
if (useInterruption) {
133+
return mFutureTask.cancel(true);
134+
} else {
135+
release();
136+
return true;
137+
}
123138
}
124139

125140
/** Calculates a new result for this TreeFuture. */
@@ -341,7 +356,7 @@ TreeFutureResult<T> runAndGet(@LayoutState.CalculateLayoutSource final int sourc
341356
didRaiseThreadPriority = false;
342357
}
343358

344-
TreeFutureResult<T> treeFutureResult;
359+
TreeFutureResult<T> treeFutureResult = null;
345360

346361
final boolean shouldTrace = notRunningOnMyThread && ComponentsSystrace.isTracing();
347362
try {
@@ -404,24 +419,37 @@ TreeFutureResult<T> runAndGet(@LayoutState.CalculateLayoutSource final int sourc
404419
} catch (ExecutionException | InterruptedException | CancellationException e) {
405420
onWaitEnd(shouldTrace, true);
406421

407-
final Throwable cause = e.getCause();
408-
if (cause instanceof RuntimeException) {
409-
throw (RuntimeException) cause;
422+
if (isExpectedInterruptionOrCancellation(e)) {
423+
throw new FutureCancelledByPolicyException();
410424
} else {
411-
throw new RuntimeException(e.getMessage(), e);
425+
final Throwable cause = e.getCause();
426+
if (cause instanceof RuntimeException) {
427+
throw (RuntimeException) cause;
428+
} else {
429+
throw new RuntimeException(e.getMessage(), e);
430+
}
412431
}
413432
} finally {
414433
onGetEnd(shouldTrace);
415434
}
416435

417436
synchronized (TreeFuture.this) {
437+
if (mCancelled) {
438+
return TreeFutureResult.cancelled();
439+
}
440+
418441
if (mReleased) {
419442
return TreeFutureResult.interruptWithMessage(FUTURE_RESULT_NULL_REASON_RELEASED);
420443
}
444+
421445
return treeFutureResult;
422446
}
423447
}
424448

449+
private boolean isExpectedInterruptionOrCancellation(Exception e) {
450+
return (e instanceof CancellationException || e instanceof InterruptedException) && mCancelled;
451+
}
452+
425453
/**
426454
* Given a provided tree-future, this method will track it via a given list, and run it.
427455
*

litho-core/src/main/java/com/facebook/litho/cancellation/CancellationHelper.kt

+3-10
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,6 @@ import com.facebook.litho.TreeFuture.TreeFutureResult
2626
import com.facebook.litho.cancellation.CancellationPolicy.CancellationExecutionMode
2727
import com.facebook.litho.stats.LithoStats
2828
import java.util.LinkedList
29-
import java.util.concurrent.CancellationException
3029

3130
/**
3231
* This is used to run either *Resolve* or *Layout* with a cancellation strategy.
@@ -79,10 +78,8 @@ F : RequestMetadataSupplier<M> {
7978

8079
return try {
8180
TreeFuture.trackAndRunTreeFuture(treeFuture, futures, source, mutex, futureExecutionListener)
82-
} catch (exception: RuntimeException) {
83-
if (exception.cause is CancellationException) {
84-
debugLog { "A cancellation exception on future ${treeFuture.version}" }
85-
}
81+
} catch (e: FutureCancelledByPolicyException) {
82+
debugLog { "Future was cancelled by CancellationPolicy" }
8683
TreeFutureResult.cancelled()
8784
}
8885
}
@@ -109,11 +106,7 @@ T : PotentiallyPartialResult {
109106
if (futureToCancel != null) {
110107
incrementCancellationStats(attribution)
111108

112-
when (cancellationMode) {
113-
CancellationExecutionMode.SHORT_CIRCUIT -> futureToCancel.release()
114-
CancellationExecutionMode.INTERRUPT -> futureToCancel.forceCancellation()
115-
}
116-
109+
futureToCancel.cancel(cancellationMode == CancellationExecutionMode.INTERRUPT)
117110
cancelledFutures.add(futureToCancel)
118111

119112
debugLog { "Cancelled future (${futureToCancel.version}) with success." }
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
/*
2+
* Copyright (c) Meta Platforms, Inc. and affiliates.
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* http://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
17+
package com.facebook.litho.cancellation
18+
19+
/**
20+
* This exception should only be thrown whenever there is an [InterruptedException] or
21+
* [CancellationException] thrown inside a [TreeFuture] while being marked with
22+
* [TreeFuture.isCancelled]
23+
*/
24+
internal class FutureCancelledByPolicyException() : RuntimeException()

litho-core/src/main/java/com/facebook/litho/cancellation/LayoutCancellationPolicy.kt

+1-1
Original file line numberDiff line numberDiff line change
@@ -72,7 +72,7 @@ interface LayoutCancellationPolicy : CancellationPolicy<LayoutMetadata> {
7272
}
7373

7474
return if (cancellableRequests.isNotEmpty()) {
75-
Result.CancelRunningRequests(ongoingRequests.map(LayoutMetadata::localVersion))
75+
Result.CancelRunningRequests(cancellableRequests.map(LayoutMetadata::localVersion))
7676
} else {
7777
Result.ProcessIncomingRequest
7878
}

litho-core/src/main/java/com/facebook/litho/cancellation/ResolveCancellationPolicy.kt

+34-10
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818

1919
package com.facebook.litho.cancellation
2020

21+
import com.facebook.litho.LayoutState
2122
import com.facebook.litho.cancellation.CancellationPolicy.Result
2223

2324
/**
@@ -58,18 +59,25 @@ interface ResolveCancellationPolicy : CancellationPolicy<ResolveMetadata> {
5859

5960
for (runningResolve in ongoingRequests) {
6061
if (runningResolve.isEquivalentTo(incomingRequest)) {
61-
if (incomingRequest.executionMode == ExecutionMode.ASYNC) {
62-
return Result.DropIncomingRequest
62+
when {
63+
incomingRequest.isAsync && !incomingRequest.isStateUpdate -> {
64+
return Result.DropIncomingRequest
65+
}
66+
incomingRequest.isAsync &&
67+
incomingRequest.isStateUpdate &&
68+
!runningResolve.isStateUpdate -> {
69+
return Result.DropIncomingRequest
70+
}
6371
}
6472
} else {
65-
if (runningResolve.executionMode == ExecutionMode.ASYNC) {
73+
if (runningResolve.isAsync) {
6674
cancellableResolves.add(runningResolve)
6775
}
6876
}
6977
}
7078

7179
return if (cancellableResolves.isNotEmpty()) {
72-
Result.CancelRunningRequests(ongoingRequests.map { it.id })
80+
Result.CancelRunningRequests(cancellableResolves.map { it.id })
7381
} else {
7482
Result.ProcessIncomingRequest
7583
}
@@ -100,23 +108,39 @@ interface ResolveCancellationPolicy : CancellationPolicy<ResolveMetadata> {
100108

101109
for (runningResolve in ongoingRequests) {
102110
if (runningResolve.isEquivalentTo(incomingRequest)) {
103-
if (incomingRequest.executionMode == ExecutionMode.ASYNC) {
104-
return Result.DropIncomingRequest
105-
} else if (runningResolve.executionMode == ExecutionMode.ASYNC) {
106-
cancellableResolves.add(runningResolve)
111+
when {
112+
incomingRequest.isAsync && !incomingRequest.isStateUpdate -> {
113+
return Result.DropIncomingRequest
114+
}
115+
incomingRequest.isAsync &&
116+
incomingRequest.isStateUpdate &&
117+
!runningResolve.isStateUpdate -> {
118+
return Result.DropIncomingRequest
119+
}
120+
runningResolve.isAsync -> {
121+
cancellableResolves.add(runningResolve)
122+
}
107123
}
108124
} else {
109-
if (runningResolve.executionMode == ExecutionMode.ASYNC) {
125+
if (runningResolve.isAsync) {
110126
cancellableResolves.add(runningResolve)
111127
}
112128
}
113129
}
114130

115131
return if (cancellableResolves.isNotEmpty()) {
116-
Result.CancelRunningRequests(ongoingRequests.map { it.id })
132+
Result.CancelRunningRequests(cancellableResolves.map { it.id })
117133
} else {
118134
Result.ProcessIncomingRequest
119135
}
120136
}
121137
}
122138
}
139+
140+
private val ResolveMetadata.isAsync: Boolean
141+
get() = executionMode == ExecutionMode.ASYNC
142+
143+
private val ResolveMetadata.isStateUpdate: Boolean
144+
get() =
145+
source == LayoutState.CalculateLayoutSource.UPDATE_STATE_ASYNC ||
146+
source == LayoutState.CalculateLayoutSource.UPDATE_STATE_SYNC

litho-core/src/main/java/com/facebook/litho/cancellation/ResolveMetadata.kt

+3-1
Original file line numberDiff line numberDiff line change
@@ -18,14 +18,16 @@
1818

1919
package com.facebook.litho.cancellation
2020

21+
import com.facebook.litho.LayoutState
2122
import com.facebook.litho.TreeProps
2223

2324
/** This metadata encompasses all characteristics associated to a specific `Resolve` request. */
2425
data class ResolveMetadata(
2526
val localVersion: Int,
2627
val componentId: Int,
2728
val treeProps: TreeProps?,
28-
val executionMode: ExecutionMode
29+
val executionMode: ExecutionMode,
30+
@LayoutState.CalculateLayoutSource val source: Int
2931
) {
3032

3133
val id: Int = localVersion

litho-core/src/test/java/com/facebook/litho/cancellation/DefaultResolveCancellationPolicyTest.kt

+33-4
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818

1919
package com.facebook.litho.cancellation
2020

21+
import com.facebook.litho.LayoutState
2122
import com.facebook.litho.TreeProps
2223
import com.facebook.litho.cancellation.CancellationPolicy.CancellationExecutionMode
2324
import com.facebook.litho.cancellation.CancellationPolicy.Result
@@ -112,18 +113,45 @@ class DefaultResolveCancellationPolicyTest {
112113
}
113114

114115
@Test
115-
fun `on async resolve request with equivalent async request running - drop incoming request`() {
116+
fun `on async non-state update resolve request with equivalent async request running - drop incoming request`() {
116117
val runningResolves =
117-
listOf(resolveMetadata.copy(localVersion = 0, executionMode = ExecutionMode.ASYNC))
118+
listOf(
119+
resolveMetadata.copy(
120+
localVersion = 0,
121+
executionMode = ExecutionMode.ASYNC,
122+
source = LayoutState.CalculateLayoutSource.UPDATE_STATE_ASYNC))
118123

119124
val incomingResolve =
120-
resolveMetadata.copy(executionMode = ExecutionMode.ASYNC, localVersion = 1)
125+
resolveMetadata.copy(
126+
executionMode = ExecutionMode.ASYNC,
127+
localVersion = 1,
128+
source = LayoutState.CalculateLayoutSource.SET_ROOT_ASYNC)
121129

122130
val result = cancellationEvaluator.evaluate(runningResolves, incomingResolve)
123131

124132
assertThat(result).isEqualTo(Result.DropIncomingRequest)
125133
}
126134

135+
@Test
136+
fun `on async state update resolve request with equivalent async state update request running - process incoming request`() {
137+
val runningResolves =
138+
listOf(
139+
resolveMetadata.copy(
140+
localVersion = 0,
141+
executionMode = ExecutionMode.ASYNC,
142+
source = LayoutState.CalculateLayoutSource.UPDATE_STATE_ASYNC))
143+
144+
val incomingResolve =
145+
resolveMetadata.copy(
146+
executionMode = ExecutionMode.ASYNC,
147+
localVersion = 1,
148+
source = LayoutState.CalculateLayoutSource.UPDATE_STATE_ASYNC)
149+
150+
val result = cancellationEvaluator.evaluate(runningResolves, incomingResolve)
151+
152+
assertThat(result).isEqualTo(Result.ProcessIncomingRequest)
153+
}
154+
127155
@Test
128156
fun `on sync resolve request with equivalent sync request running - drop incoming request`() {
129157
val runningResolves =
@@ -153,5 +181,6 @@ class DefaultResolveCancellationPolicyTest {
153181
componentId = 1,
154182
localVersion = 0,
155183
treeProps = TreeProps().apply { put(String::class.java, "a property") },
156-
executionMode = ExecutionMode.SYNC)
184+
executionMode = ExecutionMode.SYNC,
185+
source = LayoutState.CalculateLayoutSource.SET_ROOT_SYNC)
157186
}

0 commit comments

Comments
 (0)