Use futures for binding and talking to the ExternalStorageService.
The existing CountdownLatch logic had some corner cases where it would
continue waiting for the latch to count down, even when it was already
clear that the operation couldn't succeed (eg, a bind failed).
To fix this, as well as try and simplify some of this code, switch to
using futures, which have a nicely defined way of cancelling outstanding
operations.
Additionally, switch to using a dedicated handler thread for receiving
bind events for all users, not just demo users. This avoids having to
wait for a long time when the system_server main thread is busy.
Bug: 170284679
Test: atest AdoptableHostTest
atest CtsScopedStorageHostTest
atest android.multiuser.UserLifecycleTests#managedProfileUnlock
atest android.multiuser.UserLifecycleTests#managedProfileUnlockAndLaunchApp_stopped
Change-Id: I1b7fde2f6ed1074b9c814d56808ab1614f25aead
diff --git a/services/core/java/com/android/server/storage/StorageUserConnection.java b/services/core/java/com/android/server/storage/StorageUserConnection.java
index b79fc8d..af26289 100644
--- a/services/core/java/com/android/server/storage/StorageUserConnection.java
+++ b/services/core/java/com/android/server/storage/StorageUserConnection.java
@@ -23,6 +23,7 @@
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,28 +35,27 @@
import android.os.ParcelFileDescriptor;
import android.os.ParcelableException;
import android.os.RemoteCallback;
+import android.os.RemoteException;
import android.os.UserHandle;
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;
import com.android.internal.util.Preconditions;
import com.android.server.LocalServices;
-import com.android.server.pm.UserManagerInternal;
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 @@
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)
- .getUserInfo(userId).isDemo();
- if (mIsDemoUser) {
- mHandlerThread = new HandlerThread("StorageUserConnectionThread-" + mUserId);
- mHandlerThread.start();
- }
+ mHandlerThread = new HandlerThread("StorageUserConnectionThread-" + mUserId);
+ mHandlerThread.start();
}
/**
@@ -101,13 +96,12 @@
Objects.requireNonNull(upperPath);
Objects.requireNonNull(lowerPath);
- prepareRemote();
- synchronized (mLock) {
+ Session session = new Session(sessionId, upperPath, lowerPath);
+ 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 @@
Objects.requireNonNull(sessionId);
Objects.requireNonNull(vol);
- prepareRemote();
- synchronized (mLock) {
- mActiveConnection.notifyVolumeStateChangedLocked(sessionId, vol);
+ synchronized (mSessionsLock) {
+ if (!mSessions.containsKey(sessionId)) {
+ Slog.i(TAG, "No session found for sessionId: " + sessionId);
+ return;
+ }
}
+ mActiveConnection.notifyVolumeStateChanged(sessionId, vol);
}
/**
@@ -135,7 +132,7 @@
* with {@link #waitForExit}.
**/
public Session removeSession(String sessionId) {
- synchronized (mLock) {
+ synchronized (mSessionsLock) {
return mSessions.remove(sessionId);
}
}
@@ -153,10 +150,7 @@
}
Slog.i(TAG, "Waiting for session end " + session + " ...");
- prepareRemote();
- synchronized (mLock) {
- mActiveConnection.endSessionLocked(session);
- }
+ mActiveConnection.endSession(session);
}
/** Restarts all available sessions for a user without blocking.
@@ -164,7 +158,7 @@
* 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 @@
* 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 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 {
- try {
- waitForLatch(mActiveConnection.bind(), "remote_prepare_user " + mUserId);
- } catch (IllegalStateException | TimeoutException e) {
- 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");
- }
+ @FunctionalInterface
+ interface AsyncStorageServiceCall {
+ void run(@NonNull IExternalStorageService service, RemoteCallback callback) throws
+ RemoteException;
}
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
- // False && mRemote == null means we are neither connecting nor connected
- @GuardedBy("mLock") @Nullable private boolean mIsConnecting;
- // Binder object representing the external storage service.
- // Non-null indicates we are connected
- @GuardedBy("mLock") @Nullable private IExternalStorageService mRemote;
- // Exception, if any, thrown from #startSessionLocked or #endSessionLocked
- // Local variables cannot be referenced from a lambda expression :( so we
- // 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;
+
+ // A future that holds the remote interface
+ @GuardedBy("mLock")
+ @Nullable private CompletableFuture<IExternalStorageService> mRemoteFuture;
+
+ // A list of outstanding futures for async calls, for which we are still waiting
+ // for a callback. Used to unblock waiters if the service dies.
+ @GuardedBy("mLock")
+ private ArrayList<CompletableFuture<Void>> mOutstandingOps = new ArrayList<>();
@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 boolean isActiveLocked(Session session) {
- if (!session.isInitialisedLocked()) {
- Slog.i(TAG, "Session not initialised " + session);
- return false;
- }
+ private void waitForAsync(AsyncStorageServiceCall asyncCall) throws Exception {
+ CompletableFuture<IExternalStorageService> serviceFuture = connectIfNeeded();
+ CompletableFuture<Void> opFuture = new CompletableFuture<>();
- if (mRemote == null) {
- throw new IllegalStateException("Valid session with inactive connection");
+ try {
+ 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 ->
- setResultLocked(latch, result)));
- waitForLatch(latch, "start_session " + session);
- maybeThrowExceptionLocked();
+ fd, session.upperPath, session.lowerPath, callback));
} catch (Exception e) {
throw new ExternalStorageServiceException("Failed to start session: " + session, e);
} finally {
@@ -308,73 +288,49 @@
}
}
- public void endSessionLocked(Session session) throws ExternalStorageServiceException {
- if (!isActiveLocked(session)) {
- // Nothing to end, not started yet
- return;
- }
-
- CountDownLatch latch = new CountDownLatch(1);
+ public void endSession(Session session) throws ExternalStorageServiceException {
try {
- mRemote.endSession(session.sessionId, new RemoteCallback(result ->
- setResultLocked(latch, result)));
- waitForLatch(latch, "end_session " + session);
- maybeThrowExceptionLocked();
+ waitForAsync((service, callback) ->
+ service.endSession(session.sessionId, callback));
} 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(
- result -> setResultLocked(latch, result)));
- waitForLatch(latch, "notify_volume_state_changed " + vol);
- maybeThrowExceptionLocked();
+ waitForAsync((service, callback) ->
+ service.notifyVolumeStateChanged(sessionId, vol, callback));
} catch (Exception e) {
throw new ExternalStorageServiceException("Failed to notify volume state changed "
+ "for vol : " + vol, e);
}
}
- private void setResultLocked(CountDownLatch latch, Bundle result) {
- mLastException = result.getParcelable(EXTRA_ERROR);
- latch.countDown();
- }
-
- private void maybeThrowExceptionLocked() throws IOException {
- if (mLastException != null) {
- ParcelableException lastException = mLastException;
- mLastException = null;
- try {
- lastException.maybeRethrow(IOException.class);
- } catch (IOException e) {
- throw e;
- }
- throw new RuntimeException(lastException);
+ private void setResult(Bundle result, CompletableFuture<Void> future) {
+ ParcelableException ex = result.getParcelable(EXTRA_ERROR);
+ if (ex != null) {
+ future.completeExceptionally(ex);
+ } else {
+ future.complete(null);
}
}
- 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) {
- // Connected or connecting (bound or binding)
- // Will wait on a latch that will countdown when we connect, unless we are
- // connected and the latch has already countdown, yay!
- return mLatch;
- } // else neither connected nor connecting
-
- mLatch = new CountDownLatch(1);
- mIsConnecting = true;
+ if (mRemoteFuture != null) {
+ return mRemoteFuture;
+ }
+ CompletableFuture<IExternalStorageService> future = new CompletableFuture<>();
mServiceConnection = new ServiceConnection() {
@Override
public void onServiceConnected(ComponentName name, IBinder service) {
@@ -406,16 +362,9 @@
private void handleConnection(IBinder service) {
synchronized (mLock) {
- if (mIsConnecting) {
- mRemote = IExternalStorageService.Stub.asInterface(service);
- mIsConnecting = false;
- mLatch.countDown();
- // Separate thread so we don't block the main thead
- return;
- }
+ future.complete(
+ IExternalStorageService.Stub.asInterface(service));
}
- Slog.wtf(TAG, "Connection closed to the ExternalStorageService for user "
- + mUserId);
}
private void handleDisconnection() {
@@ -429,32 +378,19 @@
};
Slog.i(TAG, "Binding to the ExternalStorageService for user " + mUserId);
- if (mIsDemoUser) {
- // Schedule on a worker thread for demo user to avoid deadlock
- if (mContext.bindServiceAsUser(new Intent().setComponent(name),
- mServiceConnection,
- Context.BIND_AUTO_CREATE | Context.BIND_IMPORTANT,
- mHandlerThread.getThreadHandler(),
- 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);
- }
+ // Schedule on a worker thread, because the system server main thread can be
+ // very busy early in boot.
+ if (mContext.bindServiceAsUser(new Intent().setComponent(name),
+ mServiceConnection,
+ Context.BIND_AUTO_CREATE | Context.BIND_IMPORTANT,
+ mHandlerThread.getThreadHandler(),
+ UserHandle.of(mUserId))) {
+ Slog.i(TAG, "Bound to the ExternalStorageService for user " + mUserId);
+ mRemoteFuture = future;
+ return future;
} else {
- if (mContext.bindServiceAsUser(new Intent().setComponent(name),
- mServiceConnection,
- 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);
- }
+ throw new ExternalStorageServiceException(
+ "Failed to bind to the ExternalStorageService for user " + mUserId);
}
}
}
@@ -476,10 +412,5 @@
return "[SessionId: " + sessionId + ". UpperPath: " + upperPath + ". LowerPath: "
+ lowerPath + "]";
}
-
- @GuardedBy("mLock")
- public boolean isInitialisedLocked() {
- return !TextUtils.isEmpty(upperPath) && !TextUtils.isEmpty(lowerPath);
- }
}
}