From eed8c6bb26bcb2ae4a013f4148dad4c4d6069287 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 46aef10258d97..a7dce18a4ff98 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 7c8fb5aefd1e8..1f0066a435385 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 e6346ea52548a..d5b6a3485018f 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");