Notify the keepalive is stopped after the slot has been released

Currently, the callbacks of stopping were fired when stop procedure
is started, because the upper layer apps only care about the reason
of stopping instead of stopping result. Thus, there is no need to
wait for the result comes back. However, this behavior generates
races if apps want to re-start keepalive immediately since the
resources are not released yet.

Fix: 134891441
Fix: 140305589
Test: atest com.android.server.ConnectivityServiceTest#testPacketKeepalives \
      --rerun-until-failure 1000

Change-Id: I987776a9211a50e964c4675b747bc10e203750f1

core/java/android/net/SocketKeepalive.java

@@ -85,6 +85,12 @@ public abstract class SocketKeepalive implements AutoCloseable {
     public static final int ERROR_INVALID_SOCKET = -25;
     /** The target socket is not idle. */
     public static final int ERROR_SOCKET_NOT_IDLE = -26;
+    /**
+     * The stop reason is uninitialized. This should only be internally used as initial state
+     * of stop reason, instead of propagating to application.
+     * @hide
+     */
+    public static final int ERROR_STOP_REASON_UNINITIALIZED = -27;
 
     /** The device does not support this request. */
     public static final int ERROR_UNSUPPORTED = -30;

services/core/java/com/android/server/connectivity/KeepaliveTracker.java

@@ -29,6 +29,7 @@ import static android.net.SocketKeepalive.ERROR_INVALID_INTERVAL;
 import static android.net.SocketKeepalive.ERROR_INVALID_IP_ADDRESS;
 import static android.net.SocketKeepalive.ERROR_INVALID_NETWORK;
 import static android.net.SocketKeepalive.ERROR_INVALID_SOCKET;
+import static android.net.SocketKeepalive.ERROR_STOP_REASON_UNINITIALIZED;
 import static android.net.SocketKeepalive.ERROR_UNSUPPORTED;
 import static android.net.SocketKeepalive.MAX_INTERVAL_SEC;
 import static android.net.SocketKeepalive.MIN_INTERVAL_SEC;
@@ -152,6 +153,7 @@ public class KeepaliveTracker {
         private static final int STARTED = 3;
         private static final int STOPPING = 4;
         private int mStartedState = NOT_STARTED;
+        private int mStopReason = ERROR_STOP_REASON_UNINITIALIZED;
 
         KeepaliveInfo(@NonNull ISocketKeepaliveCallback callback,
                 @NonNull NetworkAgentInfo nai,
@@ -365,6 +367,11 @@ public class KeepaliveTracker {
                     Log.e(TAG, "Cannot stop unowned keepalive " + mSlot + " on " + mNai.network);
                 }
             }
+            // Store the reason of stopping, and report it after the keepalive is fully stopped.
+            if (mStopReason != ERROR_STOP_REASON_UNINITIALIZED) {
+                throw new IllegalStateException("Unexpected stop reason: " + mStopReason);
+            }
+            mStopReason = reason;
             Log.d(TAG, "Stopping keepalive " + mSlot + " on " + mNai.toShortString()
                     + ": " + reason);
             switch (mStartedState) {
@@ -403,24 +410,6 @@ public class KeepaliveTracker {
                     Log.wtf(TAG, "Error closing fd for keepalive " + mSlot + ": " + e);
                 }
             }
-
-            if (reason == SUCCESS) {
-                try {
-                    mCallback.onStopped();
-                } catch (RemoteException e) {
-                    Log.w(TAG, "Discarded onStop callback: " + reason);
-                }
-            } else if (reason == DATA_RECEIVED) {
-                try {
-                    mCallback.onDataReceived();
-                } catch (RemoteException e) {
-                    Log.w(TAG, "Discarded onDataReceived callback: " + reason);
-                }
-            } else {
-                notifyErrorCallback(mCallback, reason);
-            }
-
-            unlinkDeathRecipient();
         }
 
         void onFileDescriptorInitiatedStop(final int socketKeepaliveReason) {
@@ -505,12 +494,37 @@ public class KeepaliveTracker {
             Log.e(TAG, "Attempt to remove nonexistent keepalive " + slot + " on " + networkName);
             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 (networkKeepalives.isEmpty()) {
             mKeepalives.remove(nai);
         }
+
+        // Notify app that the keepalive is stopped.
+        final int reason = ki.mStopReason;
+        if (reason == SUCCESS) {
+            try {
+                ki.mCallback.onStopped();
+            } catch (RemoteException e) {
+                Log.w(TAG, "Discarded onStop callback: " + reason);
+            }
+        } else if (reason == DATA_RECEIVED) {
+            try {
+                ki.mCallback.onDataReceived();
+            } catch (RemoteException e) {
+                Log.w(TAG, "Discarded onDataReceived callback: " + reason);
+            }
+        } else if (reason == ERROR_STOP_REASON_UNINITIALIZED) {
+            throw new IllegalStateException("Unexpected stop reason: " + reason);
+        } else {
+            notifyErrorCallback(ki.mCallback, reason);
+        }
+
+        ki.unlinkDeathRecipient();
     }
 
     public void handleCheckKeepalivesStillValid(NetworkAgentInfo nai) {

tests/net/java/com/android/server/ConnectivityServiceTest.java

@@ -218,7 +218,6 @@ import android.util.Log;
 import android.util.SparseArray;
 
 import androidx.test.InstrumentationRegistry;
-import androidx.test.filters.FlakyTest;
 import androidx.test.filters.SmallTest;
 import androidx.test.runner.AndroidJUnit4;
 
@@ -4028,7 +4027,6 @@ public class ConnectivityServiceTest {
     }
 
     @Test
-    @FlakyTest(bugId = 140305589)
     public void testPacketKeepalives() throws Exception {
         InetAddress myIPv4 = InetAddress.getByName("192.0.2.129");
         InetAddress notMyIPv4 = InetAddress.getByName("192.0.2.35");