From 75125f21311add91af477735dbc7fec7c0fb88f1 Mon Sep 17 00:00:00 2001 From: junyulai Date: Tue, 3 Sep 2019 18:51:04 +0800 Subject: [PATCH] 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 | 6 +++ .../server/connectivity/KeepaliveTracker.java | 50 ++++++++++++------- .../server/ConnectivityServiceTest.java | 2 - 3 files changed, 38 insertions(+), 20 deletions(-) diff --git a/core/java/android/net/SocketKeepalive.java b/core/java/android/net/SocketKeepalive.java index 46aef10258..a7dce18a4f 100644 --- a/core/java/android/net/SocketKeepalive.java +++ b/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; diff --git a/services/core/java/com/android/server/connectivity/KeepaliveTracker.java b/services/core/java/com/android/server/connectivity/KeepaliveTracker.java index 7c8fb5aefd..1f0066a435 100644 --- a/services/core/java/com/android/server/connectivity/KeepaliveTracker.java +++ b/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) { diff --git a/tests/net/java/com/android/server/ConnectivityServiceTest.java b/tests/net/java/com/android/server/ConnectivityServiceTest.java index e6346ea525..d5b6a34850 100644 --- a/tests/net/java/com/android/server/ConnectivityServiceTest.java +++ b/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");