From 7c469179ce2a19da4b8cd787c6b2818d05581833 Mon Sep 17 00:00:00 2001 From: junyulai Date: Wed, 16 Jan 2019 20:23:34 +0800 Subject: [KA02.5] Use binder thread and executor to invoke callback Currently, client side of keepalive event handling rely on a newly created thread, looper, messenger and handler per object. However, by creating oneway AIDL interface with the executor, the callbacks can be invoked on the binder thread with user specified context, which not only greatly simplify the design but also reduce the cost of current thread modeling. Bug: 114151147 Bug: 123969871 Test: 1. atest FrameworksNetTests --generate-new-metric 10 2. atest-deflake.sh Change-Id: I27504074cd28d5b5eb94a7ec0e97ebaaaaa1ae3d --- core/java/android/net/ConnectivityManager.java | 101 ++++++++++--------- core/java/android/net/IConnectivityManager.aidl | 9 +- .../java/android/net/ISocketKeepaliveCallback.aidl | 34 +++++++ core/java/android/net/NattSocketKeepalive.java | 35 ++++--- core/java/android/net/SocketKeepalive.java | 108 +++++++++------------ core/java/android/net/TcpSocketKeepalive.java | 33 ++++--- 6 files changed, 179 insertions(+), 141 deletions(-) create mode 100644 core/java/android/net/ISocketKeepaliveCallback.aidl (limited to 'core/java/android') diff --git a/core/java/android/net/ConnectivityManager.java b/core/java/android/net/ConnectivityManager.java index 2a357ff2bcbb..d08379fab047 100644 --- a/core/java/android/net/ConnectivityManager.java +++ b/core/java/android/net/ConnectivityManager.java @@ -38,7 +38,6 @@ import android.os.Build; import android.os.Build.VERSION_CODES; import android.os.Bundle; import android.os.Handler; -import android.os.HandlerThread; import android.os.IBinder; import android.os.INetworkActivityListener; import android.os.INetworkManagementService; @@ -75,6 +74,9 @@ import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.concurrent.Executor; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.RejectedExecutionException; /** * Class that answers queries about the state of network connectivity. It also @@ -1813,23 +1815,26 @@ public class ConnectivityManager { public static final int MIN_INTERVAL = 10; private final Network mNetwork; - private final PacketKeepaliveCallback mCallback; - private final Looper mLooper; - private final Messenger mMessenger; + private final ISocketKeepaliveCallback mCallback; + private final ExecutorService mExecutor; private volatile Integer mSlot; - void stopLooper() { - mLooper.quit(); - } - @UnsupportedAppUsage public void stop() { try { - mService.stopKeepalive(mNetwork, mSlot); - } catch (RemoteException e) { - Log.e(TAG, "Error stopping packet keepalive: ", e); - stopLooper(); + mExecutor.execute(() -> { + try { + if (mSlot != null) { + mService.stopKeepalive(mNetwork, mSlot); + } + } catch (RemoteException e) { + Log.e(TAG, "Error stopping packet keepalive: ", e); + throw e.rethrowFromSystemServer(); + } + }); + } catch (RejectedExecutionException e) { + // The internal executor has already stopped due to previous event. } } @@ -1837,40 +1842,43 @@ public class ConnectivityManager { Preconditions.checkNotNull(network, "network cannot be null"); Preconditions.checkNotNull(callback, "callback cannot be null"); mNetwork = network; - mCallback = callback; - HandlerThread thread = new HandlerThread(TAG); - thread.start(); - mLooper = thread.getLooper(); - mMessenger = new Messenger(new Handler(mLooper) { + mExecutor = Executors.newSingleThreadExecutor(); + mCallback = new ISocketKeepaliveCallback.Stub() { @Override - public void handleMessage(Message message) { - switch (message.what) { - case NetworkAgent.EVENT_SOCKET_KEEPALIVE: - int error = message.arg2; - try { - if (error == SUCCESS) { - if (mSlot == null) { - mSlot = message.arg1; - mCallback.onStarted(); - } else { - mSlot = null; - stopLooper(); - mCallback.onStopped(); - } - } else { - stopLooper(); - mCallback.onError(error); - } - } catch (Exception e) { - Log.e(TAG, "Exception in keepalive callback(" + error + ")", e); - } - break; - default: - Log.e(TAG, "Unhandled message " + Integer.toHexString(message.what)); - break; - } + public void onStarted(int slot) { + Binder.withCleanCallingIdentity(() -> + mExecutor.execute(() -> { + mSlot = slot; + callback.onStarted(); + })); + } + + @Override + public void onStopped() { + Binder.withCleanCallingIdentity(() -> + mExecutor.execute(() -> { + mSlot = null; + callback.onStopped(); + })); + mExecutor.shutdown(); + } + + @Override + public void onError(int error) { + Binder.withCleanCallingIdentity(() -> + mExecutor.execute(() -> { + mSlot = null; + callback.onError(error); + })); + mExecutor.shutdown(); } - }); + + @Override + public void onDataReceived() { + // PacketKeepalive is only used for Nat-T keepalive and as such does not invoke + // this callback when data is received. + } + }; } } @@ -1887,12 +1895,11 @@ public class ConnectivityManager { InetAddress srcAddr, int srcPort, InetAddress dstAddr) { final PacketKeepalive k = new PacketKeepalive(network, callback); try { - mService.startNattKeepalive(network, intervalSeconds, k.mMessenger, new Binder(), + mService.startNattKeepalive(network, intervalSeconds, k.mCallback, srcAddr.getHostAddress(), srcPort, dstAddr.getHostAddress()); } catch (RemoteException e) { Log.e(TAG, "Error starting packet keepalive: ", e); - k.stopLooper(); - return null; + throw e.rethrowFromSystemServer(); } return k; } diff --git a/core/java/android/net/IConnectivityManager.aidl b/core/java/android/net/IConnectivityManager.aidl index 403b44d6d7d7..540ef89f3a90 100644 --- a/core/java/android/net/IConnectivityManager.aidl +++ b/core/java/android/net/IConnectivityManager.aidl @@ -27,6 +27,7 @@ import android.net.NetworkMisc; import android.net.NetworkQuotaInfo; import android.net.NetworkRequest; import android.net.NetworkState; +import android.net.ISocketKeepaliveCallback; import android.net.ProxyInfo; import android.os.Bundle; import android.os.IBinder; @@ -194,15 +195,15 @@ interface IConnectivityManager void factoryReset(); - void startNattKeepalive(in Network network, int intervalSeconds, in Messenger messenger, - in IBinder binder, String srcAddr, int srcPort, String dstAddr); + void startNattKeepalive(in Network network, int intervalSeconds, + in ISocketKeepaliveCallback cb, String srcAddr, int srcPort, String dstAddr); void startNattKeepaliveWithFd(in Network network, in FileDescriptor fd, int resourceId, - int intervalSeconds, in Messenger messenger, in IBinder binder, String srcAddr, + int intervalSeconds, in ISocketKeepaliveCallback cb, String srcAddr, String dstAddr); void startTcpKeepalive(in Network network, in FileDescriptor fd, int intervalSeconds, - in Messenger messenger, in IBinder binder); + in ISocketKeepaliveCallback cb); void stopKeepalive(in Network network, int slot); diff --git a/core/java/android/net/ISocketKeepaliveCallback.aidl b/core/java/android/net/ISocketKeepaliveCallback.aidl new file mode 100644 index 000000000000..020fbcacbfef --- /dev/null +++ b/core/java/android/net/ISocketKeepaliveCallback.aidl @@ -0,0 +1,34 @@ +/** + * Copyright (c) 2019, The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package android.net; + +/** + * Callback to provide status changes of keepalive offload. + * + * @hide + */ +oneway interface ISocketKeepaliveCallback +{ + /** The keepalive was successfully started. */ + void onStarted(int slot); + /** The keepalive was successfully stopped. */ + void onStopped(); + /** The keepalive was stopped because of an error. */ + void onError(int error); + /** The keepalive on a TCP socket was stopped because the socket received data. */ + void onDataReceived(); +} diff --git a/core/java/android/net/NattSocketKeepalive.java b/core/java/android/net/NattSocketKeepalive.java index 88631aea3a88..84da294f8940 100644 --- a/core/java/android/net/NattSocketKeepalive.java +++ b/core/java/android/net/NattSocketKeepalive.java @@ -17,7 +17,6 @@ package android.net; import android.annotation.NonNull; -import android.os.Binder; import android.os.RemoteException; import android.util.Log; @@ -52,24 +51,30 @@ public final class NattSocketKeepalive extends SocketKeepalive { @Override void startImpl(int intervalSec) { - try { - mService.startNattKeepaliveWithFd(mNetwork, mFd, mResourceId, intervalSec, mMessenger, - new Binder(), mSource.getHostAddress(), mDestination.getHostAddress()); - } catch (RemoteException e) { - Log.e(TAG, "Error starting packet keepalive: ", e); - stopLooper(); - } + mExecutor.execute(() -> { + try { + mService.startNattKeepaliveWithFd(mNetwork, mFd, mResourceId, intervalSec, + mCallback, + mSource.getHostAddress(), mDestination.getHostAddress()); + } catch (RemoteException e) { + Log.e(TAG, "Error starting socket keepalive: ", e); + throw e.rethrowFromSystemServer(); + } + }); } @Override void stopImpl() { - try { - if (mSlot != null) { - mService.stopKeepalive(mNetwork, mSlot); + mExecutor.execute(() -> { + try { + if (mSlot != null) { + mService.stopKeepalive(mNetwork, mSlot); + } + } catch (RemoteException e) { + Log.e(TAG, "Error stopping socket keepalive: ", e); + throw e.rethrowFromSystemServer(); } - } catch (RemoteException e) { - Log.e(TAG, "Error stopping packet keepalive: ", e); - stopLooper(); - } + }); + } } diff --git a/core/java/android/net/SocketKeepalive.java b/core/java/android/net/SocketKeepalive.java index 07728beb9c64..0e768dfc8eb9 100644 --- a/core/java/android/net/SocketKeepalive.java +++ b/core/java/android/net/SocketKeepalive.java @@ -20,13 +20,8 @@ import android.annotation.IntDef; import android.annotation.IntRange; import android.annotation.NonNull; import android.annotation.Nullable; -import android.os.Handler; -import android.os.HandlerThread; -import android.os.Looper; -import android.os.Message; -import android.os.Messenger; -import android.os.Process; -import android.util.Log; +import android.os.Binder; +import android.os.RemoteException; import java.lang.annotation.Retention; import java.lang.annotation.RetentionPolicy; @@ -152,10 +147,9 @@ public abstract class SocketKeepalive implements AutoCloseable { @NonNull final IConnectivityManager mService; @NonNull final Network mNetwork; - @NonNull private final Executor mExecutor; - @NonNull private final SocketKeepalive.Callback mCallback; - @NonNull private final Looper mLooper; - @NonNull final Messenger mMessenger; + @NonNull final Executor mExecutor; + @NonNull final ISocketKeepaliveCallback mCallback; + // TODO: remove slot since mCallback could be used to identify which keepalive to stop. @Nullable Integer mSlot; SocketKeepalive(@NonNull IConnectivityManager service, @NonNull Network network, @@ -163,53 +157,53 @@ public abstract class SocketKeepalive implements AutoCloseable { mService = service; mNetwork = network; mExecutor = executor; - mCallback = callback; - // TODO: 1. Use other thread modeling instead of create one thread for every instance to - // reduce the memory cost. - // 2. support restart. - // 3. Fix race condition which caused by rapidly start and stop. - HandlerThread thread = new HandlerThread(TAG, Process.THREAD_PRIORITY_BACKGROUND - + Process.THREAD_PRIORITY_LESS_FAVORABLE); - thread.start(); - mLooper = thread.getLooper(); - mMessenger = new Messenger(new Handler(mLooper) { + mCallback = new ISocketKeepaliveCallback.Stub() { @Override - public void handleMessage(Message message) { - switch (message.what) { - case NetworkAgent.EVENT_SOCKET_KEEPALIVE: - final int status = message.arg2; - try { - if (status == SUCCESS) { - if (mSlot == null) { - mSlot = message.arg1; - mExecutor.execute(() -> mCallback.onStarted()); - } else { - mSlot = null; - stopLooper(); - mExecutor.execute(() -> mCallback.onStopped()); - } - } else if (status == DATA_RECEIVED) { - stopLooper(); - mExecutor.execute(() -> mCallback.onDataReceived()); - } else { - stopLooper(); - mExecutor.execute(() -> mCallback.onError(status)); - } - } catch (Exception e) { - Log.e(TAG, "Exception in keepalive callback(" + status + ")", e); - } - break; - default: - Log.e(TAG, "Unhandled message " + Integer.toHexString(message.what)); - break; - } + public void onStarted(int slot) { + Binder.withCleanCallingIdentity(() -> + mExecutor.execute(() -> { + mSlot = slot; + callback.onStarted(); + })); } - }); + + @Override + public void onStopped() { + Binder.withCleanCallingIdentity(() -> + executor.execute(() -> { + mSlot = null; + callback.onStopped(); + })); + } + + @Override + public void onError(int error) { + Binder.withCleanCallingIdentity(() -> + executor.execute(() -> { + mSlot = null; + callback.onError(error); + })); + } + + @Override + public void onDataReceived() { + Binder.withCleanCallingIdentity(() -> + executor.execute(() -> { + mSlot = null; + callback.onDataReceived(); + })); + } + }; } /** * Request that keepalive be started with the given {@code intervalSec}. See - * {@link SocketKeepalive}. + * {@link SocketKeepalive}. If the remote binder dies, or the binder call throws an exception + * when invoking start or stop of the {@link SocketKeepalive}, a {@link RemoteException} will be + * thrown into the {@code executor}. This is typically not important to catch because the remote + * party is the system, so if it is not in shape to communicate through binder the system is + * probably going down anyway. If the caller cares regardless, it can use a custom + * {@link Executor} to catch the {@link RemoteException}. * * @param intervalSec The target interval in seconds between keepalive packet transmissions. * The interval should be between 10 seconds and 3600 seconds, otherwise @@ -222,12 +216,6 @@ public abstract class SocketKeepalive implements AutoCloseable { abstract void startImpl(int intervalSec); - /** @hide */ - protected void stopLooper() { - // TODO: remove this after changing thread modeling. - mLooper.quit(); - } - /** * Requests that keepalive be stopped. The application must wait for {@link Callback#onStopped} * before using the object. See {@link SocketKeepalive}. @@ -245,7 +233,6 @@ public abstract class SocketKeepalive implements AutoCloseable { @Override public final void close() { stop(); - stopLooper(); } /** @@ -259,7 +246,8 @@ public abstract class SocketKeepalive implements AutoCloseable { public void onStopped() {} /** An error occurred. */ public void onError(@ErrorCode int error) {} - /** The keepalive on a TCP socket was stopped because the socket received data. */ + /** The keepalive on a TCP socket was stopped because the socket received data. This is + * never called for UDP sockets. */ public void onDataReceived() {} } } diff --git a/core/java/android/net/TcpSocketKeepalive.java b/core/java/android/net/TcpSocketKeepalive.java index f691a0d24344..26cc8ff181b2 100644 --- a/core/java/android/net/TcpSocketKeepalive.java +++ b/core/java/android/net/TcpSocketKeepalive.java @@ -17,7 +17,6 @@ package android.net; import android.annotation.NonNull; -import android.os.Binder; import android.os.RemoteException; import android.util.Log; @@ -56,24 +55,28 @@ final class TcpSocketKeepalive extends SocketKeepalive { */ @Override void startImpl(int intervalSec) { - try { - final FileDescriptor fd = mSocket.getFileDescriptor$(); - mService.startTcpKeepalive(mNetwork, fd, intervalSec, mMessenger, new Binder()); - } catch (RemoteException e) { - Log.e(TAG, "Error starting packet keepalive: ", e); - stopLooper(); - } + mExecutor.execute(() -> { + try { + final FileDescriptor fd = mSocket.getFileDescriptor$(); + mService.startTcpKeepalive(mNetwork, fd, intervalSec, mCallback); + } catch (RemoteException e) { + Log.e(TAG, "Error starting packet keepalive: ", e); + throw e.rethrowFromSystemServer(); + } + }); } @Override void stopImpl() { - try { - if (mSlot != null) { - mService.stopKeepalive(mNetwork, mSlot); + mExecutor.execute(() -> { + try { + if (mSlot != null) { + mService.stopKeepalive(mNetwork, mSlot); + } + } catch (RemoteException e) { + Log.e(TAG, "Error stopping packet keepalive: ", e); + throw e.rethrowFromSystemServer(); } - } catch (RemoteException e) { - Log.e(TAG, "Error stopping packet keepalive: ", e); - stopLooper(); - } + }); } } -- cgit v1.2.3