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
(cherry picked from https://android-review.googlesource.com/q/commit:ebb0747af330b12cacb9675e43880e884f514b1d)
Merged-In: Ida325bdea198d751483a83ee5d9ec26e39812137
Change-Id: Ida325bdea198d751483a83ee5d9ec26e39812137
diff --git a/service/src/com/android/server/connectivity/KeepaliveTracker.java b/service/src/com/android/server/connectivity/KeepaliveTracker.java
index b4f74d5..cbf091b 100644
--- a/service/src/com/android/server/connectivity/KeepaliveTracker.java
+++ b/service/src/com/android/server/connectivity/KeepaliveTracker.java
@@ -148,8 +148,9 @@
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 @@
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 @@
* 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 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 @@
if (networkKeepalives != null) {
final ArrayList<KeepaliveInfo> 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 @@
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 @@
/**
* 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 @@
} else {
notifyErrorCallback(ki.mCallback, reason);
}
+
+ final HashMap<Integer, KeepaliveInfo> 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 8232658..0b20227 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 @@
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 @@
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
