From ebb0747af330b12cacb9675e43880e884f514b1d Mon Sep 17 00:00:00 2001 From: Chalard Jean Date: Tue, 23 May 2023 18:23:54 +0900 Subject: [PATCH] Have paused keepalives keep their hardware slot When a keepalive is paused, it should sit on its hardware slot for this network to make sure that it is possible to resume later, even if it means other keepalives can't be started at the same time. Test: update AutomaticOnOffKeepaliveTrackerTest for this Fixes: 268149573 Fixes: 283886067 Change-Id: Ida325bdea198d751483a83ee5d9ec26e39812137 --- .../server/connectivity/KeepaliveTracker.java | 53 ++++++++++++------ .../AutomaticOnOffKeepaliveTrackerTest.java | 54 ++++++++++--------- 2 files changed, 66 insertions(+), 41 deletions(-) diff --git a/service/src/com/android/server/connectivity/KeepaliveTracker.java b/service/src/com/android/server/connectivity/KeepaliveTracker.java index b4f74d5e5c..cbf091b0c8 100644 --- a/service/src/com/android/server/connectivity/KeepaliveTracker.java +++ b/service/src/com/android/server/connectivity/KeepaliveTracker.java @@ -148,8 +148,9 @@ public class KeepaliveTracker { public static final int TYPE_TCP = 2; // Keepalive slot. A small integer that identifies this keepalive among the ones handled - // by this network. - private int mSlot = NO_KEEPALIVE; + // by this network. This is initialized to NO_KEEPALIVE for new keepalives, but to the + // old slot for resumed keepalives. + private int mSlot; // Packet data. private final KeepalivePacketData mPacket; @@ -169,25 +170,30 @@ public class KeepaliveTracker { int interval, int type, @Nullable FileDescriptor fd) throws InvalidSocketException { - this(callback, nai, packet, interval, type, fd, false /* resumed */); + this(callback, nai, packet, Binder.getCallingPid(), Binder.getCallingUid(), interval, + type, fd, NO_KEEPALIVE /* slot */, false /* resumed */); } KeepaliveInfo(@NonNull ISocketKeepaliveCallback callback, @NonNull NetworkAgentInfo nai, @NonNull KeepalivePacketData packet, + int pid, + int uid, int interval, int type, @Nullable FileDescriptor fd, + int slot, boolean resumed) throws InvalidSocketException { mCallback = callback; - mPid = Binder.getCallingPid(); - mUid = Binder.getCallingUid(); + mPid = pid; + mUid = uid; mPrivileged = (PERMISSION_GRANTED == mContext.checkPermission(PERMISSION, mPid, mUid)); mNai = nai; mPacket = packet; mInterval = interval; mType = type; + mSlot = slot; mResumed = resumed; // For SocketKeepalive, a dup of fd is kept in mFd so the source port from which the @@ -468,8 +474,8 @@ public class KeepaliveTracker { * Construct a new KeepaliveInfo from existing KeepaliveInfo with a new fd. */ public KeepaliveInfo withFd(@NonNull FileDescriptor fd) throws InvalidSocketException { - return new KeepaliveInfo(mCallback, mNai, mPacket, mInterval, mType, fd, - true /* resumed */); + return new KeepaliveInfo(mCallback, mNai, mPacket, mPid, mUid, mInterval, mType, + fd, mSlot, true /* resumed */); } } @@ -508,7 +514,9 @@ public class KeepaliveTracker { */ public int handleStartKeepalive(KeepaliveInfo ki) { NetworkAgentInfo nai = ki.getNai(); - int slot = findFirstFreeSlot(nai); + // If this was a paused keepalive, then reuse the same slot that was kept for it. Otherwise, + // use the first free slot for this network agent. + final int slot = NO_KEEPALIVE != ki.mSlot ? ki.mSlot : findFirstFreeSlot(nai); mKeepalives.get(nai).put(slot, ki); return ki.start(slot); } @@ -518,6 +526,8 @@ public class KeepaliveTracker { if (networkKeepalives != null) { final ArrayList kalist = new ArrayList(networkKeepalives.values()); for (KeepaliveInfo ki : kalist) { + // Check if keepalive is already stopped + if (ki.mStopReason == SUCCESS_PAUSED) continue; ki.stop(reason); // Clean up keepalives since the network agent is disconnected and unable to pass // back asynchronous result of stop(). @@ -556,17 +566,22 @@ public class KeepaliveTracker { return; } - // Remove the keepalive from hash table so the slot can be considered available when reusing - // it. - networkKeepalives.remove(slot); - Log.d(TAG, "Remove keepalive " + slot + " on " + networkName + ", " - + networkKeepalives.size() + " remains."); + // If the keepalive was stopped for good, remove it from the hash table so the slot can + // be considered available when reusing it. If it was only a pause, let it sit in the map + // so it sits on the slot. + final int reason = ki.mStopReason; + if (reason != SUCCESS_PAUSED) { + networkKeepalives.remove(slot); + Log.d(TAG, "Remove keepalive " + slot + " on " + networkName + ", " + + networkKeepalives.size() + " remains."); + } else { + Log.d(TAG, "Pause keepalive " + slot + " on " + networkName + ", keep slot reserved"); + } if (networkKeepalives.isEmpty()) { mKeepalives.remove(nai); } // Notify app that the keepalive is stopped. - final int reason = ki.mStopReason; if (reason == SUCCESS) { try { ki.mCallback.onStopped(); @@ -612,7 +627,8 @@ public class KeepaliveTracker { /** * Finalize a paused keepalive. * - * This will send the appropriate callback after checking that this keepalive is indeed paused. + * This will send the appropriate callback after checking that this keepalive is indeed paused, + * and free the slot. * * @param ki the keepalive to finalize * @param reason the reason the keepalive is stopped @@ -630,6 +646,13 @@ public class KeepaliveTracker { } else { notifyErrorCallback(ki.mCallback, reason); } + + final HashMap networkKeepalives = mKeepalives.get(ki.mNai); + if (networkKeepalives == null) { + Log.e(TAG, "Attempt to finalize keepalive on nonexistent network " + ki.mNai); + return; + } + networkKeepalives.remove(ki.mSlot); } /** diff --git a/tests/unit/java/com/android/server/connectivity/AutomaticOnOffKeepaliveTrackerTest.java b/tests/unit/java/com/android/server/connectivity/AutomaticOnOffKeepaliveTrackerTest.java index 82326585fc..0b20227d78 100644 --- a/tests/unit/java/com/android/server/connectivity/AutomaticOnOffKeepaliveTrackerTest.java +++ b/tests/unit/java/com/android/server/connectivity/AutomaticOnOffKeepaliveTrackerTest.java @@ -692,6 +692,10 @@ public class AutomaticOnOffKeepaliveTrackerTest { assertNull(getAutoKiForBinder(testInfo.binder)); verifyNoMoreInteractions(ignoreStubs(testInfo.socketKeepaliveCallback)); + + // Make sure the slot is free + final TestKeepaliveInfo testInfo2 = doStartNattKeepalive(); + checkAndProcessKeepaliveStart(testInfo2.kpd); } @Test @@ -820,36 +824,34 @@ public class AutomaticOnOffKeepaliveTrackerTest { clearInvocations(mNai); // Start the second keepalive while the first is paused. - // TODO: Uncomment the following test after fixing b/283886067. Currently this attempts to - // start the keepalive on TEST_SLOT and this throws in the handler thread. - // final TestKeepaliveInfo testInfo2 = doStartNattKeepalive(); - // // The slot used is TEST_SLOT + 1 since TEST_SLOT is being taken by the paused keepalive. - // checkAndProcessKeepaliveStart(TEST_SLOT + 1, testInfo2.kpd); - // verify(testInfo2.socketKeepaliveCallback).onStarted(); - // assertNotNull(getAutoKiForBinder(testInfo2.binder)); + final TestKeepaliveInfo testInfo2 = doStartNattKeepalive(); + // The slot used is TEST_SLOT + 1 since TEST_SLOT is being taken by the paused keepalive. + checkAndProcessKeepaliveStart(TEST_SLOT + 1, testInfo2.kpd); + verify(testInfo2.socketKeepaliveCallback).onStarted(); + assertNotNull(getAutoKiForBinder(testInfo2.binder)); - // clearInvocations(mNai); - // doResumeKeepalive(autoKi1); - // // Resume on TEST_SLOT. - // checkAndProcessKeepaliveStart(TEST_SLOT, testInfo1.kpd); - // verify(testInfo1.socketKeepaliveCallback).onResumed(); + clearInvocations(mNai); + doResumeKeepalive(autoKi1); + // Resume on TEST_SLOT. + checkAndProcessKeepaliveStart(TEST_SLOT, testInfo1.kpd); + verify(testInfo1.socketKeepaliveCallback).onResumed(); - // clearInvocations(mNai); - // doStopKeepalive(autoKi1); - // checkAndProcessKeepaliveStop(TEST_SLOT); - // verify(testInfo1.socketKeepaliveCallback).onStopped(); - // verify(testInfo2.socketKeepaliveCallback, never()).onStopped(); - // assertNull(getAutoKiForBinder(testInfo1.binder)); + clearInvocations(mNai); + doStopKeepalive(autoKi1); + checkAndProcessKeepaliveStop(TEST_SLOT); + verify(testInfo1.socketKeepaliveCallback).onStopped(); + verify(testInfo2.socketKeepaliveCallback, never()).onStopped(); + assertNull(getAutoKiForBinder(testInfo1.binder)); - // clearInvocations(mNai); - // assertNotNull(getAutoKiForBinder(testInfo2.binder)); - // doStopKeepalive(getAutoKiForBinder(testInfo2.binder)); - // checkAndProcessKeepaliveStop(TEST_SLOT + 1); - // verify(testInfo2.socketKeepaliveCallback).onStopped(); - // assertNull(getAutoKiForBinder(testInfo2.binder)); + clearInvocations(mNai); + assertNotNull(getAutoKiForBinder(testInfo2.binder)); + doStopKeepalive(getAutoKiForBinder(testInfo2.binder)); + checkAndProcessKeepaliveStop(TEST_SLOT + 1); + verify(testInfo2.socketKeepaliveCallback).onStopped(); + assertNull(getAutoKiForBinder(testInfo2.binder)); - // verifyNoMoreInteractions(ignoreStubs(testInfo1.socketKeepaliveCallback)); - // verifyNoMoreInteractions(ignoreStubs(testInfo2.socketKeepaliveCallback)); + verifyNoMoreInteractions(ignoreStubs(testInfo1.socketKeepaliveCallback)); + verifyNoMoreInteractions(ignoreStubs(testInfo2.socketKeepaliveCallback)); } @Test