Skip to content

Commit d1c8377

Browse files
copybara-androidxtestcopybara-androidxtest
committed
Handle potential race condition when getting busy resources in UiControllerImpl.
Adds a new IdleCondition, POSSIBLE_RACE_CONDITION_DETECTED, to address a scenario where IdlingResourceRegistry#getBusyResources() might return null during the DYNAMIC_TASKS_HAVE_IDLED check. When null is detected, a signal is sent, and the system loops until the race has been handled / exception is thrown. PiperOrigin-RevId: 919193080
1 parent 75240e8 commit d1c8377

5 files changed

Lines changed: 106 additions & 22 deletions

File tree

‎espresso/CHANGELOG.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ The following artifacts were released:
1919
* Replace now-unnecessary reflection from TestLooperManagerCompat when using Android SDK 36 APIs
2020
* Don't suppress AppNotIdleException if dumpThreadStates throws.
2121
* Remove Espresso.onIdle tracing
22+
* Fix NullPointerException in UiControllerImpl.
2223

2324
**New Features**
2425

‎espresso/core/java/androidx/test/espresso/base/IdlingResourceRegistry.java‎

Lines changed: 16 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@
2626
import android.os.Message;
2727
import android.util.Log;
2828
import androidx.annotation.NonNull;
29+
import androidx.annotation.Nullable;
2930
import androidx.test.espresso.IdlingPolicies;
3031
import androidx.test.espresso.IdlingPolicy;
3132
import androidx.test.espresso.IdlingResource;
@@ -363,7 +364,8 @@ private void scheduleTimeoutMessages() {
363364
timeoutError, error.getIdleTimeoutUnit().toMillis(error.getIdleTimeout()));
364365
}
365366

366-
List<String> getBusyResources() {
367+
@Nullable
368+
private List<String> pollBusyResourceNames() {
367369
List<String> busyResourceNames = new ArrayList<>();
368370
List<IdlingState> racyResources = new ArrayList<>();
369371

@@ -390,6 +392,17 @@ List<String> getBusyResources() {
390392
}
391393
}
392394

395+
@NonNull
396+
String describeBusyResources() {
397+
List<String> busyResourceNames = new ArrayList<>();
398+
for (IdlingState state : idlingStates) {
399+
if (!state.idle) {
400+
busyResourceNames.add(state.resource.getName());
401+
}
402+
}
403+
return String.join(", ", busyResourceNames);
404+
}
405+
393406
private class IdlingState implements ResourceCallback {
394407
// on main
395408
final IdlingResource resource;
@@ -508,7 +521,7 @@ private void handleResourceIdled(Message m) {
508521
}
509522

510523
private void handleTimeoutWarning() {
511-
List<String> busyResources = getBusyResources();
524+
List<String> busyResources = pollBusyResourceNames();
512525
if (busyResources == null) {
513526
// null indicates that there is either a race or a programming error
514527
// a race detector message has been inserted into the q.
@@ -525,7 +538,7 @@ private void handleTimeoutWarning() {
525538
}
526539

527540
private void handleTimeout() {
528-
List<String> busyResources = getBusyResources();
541+
List<String> busyResources = pollBusyResourceNames();
529542
if (busyResources == null) {
530543
// detected a possible race... we've enqueued a race busting message
531544
// so either that'll resolve the race or kill the app because it's buggy.

‎espresso/core/java/androidx/test/espresso/base/UiControllerImpl.java‎

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -35,9 +35,7 @@
3535
import androidx.test.espresso.IdlingPolicies;
3636
import androidx.test.espresso.IdlingPolicy;
3737
import androidx.test.espresso.InjectEventSecurityException;
38-
import androidx.test.espresso.UiController;
3938
import androidx.test.espresso.base.IdlingResourceRegistry.IdleNotificationCallback;
40-
import androidx.test.espresso.util.StringJoinerKt;
4139
import androidx.test.espresso.util.concurrent.ThreadFactoryBuilder;
4240
import java.util.ArrayList;
4341
import java.util.BitSet;
@@ -77,7 +75,8 @@ enum IdleCondition {
7775
COMPAT_TASKS_HAVE_IDLED,
7876
KEY_INJECT_HAS_COMPLETED,
7977
MOTION_INJECTION_HAS_COMPLETED,
80-
DYNAMIC_TASKS_HAVE_IDLED;
78+
DYNAMIC_TASKS_HAVE_IDLED,
79+
POSSIBLE_RACE_CONDITION_DETECTED;
8180

8281
/** Checks whether this condition has been signaled. */
8382
public boolean isSignaled(BitSet conditionSet) {
@@ -156,7 +155,6 @@ private enum InterrogationStatus {
156155
private IdleNotifier<Runnable> asyncIdle;
157156
private IdleNotifier<Runnable> compatIdle;
158157
private Provider<IdleNotifier<IdleNotificationCallback>> dynamicIdleProvider;
159-
private Interrogator interrogator;
160158

161159
@VisibleForTesting
162160
@Inject
@@ -550,13 +548,12 @@ private IdleNotifier<IdleNotificationCallback> loopUntil(
550548
dynamicIdle = dynamicIdleProvider.get();
551549
}
552550

553-
List<String> busyResources = idlingResourceRegistry.getBusyResources();
554551
conditionName =
555552
String.format(
556553
Locale.ROOT,
557554
"%s(busy resources=%s)",
558555
conditionName,
559-
StringJoinerKt.joinToString(busyResources, ","));
556+
idlingResourceRegistry.describeBusyResources());
560557
break;
561558
default:
562559
break;
@@ -588,6 +585,7 @@ private IdleNotifier<IdleNotificationCallback> loopUntil(
588585
}
589586
return dynamicIdle;
590587
}
588+
591589
@Override
592590
public void interruptEspressoTasks() {
593591
controllerHandler.post(

‎espresso/core/javatests/androidx/test/espresso/base/BUILD‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -236,7 +236,6 @@ axt_android_library_test(
236236
"//espresso/core/java/androidx/test/espresso/base:default_failure_handler",
237237
"//espresso/core/java/androidx/test/espresso/base:idling_resource_registry",
238238
"//ext/junit",
239-
"//opensource/androidx:annotation",
240239
"//runner/android_junit_runner/java/androidx/test:runner",
241240
"//testapps/ui_testapp/java/androidx/test/ui/app:lib_exported",
242241
"//testapps/ui_testapp/javatests/androidx/test/ui/app:test_resources",

‎espresso/core/javatests/androidx/test/espresso/base/UiControllerImplTest.java‎

Lines changed: 85 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -20,12 +20,17 @@
2020
import static junit.framework.Assert.assertTrue;
2121
import static org.junit.Assert.assertEquals;
2222
import static org.junit.Assert.assertFalse;
23+
import static org.junit.Assert.assertThrows;
2324
import static org.junit.Assert.fail;
2425

2526
import android.os.Handler;
2627
import android.os.Looper;
2728
import android.os.Message;
29+
import android.os.SystemClock;
2830
import android.util.Log;
31+
import androidx.test.espresso.AppNotIdleException;
32+
import androidx.test.espresso.IdlingPolicies;
33+
import androidx.test.espresso.IdlingPolicy;
2934
import androidx.test.espresso.IdlingResourceTimeoutException;
3035
import androidx.test.espresso.base.IdlingResourceRegistry.IdleNotificationCallback;
3136
import androidx.test.ext.junit.runners.AndroidJUnit4;
@@ -95,6 +100,10 @@ public Handler getHandler() {
95100

96101
@Before
97102
public void setUp() throws Exception {
103+
// Reset the master policy timeout to the default value.
104+
IdlingPolicies.setMasterPolicyTimeout(2, TimeUnit.SECONDS);
105+
IdlingPolicies.setIdlingResourceTimeout(1, TimeUnit.SECONDS);
106+
98107
testThread = new LooperThread();
99108
testThread.setUncaughtExceptionHandler(
100109
new Thread.UncaughtExceptionHandler() {
@@ -127,6 +136,10 @@ public IdleNotifier<IdleNotificationCallback> get() {
127136

128137
@After
129138
public void tearDown() throws Exception {
139+
// Reset the master policy timeout to the default value.
140+
IdlingPolicies.setMasterPolicyTimeout(60, TimeUnit.SECONDS);
141+
IdlingPolicies.setIdlingResourceTimeout(26, TimeUnit.SECONDS);
142+
130143
testThread.quitLooper();
131144
asyncPool.shutdown();
132145
}
@@ -319,10 +332,10 @@ public void run() {
319332
}
320333
}));
321334
assertFalse(
322-
"Should not have stopped looping the main thread yet!", latch.await(2, TimeUnit.SECONDS));
335+
"Should not have stopped looping the main thread yet!", latch.await(1, TimeUnit.SECONDS));
323336
assertEquals("Not all main thread tasks have checked in", 1L, latch.getCount());
324337
asyncTaskShouldComplete.countDown();
325-
assertTrue("App should be idle.", latch.await(5, TimeUnit.SECONDS));
338+
assertTrue("App should be idle.", latch.await(1, TimeUnit.SECONDS));
326339
}
327340

328341
@Test
@@ -366,9 +379,63 @@ public void run() {
366379
}
367380
}));
368381
assertFalse(
369-
"Should not have stopped looping the main thread yet!", latch.await(2, TimeUnit.SECONDS));
382+
"Should not have stopped looping the main thread yet!",
383+
latch.await(100, TimeUnit.MILLISECONDS));
370384
fakeResource.forceIdleNow();
371-
assertTrue("App should be idle.", latch.await(5, TimeUnit.SECONDS));
385+
assertTrue("App should be idle.", latch.await(1, TimeUnit.SECONDS));
386+
}
387+
388+
@Test
389+
public void loopMainThreadUntilIdle_racyIdleResource() throws InterruptedException {
390+
IdlingPolicies.setMasterPolicyTimeout(100, TimeUnit.MILLISECONDS);
391+
OnDemandIdlingResource fakeResource = new OnDemandIdlingResource("FakeResource");
392+
idlingResourceRegistry.registerResources(singletonList(fakeResource));
393+
final CountDownLatch startedLatch = new CountDownLatch(1);
394+
final CountDownLatch interruptedLatch = new CountDownLatch(1);
395+
assertTrue(
396+
testThread
397+
.getHandler()
398+
.post(
399+
() -> {
400+
// This is the sequence of events we want:
401+
// - Resource starts busy.
402+
// - Resource becomes idle, event posted to queue to update idle registry.
403+
// - Timeout occurs, no more events will be processed.
404+
// - Espresso detects race condition while checking if the resource is idle.
405+
IdlingPolicy masterIdlingPolicy = IdlingPolicies.getMasterIdlingPolicy();
406+
long expectedTimeout =
407+
SystemClock.uptimeMillis()
408+
+ masterIdlingPolicy
409+
.getIdleTimeoutUnit()
410+
.toMillis(masterIdlingPolicy.getIdleTimeout());
411+
testThread
412+
.getHandler()
413+
.post(
414+
() -> {
415+
Log.i(TAG, "Forcing resource to be idle.");
416+
fakeResource.forceIdleNow();
417+
// Busy wait until after the timeout to ensure race buster cannot run.
418+
try {
419+
Thread.sleep(200);
420+
} catch (InterruptedException e) {
421+
throw new RuntimeException(e);
422+
}
423+
assertTrue(
424+
"Sleep should bring us past the timeout.",
425+
SystemClock.uptimeMillis() > expectedTimeout);
426+
});
427+
428+
Log.i(TAG, "Hijacking thread and looping it.");
429+
startedLatch.countDown();
430+
assertThrows(
431+
"Expected for loopMainThreadUntilIdle to be interrupted",
432+
AppNotIdleException.class,
433+
() -> uiController.get().loopMainThreadUntilIdle());
434+
interruptedLatch.countDown();
435+
}));
436+
437+
assertTrue("looper has started.", startedLatch.await(2, TimeUnit.SECONDS));
438+
assertTrue("App should be interrupted.", interruptedLatch.await(5, TimeUnit.SECONDS));
372439
}
373440

374441
@Test
@@ -392,18 +459,22 @@ public void run() {
392459
}
393460
}));
394461
assertFalse(
395-
"Should not have stopped looping the main thread yet!", latch.await(1, TimeUnit.SECONDS));
462+
"Should not have stopped looping the main thread yet!",
463+
latch.await(100, TimeUnit.MILLISECONDS));
396464
fakeResource1.forceIdleNow();
397465
assertFalse(
398-
"Should not have stopped looping the main thread yet!", latch.await(1, TimeUnit.SECONDS));
466+
"Should not have stopped looping the main thread yet!",
467+
latch.await(100, TimeUnit.MILLISECONDS));
399468
idlingResourceRegistry.registerResources(singletonList(fakeResource3));
400469
assertFalse(
401-
"Should not have stopped looping the main thread yet!", latch.await(1, TimeUnit.SECONDS));
470+
"Should not have stopped looping the main thread yet!",
471+
latch.await(100, TimeUnit.MILLISECONDS));
402472
fakeResource2.forceIdleNow();
403473
assertFalse(
404-
"Should not have stopped looping the main thread yet!", latch.await(1, TimeUnit.SECONDS));
474+
"Should not have stopped looping the main thread yet!",
475+
latch.await(100, TimeUnit.MILLISECONDS));
405476
fakeResource3.forceIdleNow();
406-
assertTrue("App should be idle.", latch.await(5, TimeUnit.SECONDS));
477+
assertTrue("App should be idle.", latch.await(1, TimeUnit.SECONDS));
407478
}
408479

409480
@Test
@@ -430,13 +501,15 @@ public void run() {
430501
}
431502
}));
432503
assertFalse(
433-
"Should not have stopped looping the main thread yet!", latch.await(4, TimeUnit.SECONDS));
504+
"Should not have stopped looping the main thread yet!",
505+
latch.await(100, TimeUnit.MILLISECONDS));
434506
goodResource.forceIdleNow();
435507
assertFalse(
436-
"Should not have stopped looping the main thread yet!", latch.await(12, TimeUnit.SECONDS));
508+
"Should not have stopped looping the main thread yet!",
509+
latch.await(100, TimeUnit.MILLISECONDS));
437510
kindaCrappyResource.forceIdleNow();
438511
assertTrue(
439-
"Should have caught IdlingResourceTimeoutException", latch.await(11, TimeUnit.SECONDS));
512+
"Should have caught IdlingResourceTimeoutException", latch.await(900, TimeUnit.SECONDS));
440513
}
441514

442515
@Test

0 commit comments

Comments
 (0)