Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 9a18f8da authored by Lucas Silva's avatar Lucas Silva
Browse files

Move bindService/unbindService calls to background thread 

This ensures that bindService/unbindService always executes inside a
background executor in PersistentConnectionManager.

Bug: 330902851
Test: atest ObservableServiceConnectionTest
Flag: NONE
Change-Id: I27d0e1601951e08cd7cd621ae3e118cd9b4a7ca2
parent 0499cf71

packages/SystemUI/src/com/android/systemui/util/service/ObservableServiceConnection.java

+24 −19
Original line number Diff line number Diff line
@@ -26,8 +26,9 @@ import android.util.IndentingPrintWriter; 
import android.util.Log;



import androidx.annotation.NonNull;

import androidx.annotation.WorkerThread;



import com.android.systemui.dagger.qualifiers.Main;

import com.android.systemui.dagger.qualifiers.Background;

import com.android.systemui.settings.UserTracker;

import com.android.systemui.util.DumpUtilsKt;

import com.android.systemui.util.annotations.WeaklyReferencedCallback;
@@ -118,7 +119,7 @@ public class ObservableServiceConnection<T> implements ServiceConnection { 
    private final Intent mServiceIntent;

    private final UserTracker mUserTracker;

    private final int mFlags;

    private final Executor mExecutor;

    private final Executor mBgExecutor;

    private final ServiceTransformer<T> mTransformer;

    private final ArrayList<WeakReference<Callback<T>>> mCallbacks;

    private Optional<Integer> mLastDisconnectReason;
@@ -130,30 +131,34 @@ public class ObservableServiceConnection<T> implements ServiceConnection { 
     * Default constructor for {@link ObservableServiceConnection}.

     * @param context       The context from which the service will be bound with.

     * @param serviceIntent The intent to  bind service with.

     * @param executor      The executor for connection callbacks to be delivered on

     * @param bgExecutor    The executor for connection callbacks to be delivered on

     * @param transformer   A {@link ServiceTransformer} for transforming the resulting service

     *                      into a desired type.

     */

    @Inject

    public ObservableServiceConnection(Context context, Intent serviceIntent,

            UserTracker userTracker,

            @Main Executor executor,

            @Background Executor bgExecutor,

            ServiceTransformer<T> transformer) {

        mContext = context;

        mServiceIntent = serviceIntent;

        mUserTracker = userTracker;

        mFlags = Context.BIND_AUTO_CREATE;

        mExecutor = executor;

        mBgExecutor = bgExecutor;

        mTransformer = transformer;

        mCallbacks = new ArrayList<>();

        mLastDisconnectReason = Optional.empty();

    }



    /**

     * Initiate binding to the service.

     * @return {@code true} if initiating binding succeed, {@code false} otherwise.

     * Initiate binding to the service in the background.

     */

    public boolean bind() {

    public void bind() {

        mBgExecutor.execute(this::bindInternal);

    }



    @WorkerThread

    private void bindInternal() {

        boolean bindResult = false;

        try {

            bindResult = mContext.bindServiceAsUser(mServiceIntent, this, mFlags,
@@ -166,18 +171,17 @@ public class ObservableServiceConnection<T> implements ServiceConnection { 
        if (DEBUG) {

            Log.d(TAG, "bind. bound:" + bindResult);

        }

        return bindResult;

    }



    /**

     * Disconnect from the service if bound.

     */

    public void unbind() {

        onDisconnected(DISCONNECT_REASON_UNBIND);

        mBgExecutor.execute(() -> onDisconnected(DISCONNECT_REASON_UNBIND));

    }



    /**

     * Adds a callback for receiving connection updates.

     * Adds a callback for receiving connection updates. The callback is executed in the background.

     * @param callback The {@link Callback} to receive future updates.

     */

    public void addCallback(Callback<T> callback) {
@@ -185,7 +189,7 @@ public class ObservableServiceConnection<T> implements ServiceConnection { 
            Log.d(TAG, "addCallback:" + callback);

        }



        mExecutor.execute(() -> {

        mBgExecutor.execute(() -> {

            final Iterator<WeakReference<Callback<T>>> iterator = mCallbacks.iterator();



            while (iterator.hasNext()) {
@@ -210,14 +214,15 @@ public class ObservableServiceConnection<T> implements ServiceConnection { 
     * Removes previously added callback from receiving future connection updates.

     * @param callback The {@link Callback} to be removed.

     */

    public void removeCallback(Callback callback) {

    public void removeCallback(Callback<T> callback) {

        if (DEBUG) {

            Log.d(TAG, "removeCallback:" + callback);

        }



        mExecutor.execute(() -> mCallbacks.removeIf(el-> el.get() == callback));

        mBgExecutor.execute(() -> mCallbacks.removeIf(el-> el.get() == callback));

    }



    @WorkerThread

    private void onDisconnected(@DisconnectReason int reason) {

        if (DEBUG) {

            Log.d(TAG, "onDisconnected:" + reason);
@@ -240,7 +245,7 @@ public class ObservableServiceConnection<T> implements ServiceConnection { 


    @Override

    public void onServiceConnected(ComponentName name, IBinder service) {

        mExecutor.execute(() -> {

        mBgExecutor.execute(() -> {

            if (DEBUG) {

                Log.d(TAG, "onServiceConnected");

            }
@@ -268,7 +273,7 @@ public class ObservableServiceConnection<T> implements ServiceConnection { 
        final Iterator<WeakReference<Callback<T>>> iterator = mCallbacks.iterator();



        while (iterator.hasNext()) {

            final Callback cb = iterator.next().get();

            final Callback<T> cb = iterator.next().get();

            if (cb != null) {

                applicator.accept(cb);

            } else {
@@ -279,16 +284,16 @@ public class ObservableServiceConnection<T> implements ServiceConnection { 


    @Override

    public void onServiceDisconnected(ComponentName name) {

        mExecutor.execute(() -> onDisconnected(DISCONNECT_REASON_DISCONNECTED));

        mBgExecutor.execute(() -> onDisconnected(DISCONNECT_REASON_DISCONNECTED));

    }



    @Override

    public void onBindingDied(ComponentName name) {

        mExecutor.execute(() -> onDisconnected(DISCONNECT_REASON_BINDING_DIED));

        mBgExecutor.execute(() -> onDisconnected(DISCONNECT_REASON_BINDING_DIED));

    }



    @Override

    public void onNullBinding(ComponentName name) {

        mExecutor.execute(() -> onDisconnected(DISCONNECT_REASON_NULL_BINDING));

        mBgExecutor.execute(() -> onDisconnected(DISCONNECT_REASON_NULL_BINDING));

    }

}

packages/SystemUI/src/com/android/systemui/util/service/PersistentConnectionManager.java

+6 −8
Original line number Diff line number Diff line
@@ -48,7 +48,7 @@ public class PersistentConnectionManager<T> implements Dumpable { 
    private static final boolean DEBUG = Log.isLoggable(TAG, Log.DEBUG);



    private final SystemClock mSystemClock;

    private final DelayableExecutor mMainExecutor;

    private final DelayableExecutor mBgExecutor;

    private final int mBaseReconnectDelayMs;

    private final int mMaxReconnectAttempts;

    private final int mMinConnectionDuration;
@@ -71,8 +71,8 @@ public class PersistentConnectionManager<T> implements Dumpable { 


    private final Observer.Callback mObserverCallback = () -> initiateConnectionAttempt();



    private final ObservableServiceConnection.Callback mConnectionCallback =

            new ObservableServiceConnection.Callback() {

    private final ObservableServiceConnection.Callback<T> mConnectionCallback =

            new ObservableServiceConnection.Callback<>() {

        private long mStartTime;



        @Override
@@ -95,12 +95,10 @@ public class PersistentConnectionManager<T> implements Dumpable { 
        }

    };



    // TODO: b/326449074 - Ensure the DelayableExecutor is on the correct thread, and update the

    //                     qualifier (to @Main) or name (to bgExecutor) to be consistent with that.

    @Inject

    public PersistentConnectionManager(

            SystemClock clock,

            @Background DelayableExecutor mainExecutor,

            @Background DelayableExecutor bgExecutor,

            DumpManager dumpManager,

            @Named(DUMPSYS_NAME) String dumpsysName,

            @Named(SERVICE_CONNECTION) ObservableServiceConnection<T> serviceConnection,
@@ -109,7 +107,7 @@ public class PersistentConnectionManager<T> implements Dumpable { 
            @Named(MIN_CONNECTION_DURATION_MS) int minConnectionDurationMs,

            @Named(OBSERVER) Observer observer) {

        mSystemClock = clock;

        mMainExecutor = mainExecutor;

        mBgExecutor = bgExecutor;

        mConnection = serviceConnection;

        mObserver = observer;

        mDumpManager = dumpManager;
@@ -195,7 +193,7 @@ public class PersistentConnectionManager<T> implements Dumpable { 
                    "scheduling connection attempt in " + reconnectDelayMs + "milliseconds");

        }



        mCurrentReconnectCancelable = mMainExecutor.executeDelayed(mConnectRunnable,

        mCurrentReconnectCancelable = mBgExecutor.executeDelayed(mConnectRunnable,

                reconnectDelayMs);



        mReconnectAttempts++;

packages/SystemUI/tests/src/com/android/systemui/util/service/ObservableServiceConnectionTest.java

+8 −5
Original line number Diff line number Diff line
@@ -16,8 +16,6 @@ 


package com.android.systemui.util.service;



import static com.google.common.truth.Truth.assertThat;



import static org.mockito.ArgumentMatchers.anyInt;

import static org.mockito.ArgumentMatchers.eq;

import static org.mockito.Mockito.clearInvocations;
@@ -118,6 +116,7 @@ public class ObservableServiceConnectionTest extends SysuiTestCase { 
        connection.addCallback(mCallback);

        mExecutor.runAllReady();

        connection.bind();

        mExecutor.runAllReady();



        when(mTransformer.convert(eq(mBinder))).thenReturn(mResult);
@@ -143,8 +142,8 @@ public class ObservableServiceConnectionTest extends SysuiTestCase { 
        when(mContext.bindServiceAsUser(eq(mIntent), eq(connection), anyInt(),

                eq(UserHandle.of(MAIN_USER_ID)))).thenReturn(true);

        connection.bind();

        mExecutor.runAllReady();

        connection.onServiceDisconnected(mComponentName);



        mExecutor.runAllReady();



        // Ensure proper disconnect reason reported back
@@ -157,6 +156,7 @@ public class ObservableServiceConnectionTest extends SysuiTestCase { 
        clearInvocations(mContext);

        // Ensure unbind after disconnect has no effect on the connection

        connection.unbind();

        mExecutor.runAllReady();

        verify(mContext, never()).unbindService(eq(connection));

    }
@@ -197,7 +197,8 @@ public class ObservableServiceConnectionTest extends SysuiTestCase { 


        // Verify that the exception was caught and that bind returns false, and we properly

        // unbind.

        assertThat(connection.bind()).isFalse();

        connection.bind();

        mExecutor.runAllReady();

        verify(mContext).unbindService(connection);

    }
@@ -212,13 +213,15 @@ public class ObservableServiceConnectionTest extends SysuiTestCase { 
                .thenThrow(new SecurityException());



        // Verify that bind returns false and we properly unbind.

        assertThat(connection.bind()).isFalse();

        connection.bind();

        mExecutor.runAllReady();

        verify(mContext).unbindService(connection);



        clearInvocations(mContext);



        // Ensure unbind after the failed bind has no effect.

        connection.unbind();

        mExecutor.runAllReady();

        verify(mContext, never()).unbindService(eq(connection));

    }

}