Android/frameworks_base
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Fix a race condition between calculateWaitTime and actually waiting.

Browse Source 
When the race was lost with a completion callback, the operation
being waited upon may have been de-queued by the completion callback
for immediate execution, but the thread was still proceeding with
the wait.

This caused flaky tests as the extra delay caused the thread to
take longer to complete, and the newly extended callback delay
happened to coincide with the test's wait timeout. Consequently,
the test's wait timeout is shortened slightly to make it clearer
when the issue is caused by "falling into" the OffStep callback
delay, which isn't intended to be a normal behavior.

Test: manual atest
Bug: 194408223
Change-Id: I97ec74bbc26171a4384ed9ddd7e2130fb8004b9b
This commit is contained in:
Simon Bowden
2021-08-05 12:46:45 +00:00
parent 5528d08a4f
commit 7ab12ba890
2 changed files with 18 additions and 14 deletions
Download Patch File Download Diff File Expand all files Collapse all files

30
services/core/java/com/android/server/vibrator/VibrationThread.java
View File

@@ -228,17 +228,20 @@ final class VibrationThread extends Thread implements IBinder.DeathRecipient {
Vibration.Status status = null;
while (!mStepQueue.isEmpty()) {
long waitTime = mStepQueue.calculateWaitTime();
if (waitTime <= 0) {
mStepQueue.consumeNext();
} else {
synchronized (mLock) {
long waitTime;
synchronized (mLock) {
waitTime = mStepQueue.calculateWaitTime();
if (waitTime > 0) {
try {
mLock.wait(waitTime);
} catch (InterruptedException e) {
}
}
}
// If we waited, the queue may have changed, so let the loop run again.
if (waitTime <= 0) {
mStepQueue.consumeNext();
}
Vibration.Status currentStatus = mStop ? Vibration.Status.CANCELLED
: mStepQueue.calculateVibrationStatus(sequentialEffectSize);
if (status == null && currentStatus != Vibration.Status.RUNNING) {
@@ -387,15 +390,13 @@ final class VibrationThread extends Thread implements IBinder.DeathRecipient {
}
/** Returns the time in millis to wait before calling {@link #consumeNext()}. */
@GuardedBy("mLock")
public long calculateWaitTime() {
Step nextStep;
synchronized (mLock) {
if (!mPendingOnVibratorCompleteSteps.isEmpty()) {
// Steps anticipated by vibrator complete callback should be played right away.
return 0;
}
nextStep = mNextSteps.peek();
if (!mPendingOnVibratorCompleteSteps.isEmpty()) {
// Steps anticipated by vibrator complete callback should be played right away.
return 0;
}
Step nextStep = mNextSteps.peek();
return nextStep == null ? 0 : nextStep.calculateWaitTime();
}
@@ -603,7 +604,10 @@ final class VibrationThread extends Thread implements IBinder.DeathRecipient {
return false;
}
/** Returns the time in millis to wait before playing this step. */
/**
* Returns the time in millis to wait before playing this step. This is performed
* while holding the queue lock, so should not rely on potentially slow operations.
*/
public long calculateWaitTime() {
if (startTime == Long.MAX_VALUE) {
// This step don't have a predefined start time, it's just marked to be executed

2
services/tests/servicestests/src/com/android/server/vibrator/VibrationThreadTest.java
View File

@@ -83,7 +83,7 @@ import java.util.stream.Collectors;
@Presubmit
public class VibrationThreadTest {
private static final int TEST_TIMEOUT_MILLIS = 1_000;
private static final int TEST_TIMEOUT_MILLIS = 900;
private static final int UID = Process.ROOT_UID;
private static final int VIBRATOR_ID = 1;
private static final String PACKAGE_NAME = "package";