Merge "Use futures for binding and talking to the ExternalStorageService." into rvc-qpr-dev am: 61e7956648

Original change: https://googleplex-android-review.googlesource.com/c/platform/frameworks/base/+/13713298

MUST ONLY BE SUBMITTED BY AUTOMERGER

Change-Id: Ia4483ed25dddde3dba6f9d64c4dc40a6cdd67c55

services/core/java/com/android/server/storage/StorageUserConnection.java

@@ -23,6 +23,7 @@ import static android.service.storage.ExternalStorageService.FLAG_SESSION_TYPE_F
 import static com.android.server.storage.StorageSessionController.ExternalStorageServiceException;
 
 import android.annotation.MainThread;
+import android.annotation.NonNull;
 import android.annotation.Nullable;
 import android.content.ComponentName;
 import android.content.Context;
@@ -34,13 +35,12 @@ import android.os.IBinder;
 import android.os.ParcelFileDescriptor;
 import android.os.ParcelableException;
 import android.os.RemoteCallback;
+import android.os.RemoteException;
 import android.os.UserHandle;
-import android.os.UserManagerInternal;
 import android.os.storage.StorageManagerInternal;
 import android.os.storage.StorageVolume;
 import android.service.storage.ExternalStorageService;
 import android.service.storage.IExternalStorageService;
-import android.text.TextUtils;
 import android.util.Slog;
 
 import com.android.internal.annotations.GuardedBy;
@@ -48,14 +48,14 @@ import com.android.internal.util.Preconditions;
 import com.android.server.LocalServices;
 
 import java.io.IOException;
+import java.util.ArrayList;
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.Map;
 import java.util.Objects;
 import java.util.Set;
-import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.CompletableFuture;
 import java.util.concurrent.TimeUnit;
-import java.util.concurrent.TimeoutException;
 
 /**
  * Controls the lifecycle of the {@link ActiveConnection} to an {@link ExternalStorageService}
@@ -66,25 +66,20 @@ public final class StorageUserConnection {
 
     private static final int DEFAULT_REMOTE_TIMEOUT_SECONDS = 20;
 
-    private final Object mLock = new Object();
+    private final Object mSessionsLock = new Object();
     private final Context mContext;
     private final int mUserId;
     private final StorageSessionController mSessionController;
     private final ActiveConnection mActiveConnection = new ActiveConnection();
-    private final boolean mIsDemoUser;
     @GuardedBy("mLock") private final Map<String, Session> mSessions = new HashMap<>();
-    @GuardedBy("mLock") @Nullable private HandlerThread mHandlerThread;
+    private final HandlerThread mHandlerThread;
 
     public StorageUserConnection(Context context, int userId, StorageSessionController controller) {
         mContext = Objects.requireNonNull(context);
         mUserId = Preconditions.checkArgumentNonnegative(userId);
         mSessionController = controller;
-        mIsDemoUser = LocalServices.getService(UserManagerInternal.class)
+        mHandlerThread = new HandlerThread("StorageUserConnectionThread-" + mUserId);
-                .getUserInfo(userId).isDemo();
+        mHandlerThread.start();
-        if (mIsDemoUser) {
-            mHandlerThread = new HandlerThread("StorageUserConnectionThread-" + mUserId);
-            mHandlerThread.start();
-        }
     }
 
     /**
@@ -101,13 +96,12 @@ public final class StorageUserConnection {
         Objects.requireNonNull(upperPath);
         Objects.requireNonNull(lowerPath);
 
-        prepareRemote();
+        Session session = new Session(sessionId, upperPath, lowerPath);
-        synchronized (mLock) {
+        synchronized (mSessionsLock) {
             Preconditions.checkArgument(!mSessions.containsKey(sessionId));
-            Session session = new Session(sessionId, upperPath, lowerPath);
             mSessions.put(sessionId, session);
-            mActiveConnection.startSessionLocked(session, pfd);
         }
+        mActiveConnection.startSession(session, pfd);
     }
 
     /**
@@ -121,10 +115,13 @@ public final class StorageUserConnection {
         Objects.requireNonNull(sessionId);
         Objects.requireNonNull(vol);
 
-        prepareRemote();
+        synchronized (mSessionsLock) {
-        synchronized (mLock) {
+            if (!mSessions.containsKey(sessionId)) {
-            mActiveConnection.notifyVolumeStateChangedLocked(sessionId, vol);
+                Slog.i(TAG, "No session found for sessionId: " + sessionId);
+                return;
+            }
         }
+        mActiveConnection.notifyVolumeStateChanged(sessionId, vol);
     }
 
     /**
@@ -135,7 +132,7 @@ public final class StorageUserConnection {
      * with {@link #waitForExit}.
      **/
     public Session removeSession(String sessionId) {
-        synchronized (mLock) {
+        synchronized (mSessionsLock) {
             return mSessions.remove(sessionId);
         }
     }
@@ -153,10 +150,7 @@ public final class StorageUserConnection {
         }
 
         Slog.i(TAG, "Waiting for session end " + session + " ...");
-        prepareRemote();
+        mActiveConnection.endSession(session);
-        synchronized (mLock) {
-            mActiveConnection.endSessionLocked(session);
-        }
     }
 
     /** Restarts all available sessions for a user without blocking.
@@ -164,7 +158,7 @@ public final class StorageUserConnection {
      * Any failures will be ignored.
      **/
     public void resetUserSessions() {
-        synchronized (mLock) {
+        synchronized (mSessionsLock) {
             if (mSessions.isEmpty()) {
                 // Nothing to reset if we have no sessions to restart; we typically
                 // hit this path if the user was consciously shut down.
@@ -179,7 +173,7 @@ public final class StorageUserConnection {
      * Removes all sessions, without waiting.
      */
     public void removeAllSessions() {
-        synchronized (mLock) {
+        synchronized (mSessionsLock) {
             Slog.i(TAG, "Removing  " + mSessions.size() + " sessions for user: " + mUserId + "...");
             mSessions.clear();
         }
@@ -191,68 +185,54 @@ public final class StorageUserConnection {
      */
     public void close() {
         mActiveConnection.close();
-        if (mIsDemoUser) {
+        mHandlerThread.quit();
-            mHandlerThread.quit();
-        }
     }
 
     /** Returns all created sessions. */
     public Set<String> getAllSessionIds() {
-        synchronized (mLock) {
+        synchronized (mSessionsLock) {
             return new HashSet<>(mSessions.keySet());
         }
     }
 
-    private void prepareRemote() throws ExternalStorageServiceException {
+    @FunctionalInterface
-        try {
+    interface AsyncStorageServiceCall {
-            waitForLatch(mActiveConnection.bind(), "remote_prepare_user " + mUserId);
+        void run(@NonNull IExternalStorageService service, RemoteCallback callback) throws
-        } catch (IllegalStateException | TimeoutException e) {
+                RemoteException;
-            throw new ExternalStorageServiceException("Failed to prepare remote", e);
-        }
-    }
-
-    private void waitForLatch(CountDownLatch latch, String reason) throws TimeoutException {
-        try {
-            if (!latch.await(DEFAULT_REMOTE_TIMEOUT_SECONDS, TimeUnit.SECONDS)) {
-                // TODO(b/140025078): Call ActivityManager ANR API?
-                Slog.wtf(TAG, "Failed to bind to the ExternalStorageService for user " + mUserId);
-                throw new TimeoutException("Latch wait for " + reason + " elapsed");
-            }
-        } catch (InterruptedException e) {
-            Thread.currentThread().interrupt();
-            throw new IllegalStateException("Latch wait for " + reason + " interrupted");
-        }
     }
 
     private final class ActiveConnection implements AutoCloseable {
+        private final Object mLock = new Object();
+
         // Lifecycle connection to the external storage service, needed to unbind.
         @GuardedBy("mLock") @Nullable private ServiceConnection mServiceConnection;
-        // True if we are connecting, either bound or binding
+
-        // False && mRemote != null means we are connected
+        // A future that holds the remote interface
-        // False && mRemote == null means we are neither connecting nor connected
+        @GuardedBy("mLock")
-        @GuardedBy("mLock") @Nullable private boolean mIsConnecting;
+        @Nullable private CompletableFuture<IExternalStorageService> mRemoteFuture;
-        // Binder object representing the external storage service.
+
-        // Non-null indicates we are connected
+        // A list of outstanding futures for async calls, for which we are still waiting
-        @GuardedBy("mLock") @Nullable private IExternalStorageService mRemote;
+        // for a callback. Used to unblock waiters if the service dies.
-        // Exception, if any, thrown from #startSessionLocked or #endSessionLocked
+        @GuardedBy("mLock")
-        // Local variables cannot be referenced from a lambda expression :( so we
+        private ArrayList<CompletableFuture<Void>> mOutstandingOps = new ArrayList<>();
-        // save the exception received in the callback here. Since we guard access
-        // (and clear the exception state) with the same lock which we hold during
-        // the entire transaction, there is no risk of race.
-        @GuardedBy("mLock") @Nullable private ParcelableException mLastException;
-        // Not guarded by any lock intentionally and non final because we cannot
-        // reset latches so need to create a new one after one use
-        private CountDownLatch mLatch;
 
         @Override
         public void close() {
             ServiceConnection oldConnection = null;
             synchronized (mLock) {
                 Slog.i(TAG, "Closing connection for user " + mUserId);
-                mIsConnecting = false;
                 oldConnection = mServiceConnection;
                 mServiceConnection = null;
-                mRemote = null;
+                if (mRemoteFuture != null) {
+                    // Let folks who are waiting for the connection know it ain't gonna happen
+                    mRemoteFuture.cancel(true);
+                    mRemoteFuture = null;
+                }
+                // Let folks waiting for callbacks from the remote know it ain't gonna happen
+                for (CompletableFuture<Void> op : mOutstandingOps) {
+                    op.cancel(true);
+                }
+                mOutstandingOps.clear();
             }
 
             if (oldConnection != null) {
@@ -266,37 +246,37 @@ public final class StorageUserConnection {
             }
         }
 
-        public boolean isActiveLocked(Session session) {
+        private void waitForAsync(AsyncStorageServiceCall asyncCall) throws Exception {
-            if (!session.isInitialisedLocked()) {
+            CompletableFuture<IExternalStorageService> serviceFuture = connectIfNeeded();
-                Slog.i(TAG, "Session not initialised " + session);
+            CompletableFuture<Void> opFuture = new CompletableFuture<>();
-                return false;
-            }
 
-            if (mRemote == null) {
+            try {
-                throw new IllegalStateException("Valid session with inactive connection");
+                synchronized (mLock) {
+                    mOutstandingOps.add(opFuture);
+                }
+                serviceFuture.thenCompose(service -> {
+                    try {
+                        asyncCall.run(service,
+                                new RemoteCallback(result -> setResult(result, opFuture)));
+                    } catch (RemoteException e) {
+                        opFuture.completeExceptionally(e);
+                    }
+
+                    return opFuture;
+                }).get(DEFAULT_REMOTE_TIMEOUT_SECONDS, TimeUnit.SECONDS);
+            } finally {
+                synchronized (mLock) {
+                    mOutstandingOps.remove(opFuture);
+                }
             }
-            return true;
         }
 
-        public void startSessionLocked(Session session, ParcelFileDescriptor fd)
+        public void startSession(Session session, ParcelFileDescriptor fd)
                 throws ExternalStorageServiceException {
-            if (!isActiveLocked(session)) {
-                try {
-                    fd.close();
-                } catch (IOException e) {
-                    // ignore
-                }
-                return;
-            }
-
-            CountDownLatch latch = new CountDownLatch(1);
             try {
-                mRemote.startSession(session.sessionId,
+                waitForAsync((service, callback) -> service.startSession(session.sessionId,
                         FLAG_SESSION_TYPE_FUSE | FLAG_SESSION_ATTRIBUTE_INDEXABLE,
-                        fd, session.upperPath, session.lowerPath, new RemoteCallback(result ->
+                        fd, session.upperPath, session.lowerPath, callback));
-                                setResultLocked(latch, result)));
-                waitForLatch(latch, "start_session " + session);
-                maybeThrowExceptionLocked();
             } catch (Exception e) {
                 throw new ExternalStorageServiceException("Failed to start session: " + session, e);
             } finally {
@@ -308,73 +288,49 @@ public final class StorageUserConnection {
             }
         }
 
-        public void endSessionLocked(Session session) throws ExternalStorageServiceException {
+        public void endSession(Session session) throws ExternalStorageServiceException {
-            if (!isActiveLocked(session)) {
-                // Nothing to end, not started yet
-                return;
-            }
-
-            CountDownLatch latch = new CountDownLatch(1);
             try {
-                mRemote.endSession(session.sessionId, new RemoteCallback(result ->
+                waitForAsync((service, callback) ->
-                        setResultLocked(latch, result)));
+                        service.endSession(session.sessionId, callback));
-                waitForLatch(latch, "end_session " + session);
-                maybeThrowExceptionLocked();
             } catch (Exception e) {
                 throw new ExternalStorageServiceException("Failed to end session: " + session, e);
             }
         }
 
-        public void notifyVolumeStateChangedLocked(String sessionId, StorageVolume vol) throws
+
+        public void notifyVolumeStateChanged(String sessionId, StorageVolume vol) throws
                 ExternalStorageServiceException {
-            CountDownLatch latch = new CountDownLatch(1);
             try {
-                mRemote.notifyVolumeStateChanged(sessionId, vol, new RemoteCallback(
+                waitForAsync((service, callback) ->
-                        result -> setResultLocked(latch, result)));
+                        service.notifyVolumeStateChanged(sessionId, vol, callback));
-                waitForLatch(latch, "notify_volume_state_changed " + vol);
-                maybeThrowExceptionLocked();
             } catch (Exception e) {
                 throw new ExternalStorageServiceException("Failed to notify volume state changed "
                         + "for vol : " + vol, e);
             }
         }
 
-        private void setResultLocked(CountDownLatch latch, Bundle result) {
+        private void setResult(Bundle result, CompletableFuture<Void> future) {
-            mLastException = result.getParcelable(EXTRA_ERROR);
+            ParcelableException ex = result.getParcelable(EXTRA_ERROR);
-            latch.countDown();
+            if (ex != null) {
-        }
+                future.completeExceptionally(ex);
-
+            } else {
-        private void maybeThrowExceptionLocked() throws IOException {
+                future.complete(null);
-            if (mLastException != null) {
-                ParcelableException lastException = mLastException;
-                mLastException = null;
-                try {
-                    lastException.maybeRethrow(IOException.class);
-                } catch (IOException e) {
-                    throw e;
-                }
-                throw new RuntimeException(lastException);
             }
         }
 
-        public CountDownLatch bind() throws ExternalStorageServiceException {
+        private CompletableFuture<IExternalStorageService> connectIfNeeded() throws
+                ExternalStorageServiceException {
             ComponentName name = mSessionController.getExternalStorageServiceComponentName();
             if (name == null) {
                 // Not ready to bind
                 throw new ExternalStorageServiceException(
                         "Not ready to bind to the ExternalStorageService for user " + mUserId);
             }
-
             synchronized (mLock) {
-                if (mRemote != null || mIsConnecting) {
+                if (mRemoteFuture != null) {
-                    // Connected or connecting (bound or binding)
+                    return mRemoteFuture;
-                    // Will wait on a latch that will countdown when we connect, unless we are
+                }
-                    // connected and the latch has already countdown, yay!
+                CompletableFuture<IExternalStorageService> future = new CompletableFuture<>();
-                    return mLatch;
-                } // else neither connected nor connecting
-
-                mLatch = new CountDownLatch(1);
-                mIsConnecting = true;
                 mServiceConnection = new ServiceConnection() {
                     @Override
                     public void onServiceConnected(ComponentName name, IBinder service) {
@@ -406,16 +362,9 @@ public final class StorageUserConnection {
 
                     private void handleConnection(IBinder service) {
                         synchronized (mLock) {
-                            if (mIsConnecting) {
+                            future.complete(
-                                mRemote = IExternalStorageService.Stub.asInterface(service);
+                                    IExternalStorageService.Stub.asInterface(service));
-                                mIsConnecting = false;
-                                mLatch.countDown();
-                                // Separate thread so we don't block the main thead
-                                return;
-                            }
                         }
-                        Slog.wtf(TAG, "Connection closed to the ExternalStorageService for user "
-                                + mUserId);
                     }
 
                     private void handleDisconnection() {
@@ -429,32 +378,19 @@ public final class StorageUserConnection {
                 };
 
                 Slog.i(TAG, "Binding to the ExternalStorageService for user " + mUserId);
-                if (mIsDemoUser) {
+                // Schedule on a worker thread, because the system server main thread can be
-                    // Schedule on a worker thread for demo user to avoid deadlock
+                // very busy early in boot.
-                    if (mContext.bindServiceAsUser(new Intent().setComponent(name),
+                if (mContext.bindServiceAsUser(new Intent().setComponent(name),
-                                    mServiceConnection,
+                                mServiceConnection,
-                                    Context.BIND_AUTO_CREATE | Context.BIND_IMPORTANT,
+                                Context.BIND_AUTO_CREATE | Context.BIND_IMPORTANT,
-                                    mHandlerThread.getThreadHandler(),
+                                mHandlerThread.getThreadHandler(),
-                                    UserHandle.of(mUserId))) {
+                                UserHandle.of(mUserId))) {
-                        Slog.i(TAG, "Bound to the ExternalStorageService for user " + mUserId);
+                    Slog.i(TAG, "Bound to the ExternalStorageService for user " + mUserId);
-                        return mLatch;
+                    mRemoteFuture = future;
-                    } else {
+                    return future;
-                        mIsConnecting = false;
-                        throw new ExternalStorageServiceException(
-                                "Failed to bind to the ExternalStorageService for user " + mUserId);
-                    }
                 } else {
-                    if (mContext.bindServiceAsUser(new Intent().setComponent(name),
+                    throw new ExternalStorageServiceException(
-                                    mServiceConnection,
+                            "Failed to bind to the ExternalStorageService for user " + mUserId);
-                                    Context.BIND_AUTO_CREATE | Context.BIND_IMPORTANT,
-                                    UserHandle.of(mUserId))) {
-                        Slog.i(TAG, "Bound to the ExternalStorageService for user " + mUserId);
-                        return mLatch;
-                    } else {
-                        mIsConnecting = false;
-                        throw new ExternalStorageServiceException(
-                                "Failed to bind to the ExternalStorageService for user " + mUserId);
-                    }
                 }
             }
         }
@@ -476,10 +412,5 @@ public final class StorageUserConnection {
             return "[SessionId: " + sessionId + ". UpperPath: " + upperPath + ". LowerPath: "
                     + lowerPath + "]";
         }
-
-        @GuardedBy("mLock")
-        public boolean isInitialisedLocked() {
-            return !TextUtils.isEmpty(upperPath) && !TextUtils.isEmpty(lowerPath);
-        }
     }
 }