From f861619bb016e97b7d097a4e72ee2351b0b2a546 Mon Sep 17 00:00:00 2001 From: Michael Wright Date: Wed, 23 Feb 2022 03:48:52 +0000 Subject: [PATCH] Allow for replaceable clock within Looper Handler, Looper, and MessageQueue are all difficult to test since their internal implementations implicitly rely on the system clock. By making their clock replaceable in tests, we can make their behavior more deterministic, allowing for less flaky tests overall. Also, update TestLooper to take advantage of the replaceable clock. Bug: 184781936 Test: atest LooperTest MessageQueueTest HandlerTest Change-Id: I215d2eab99888a162175c37157d7be2be911b96d --- core/java/android/os/Handler.java | 78 +++++++++++++------ core/java/android/os/Looper.java | 26 ++++++- core/java/android/os/MessageQueue.java | 66 ++++++---------- .../java/android/os/test/TestLooper.java | 15 ++-- 4 files changed, 106 insertions(+), 79 deletions(-) diff --git a/core/java/android/os/Handler.java b/core/java/android/os/Handler.java index ceaf337b2122d..aefa5f827f4ad 100644 --- a/core/java/android/os/Handler.java +++ b/core/java/android/os/Handler.java @@ -182,7 +182,7 @@ public class Handler { * * Asynchronous messages represent interrupts or events that do not require global ordering * with respect to synchronous messages. Asynchronous messages are not subject to - * the synchronization barriers introduced by {@link MessageQueue#enqueueSyncBarrier(long)}. + * the synchronization barriers introduced by {@link MessageQueue#postSyncBarrier()}. * * @param async If true, the handler calls {@link Message#setAsynchronous(boolean)} for * each {@link Message} that is sent to it or {@link Runnable} that is posted to it. @@ -203,7 +203,7 @@ public class Handler { * * Asynchronous messages represent interrupts or events that do not require global ordering * with respect to synchronous messages. Asynchronous messages are not subject to - * the synchronization barriers introduced by {@link MessageQueue#enqueueSyncBarrier(long)}. + * the synchronization barriers introduced by {@link MessageQueue#postSyncBarrier()}. * * @param callback The callback interface in which to handle messages, or null. * @param async If true, the handler calls {@link Message#setAsynchronous(boolean)} for @@ -212,25 +212,17 @@ public class Handler { * @hide */ public Handler(@Nullable Callback callback, boolean async) { - if (FIND_POTENTIAL_LEAKS) { - final Class klass = getClass(); - if ((klass.isAnonymousClass() || klass.isMemberClass() || klass.isLocalClass()) && - (klass.getModifiers() & Modifier.STATIC) == 0) { - Log.w(TAG, "The following Handler class should be static or leaks might occur: " + - klass.getCanonicalName()); - } - } + this(getThreadLooper(), callback, async); + } - mLooper = Looper.myLooper(); - if (mLooper == null) { + private static Looper getThreadLooper() { + final Looper looper = Looper.myLooper(); + if (looper == null) { throw new RuntimeException( - "Can't create handler inside thread " + Thread.currentThread() - + " that has not called Looper.prepare()"); + "Can't create handler inside thread " + Thread.currentThread() + + " that has not called Looper.prepare()"); } - mQueue = mLooper.mQueue; - mCallback = callback; - mAsynchronous = async; - mIsShared = false; + return looper; } /** @@ -257,14 +249,44 @@ public class Handler { this(looper, callback, async, /* shared= */ false); } - /** @hide */ + /** + * Use the provided {@link Looper} instead of the default one and take a callback + * interface in which to handle messages. Also set whether the handler + * should be asynchronous. + * + * Handlers are synchronous by default unless this constructor is used to make + * one that is strictly asynchronous. + * + * Asynchronous messages represent interrupts or events that do not require global ordering + * with respect to synchronous messages. Asynchronous messages are not subject to + * the synchronization barriers introduced by conditions such as display vsync. + * + * @param looper The looper, must not be null. + * @param callback The callback interface in which to handle messages, or null. + * @param async If true, the handler calls {@link Message#setAsynchronous(boolean)} for + * each {@link Message} that is sent to it or {@link Runnable} that is posted to + * it. + * @param shared Whether this Handler might be used by more than one client. A shared Handler + * applies some extra policy, such as disallowing the removal of all messages, + * in order to avoid one client affecting another's messages. + * @hide + */ public Handler(@NonNull Looper looper, @Nullable Callback callback, boolean async, boolean shared) { + if (FIND_POTENTIAL_LEAKS) { + final Class klass = getClass(); + if ((klass.isAnonymousClass() || klass.isMemberClass() || klass.isLocalClass()) + && (klass.getModifiers() & Modifier.STATIC) == 0) { + Log.w(TAG, "The following Handler class should be static or leaks might occur: " + + klass.getCanonicalName()); + } + } mLooper = looper; mQueue = looper.mQueue; mCallback = callback; mAsynchronous = async; mIsShared = shared; + mClock = looper.getClock(); } /** @@ -702,7 +724,14 @@ public class Handler { if (delayMillis < 0) { delayMillis = 0; } - return sendMessageAtTime(msg, SystemClock.uptimeMillis() + delayMillis); + // mClock should theoretically never be null, but some tests create a mock handler that + // instantiates an instance where all members are null. Ideally we'd fix these tests to + // not rely on this but there are quite a lot at this point, so it's easier to just keep + // the existing behavior. + if (mClock == null) { + return false; + } + return sendMessageAtTime(msg, mClock.uptimeMillis() + delayMillis); } /** @@ -895,7 +924,7 @@ public class Handler { } public final void dump(@NonNull Printer pw, @NonNull String prefix) { - pw.println(prefix + this + " @ " + SystemClock.uptimeMillis()); + pw.println(prefix + this + " @ " + mClock.uptimeMillis()); if (mLooper == null) { pw.println(prefix + "looper uninitialized"); } else { @@ -907,7 +936,7 @@ public class Handler { * @hide */ public final void dumpMine(@NonNull Printer pw, @NonNull String prefix) { - pw.println(prefix + this + " @ " + SystemClock.uptimeMillis()); + pw.println(prefix + this + " @ " + mClock.uptimeMillis()); if (mLooper == null) { pw.println(prefix + "looper uninitialized"); } else { @@ -964,6 +993,7 @@ public class Handler { @UnsupportedAppUsage final Callback mCallback; final boolean mAsynchronous; + final MessageQueue.Clock mClock; @UnsupportedAppUsage IMessenger mMessenger; @@ -997,9 +1027,9 @@ public class Handler { synchronized (this) { if (timeout > 0) { - final long expirationTime = SystemClock.uptimeMillis() + timeout; + final long expirationTime = handler.mClock.uptimeMillis() + timeout; while (!mDone) { - long delay = expirationTime - SystemClock.uptimeMillis(); + long delay = expirationTime - handler.mClock.uptimeMillis(); if (delay <= 0) { return false; // timeout } diff --git a/core/java/android/os/Looper.java b/core/java/android/os/Looper.java index a529ac6569bd5..f0fda15f6e90f 100644 --- a/core/java/android/os/Looper.java +++ b/core/java/android/os/Looper.java @@ -24,6 +24,8 @@ import android.util.Printer; import android.util.Slog; import android.util.proto.ProtoOutputStream; +import java.util.Objects; + /** * Class used to run a message loop for a thread. Threads by default do * not have a message loop associated with them; to create one, call @@ -69,7 +71,7 @@ public final class Looper { // sThreadLocal.get() will return null unless you've called prepare(). @UnsupportedAppUsage - static final ThreadLocal sThreadLocal = new ThreadLocal(); + static final ThreadLocal sThreadLocal = new ThreadLocal<>(); @UnsupportedAppUsage private static Looper sMainLooper; // guarded by Looper.class private static Observer sObserver; @@ -77,6 +79,7 @@ public final class Looper { @UnsupportedAppUsage final MessageQueue mQueue; final Thread mThread; + final MessageQueue.Clock mClock; private boolean mInLoop; @UnsupportedAppUsage @@ -191,7 +194,7 @@ public final class Looper { Trace.traceBegin(traceTag, msg.target.getTraceName(msg)); } - final long dispatchStart = needStartTime ? SystemClock.uptimeMillis() : 0; + final long dispatchStart = needStartTime ? me.mClock.uptimeMillis() : 0; final long dispatchEnd; Object token = null; if (observer != null) { @@ -203,7 +206,7 @@ public final class Looper { if (observer != null) { observer.messageDispatched(token, msg); } - dispatchEnd = needEndTime ? SystemClock.uptimeMillis() : 0; + dispatchEnd = needEndTime ? me.mClock.uptimeMillis() : 0; } catch (Exception exception) { if (observer != null) { observer.dispatchingThrewException(token, msg, exception); @@ -325,8 +328,13 @@ public final class Looper { } private Looper(boolean quitAllowed) { - mQueue = new MessageQueue(quitAllowed); + this(quitAllowed, SystemClock::uptimeMillis); + } + + private Looper(boolean quitAllowed, @NonNull MessageQueue.Clock clock) { + mQueue = new MessageQueue(quitAllowed, Objects.requireNonNull(clock)); mThread = Thread.currentThread(); + mClock = clock; } /** @@ -418,6 +426,16 @@ public final class Looper { return mQueue; } + /** + * Gets the looper's clock. + * + * @return The looper's clock + * @hide + */ + public @NonNull MessageQueue.Clock getClock() { + return mClock; + } + /** * Dumps the state of the looper for debugging purposes. * diff --git a/core/java/android/os/MessageQueue.java b/core/java/android/os/MessageQueue.java index 87c4f331e93f8..a8ed5ba994899 100644 --- a/core/java/android/os/MessageQueue.java +++ b/core/java/android/os/MessageQueue.java @@ -19,6 +19,7 @@ package android.os; import android.annotation.IntDef; import android.annotation.NonNull; import android.annotation.TestApi; +import android.annotation.UptimeMillisLong; import android.compat.annotation.UnsupportedAppUsage; import android.util.Log; import android.util.Printer; @@ -29,6 +30,7 @@ import java.io.FileDescriptor; import java.lang.annotation.Retention; import java.lang.annotation.RetentionPolicy; import java.util.ArrayList; +import java.util.Objects; /** * Low-level class holding the list of messages to be dispatched by a @@ -54,6 +56,7 @@ public final class MessageQueue { Message mMessages; @UnsupportedAppUsage private final ArrayList mIdleHandlers = new ArrayList(); + private final Clock mClock; private SparseArray mFileDescriptorRecords; private IdleHandler[] mPendingIdleHandlers; private boolean mQuitting; @@ -74,9 +77,10 @@ public final class MessageQueue { private native static boolean nativeIsPolling(long ptr); private native static void nativeSetFileDescriptorEvents(long ptr, int fd, int events); - MessageQueue(boolean quitAllowed) { + MessageQueue(boolean quitAllowed, @NonNull Clock clock) { mQuitAllowed = quitAllowed; mPtr = nativeInit(); + mClock = Objects.requireNonNull(clock); } @Override @@ -106,7 +110,7 @@ public final class MessageQueue { */ public boolean isIdle() { synchronized (this) { - final long now = SystemClock.uptimeMillis(); + final long now = mClock.uptimeMillis(); return mMessages == null || now < mMessages.when; } } @@ -336,7 +340,7 @@ public final class MessageQueue { synchronized (this) { // Try to retrieve the next message. Return if found. - final long now = SystemClock.uptimeMillis(); + final long now = mClock.uptimeMillis(); Message prevMsg = null; Message msg = mMessages; if (msg != null && msg.target == null) { @@ -470,7 +474,7 @@ public final class MessageQueue { @UnsupportedAppUsage @TestApi public int postSyncBarrier() { - return postSyncBarrier(SystemClock.uptimeMillis()); + return postSyncBarrier(mClock.uptimeMillis()); } private int postSyncBarrier(long when) { @@ -772,41 +776,6 @@ public final class MessageQueue { } } - void removeEqualMessages(Handler h, Runnable r, Object object) { - if (h == null || r == null) { - return; - } - - synchronized (this) { - Message p = mMessages; - - // Remove all messages at front. - while (p != null && p.target == h && p.callback == r - && (object == null || object.equals(p.obj))) { - Message n = p.next; - mMessages = n; - p.recycleUnchecked(); - p = n; - } - - // Remove all messages after front. - while (p != null) { - Message n = p.next; - if (n != null) { - if (n.target == h && n.callback == r - && (object == null || object.equals(n.obj))) { - Message nn = n.next; - n.recycleUnchecked(); - p.next = nn; - continue; - } - } - p = n; - } - } - } - - void removeCallbacksAndMessages(Handler h, Object object) { if (h == null) { return; @@ -884,7 +853,7 @@ public final class MessageQueue { } private void removeAllFutureMessagesLocked() { - final long now = SystemClock.uptimeMillis(); + final long now = mClock.uptimeMillis(); Message p = mMessages; if (p != null) { if (p.when > now) { @@ -913,7 +882,7 @@ public final class MessageQueue { void dump(Printer pw, String prefix, Handler h) { synchronized (this) { - long now = SystemClock.uptimeMillis(); + long now = mClock.uptimeMillis(); int n = 0; for (Message msg = mMessages; msg != null; msg = msg.next) { if (h == null || h == msg.target) { @@ -942,7 +911,7 @@ public final class MessageQueue { * Callback interface for discovering when a thread is going to block * waiting for more messages. */ - public static interface IdleHandler { + public interface IdleHandler { /** * Called when the message queue has run out of messages and will now * wait for more. Return true to keep your idle handler active, false @@ -1041,4 +1010,17 @@ public final class MessageQueue { mListener = listener; } } + + /** + * Time supplier for MessageQueue and the things that interact with it (e.g. {@link Looper}). + * + * Intentionally replaceable for testing. + * + * @hide + */ + public interface Clock { + /** @see SystemClock#uptimeMillis */ + @UptimeMillisLong + long uptimeMillis(); + } } diff --git a/tests/utils/testutils/java/android/os/test/TestLooper.java b/tests/utils/testutils/java/android/os/test/TestLooper.java index a826646f69f3f..9ad6d53a32462 100644 --- a/tests/utils/testutils/java/android/os/test/TestLooper.java +++ b/tests/utils/testutils/java/android/os/test/TestLooper.java @@ -48,13 +48,14 @@ public class TestLooper { private static final Method MESSAGE_MARK_IN_USE_METHOD; private static final String TAG = "TestLooper"; - private final Clock mClock; + private final MessageQueue.Clock mClock; private AutoDispatchThread mAutoDispatchThread; static { try { - LOOPER_CONSTRUCTOR = Looper.class.getDeclaredConstructor(Boolean.TYPE); + LOOPER_CONSTRUCTOR = Looper.class.getDeclaredConstructor(Boolean.TYPE, + MessageQueue.Clock.class); LOOPER_CONSTRUCTOR.setAccessible(true); THREAD_LOCAL_LOOPER_FIELD = Looper.class.getDeclaredField("sThreadLocal"); THREAD_LOCAL_LOOPER_FIELD.setAccessible(true); @@ -83,15 +84,15 @@ public class TestLooper { * thread. * * Messages are dispatched when their {@link Message#when} is before or at {@link - * Clock#uptimeMillis()}. + * MessageQueue.Clock#uptimeMillis()}. * Use a custom clock with care. When using an offsettable clock like {@link * com.android.server.testutils.OffsettableClock} be sure not to double offset messages by * offsetting the clock and calling {@link #moveTimeForward(long)}. Instead, offset the clock * and call {@link #dispatchAll()}. */ - public TestLooper(Clock clock) { + public TestLooper(MessageQueue.Clock clock) { try { - mLooper = LOOPER_CONSTRUCTOR.newInstance(false); + mLooper = LOOPER_CONSTRUCTOR.newInstance(false, clock); ThreadLocal threadLocalLooper = (ThreadLocal) THREAD_LOCAL_LOOPER_FIELD .get(null); @@ -224,10 +225,6 @@ public class TestLooper { return count; } - public interface Clock { - long uptimeMillis(); - } - /** * Thread used to dispatch messages when the main thread is blocked waiting for a response. */