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

Commit 21c886cc authored by Ytai Ben-Tsvi's avatar Ytai Ben-Tsvi
Browse files

Fix SoundTriggerMiddleware deadlock issues 

This change enforces strict lock ordering between the various layers
of the service and the audio policy lock. Previously, we were exposed
to various deadlock scenarios due to nested locking done without
consistent lock ordering, specifically in cases involving callbacks
from SoundTriggerModule to SoundTriggerMiddlewareValidation.



Change-Id: I28382d10c7bc7faaa8f9b33dda3aa9c94a68061e
Bug: 159151235
Test: Manually disable concurrent capture, verify basic soundtrigger
      functionality and do some stress testing by enabling/disabling
      recording quickly and check for deadlocks.
parent df38e692

services/core/java/com/android/server/soundtrigger_middleware/README.md

0 → 100644
+19 −0
Original line number Diff line number Diff line 
# Sound Trigger Middleware

TODO: Add component description.



## Notes about thread synchronization

This component has some tricky thread synchronization considerations due to its layered design and

due to the fact that it is involved in both in-bound and out-bound calls from / to

external components. To avoid potential deadlocks, a strict locking order must be ensured whenever

nesting locks. The order is:

- `SoundTriggerMiddlewareValidation` lock.

- Audio policy service lock. This one is external - it should be assumed to be held whenever we're

  inside the `ExternalCaptureStateTracker.setCaptureState()` call stack *AND* to be acquired from

  within our calls into `AudioSessionProvider.acquireSession()`.

- `SoundTriggerModule` lock.



This dictates careful consideration of callbacks going from `SoundTriggerModule` to

`SoundTriggerMiddlewareValidation` and especially those coming from the `setCaptureState()` path.

We always invoke those calls outside of the `SoundTriggerModule` lock, so we can lock

`SoundTriggerMiddlewareValidation`. However, in the `setCaptureState()` case, we have to use atomics

in `SoundTriggerMiddlewareValidation` and avoid the lock.

services/core/java/com/android/server/soundtrigger_middleware/SoundTriggerMiddlewareValidation.java

+76 −47
Original line number Diff line number Diff line
@@ -47,6 +47,9 @@ import java.util.HashSet; 
import java.util.Map;

import java.util.Objects;

import java.util.Set;

import java.util.concurrent.ConcurrentHashMap;

import java.util.concurrent.ConcurrentMap;

import java.util.concurrent.atomic.AtomicInteger;

import java.util.concurrent.atomic.AtomicReference;



/**
@@ -328,7 +331,7 @@ public class SoundTriggerMiddlewareValidation implements ISoundTriggerMiddleware 
        }



        /** Activity state. */

        Activity activityState = Activity.LOADED;

        private AtomicInteger mActivityState = new AtomicInteger(Activity.LOADED.ordinal());



        /** Human-readable description of the model. */

        final String description;
@@ -383,6 +386,14 @@ public class SoundTriggerMiddlewareValidation implements ISoundTriggerMiddleware 
        void updateParameterSupport(int modelParam, @Nullable ModelParameterRange range) {

            parameterSupport.put(modelParam, range);

        }



        Activity getActivityState() {

            return Activity.values()[mActivityState.get()];

        }



        void setActivityState(Activity activity) {

            mActivityState.set(activity.ordinal());

        }

    }



    /**
@@ -393,7 +404,13 @@ public class SoundTriggerMiddlewareValidation implements ISoundTriggerMiddleware 
            IBinder.DeathRecipient {

        private final ISoundTriggerCallback mCallback;

        private ISoundTriggerModule mDelegate;

        private @NonNull Map<Integer, ModelState> mLoadedModels = new HashMap<>();

        // While generally all the fields of this class must be changed under a lock, an exception

        // is made for the specific case of changing a model state from ACTIVE to LOADED, which

        // may happen as result of a recognition callback. This would happen atomically and is

        // necessary in order to avoid deadlocks associated with locking from within callbacks

        // possibly originating from the audio server.

        private @NonNull

        ConcurrentMap<Integer, ModelState> mLoadedModels = new ConcurrentHashMap<>();

        private final int mHandle;

        private ModuleStatus mState = ModuleStatus.ALIVE;
@@ -476,10 +493,9 @@ public class SoundTriggerMiddlewareValidation implements ISoundTriggerMiddleware 
                if (modelState == null) {

                    throw new IllegalStateException("Invalid handle: " + modelHandle);

                }

                if (modelState.activityState

                        != ModelState.Activity.LOADED) {

                if (modelState.getActivityState() != ModelState.Activity.LOADED) {

                    throw new IllegalStateException("Model with handle: " + modelHandle

                            + " has invalid state for unloading: " + modelState.activityState);

                            + " has invalid state for unloading: " + modelState.getActivityState());

                }



                // From here on, every exception isn't client's fault.
@@ -509,19 +525,21 @@ public class SoundTriggerMiddlewareValidation implements ISoundTriggerMiddleware 
                if (modelState == null) {

                    throw new IllegalStateException("Invalid handle: " + modelHandle);

                }

                if (modelState.activityState

                        != ModelState.Activity.LOADED) {

                if (modelState.getActivityState() != ModelState.Activity.LOADED) {

                    throw new IllegalStateException("Model with handle: " + modelHandle

                            + " has invalid state for starting recognition: "

                            + modelState.activityState);

                            + modelState.getActivityState());

                }



                // From here on, every exception isn't client's fault.

                try {

                    // Normally, we would set the state after the operation succeeds. However, since

                    // the activity state may be reset outside of the lock, we set it here first,

                    // and reset it in case of exception.

                    modelState.setActivityState(ModelState.Activity.ACTIVE);

                    mDelegate.startRecognition(modelHandle, config);

                    modelState.activityState =

                            ModelState.Activity.ACTIVE;

                } catch (Exception e) {

                    modelState.setActivityState(ModelState.Activity.LOADED);

                    throw handleException(e);

                }

            }
@@ -548,8 +566,7 @@ public class SoundTriggerMiddlewareValidation implements ISoundTriggerMiddleware 
                // From here on, every exception isn't client's fault.

                try {

                    mDelegate.stopRecognition(modelHandle);

                    modelState.activityState =

                            ModelState.Activity.LOADED;

                    modelState.setActivityState(ModelState.Activity.LOADED);

                } catch (Exception e) {

                    throw handleException(e);

                }
@@ -719,7 +736,7 @@ public class SoundTriggerMiddlewareValidation implements ISoundTriggerMiddleware 
                for (Map.Entry<Integer, ModelState> entry : mLoadedModels.entrySet()) {

                    pw.print(entry.getKey());

                    pw.print('\t');

                    pw.print(entry.getValue().activityState.name());

                    pw.print(entry.getValue().getActivityState().name());

                    pw.print('\t');

                    pw.print(entry.getValue().description);

                    pw.println();
@@ -735,10 +752,18 @@ public class SoundTriggerMiddlewareValidation implements ISoundTriggerMiddleware 


        @Override

        public void onRecognition(int modelHandle, @NonNull RecognitionEvent event) {

            synchronized (SoundTriggerMiddlewareValidation.this) {

            // We cannot obtain a lock on SoundTriggerMiddlewareValidation.this, since this call

            // might be coming from the audio server (via setCaptureState()) while it is holding

            // a lock that is also acquired while loading / unloading models. Thus, we require a

            // strict locking order here, where obtaining our lock must always come first.

            // To avoid this problem, we use an atomic model activity state. There is a risk of the

            // model not being in the mLoadedModels map here, since it might have been stopped /

            // unloaded while the event was in flight.

            if (event.status != RecognitionStatus.FORCED) {

                    mLoadedModels.get(modelHandle).activityState =

                            ModelState.Activity.LOADED;

                ModelState modelState = mLoadedModels.get(modelHandle);

                if (modelState != null) {

                    modelState.setActivityState(ModelState.Activity.LOADED);

                }

            }

            try {

                mCallback.onRecognition(modelHandle, event);
@@ -748,14 +773,21 @@ public class SoundTriggerMiddlewareValidation implements ISoundTriggerMiddleware 
                Log.e(TAG, "Client callback exception.", e);

            }

        }

        }



        @Override

        public void onPhraseRecognition(int modelHandle, @NonNull PhraseRecognitionEvent event) {

            synchronized (SoundTriggerMiddlewareValidation.this) {

            // We cannot obtain a lock on SoundTriggerMiddlewareValidation.this, since this call

            // might be coming from the audio server (via setCaptureState()) while it is holding

            // a lock that is also acquired while loading / unloading models. Thus, we require a

            // strict locking order here, where obtaining our lock must always come first.

            // To avoid this problem, we use an atomic model activity state. There is a risk of the

            // model not being in the mLoadedModels map here, since it might have been stopped /

            // unloaded while the event was in flight.

            if (event.common.status != RecognitionStatus.FORCED) {

                    mLoadedModels.get(modelHandle).activityState =

                            ModelState.Activity.LOADED;

                ModelState modelState = mLoadedModels.get(modelHandle);

                if (modelState != null) {

                    modelState.setActivityState(ModelState.Activity.LOADED);

                }

            }

            try {

                mCallback.onPhraseRecognition(modelHandle, event);
@@ -765,11 +797,10 @@ public class SoundTriggerMiddlewareValidation implements ISoundTriggerMiddleware 
                Log.e(TAG, "Client callback exception.", e);

            }

        }

        }



        @Override

        public void onRecognitionAvailabilityChange(boolean available) {

            synchronized (SoundTriggerMiddlewareValidation.this) {

            // Not locking to avoid deadlocks (not affecting any state).

            try {

                mCallback.onRecognitionAvailabilityChange(available);

            } catch (RemoteException e) {
@@ -778,7 +809,6 @@ public class SoundTriggerMiddlewareValidation implements ISoundTriggerMiddleware 
                Log.e(TAG, "Client callback exception.", e);

            }

        }

        }



        @Override

        public void onModuleDied() {
@@ -804,10 +834,9 @@ public class SoundTriggerMiddlewareValidation implements ISoundTriggerMiddleware 
                    // Gracefully stop all active recognitions and unload the models.

                    for (Map.Entry<Integer, ModelState> entry :

                            mLoadedModels.entrySet()) {

                        if (entry.getValue().activityState

                                == ModelState.Activity.ACTIVE) {

                        // Idempotent call, no harm in calling even for models that are already

                        // stopped.

                        mDelegate.stopRecognition(entry.getKey());

                        }

                        mDelegate.unloadModel(entry.getKey());

                    }

                    // Detach.

services/core/java/com/android/server/soundtrigger_middleware/SoundTriggerModule.java

+68 −39
Original line number Diff line number Diff line
@@ -42,6 +42,7 @@ import android.util.Log; 
import java.util.ArrayList;

import java.util.HashMap;

import java.util.HashSet;

import java.util.LinkedList;

import java.util.List;

import java.util.Map;

import java.util.Set;
@@ -153,7 +154,13 @@ class SoundTriggerModule implements IHwBinder.DeathRecipient { 
     *

     * @param active true iff external capture is active.

     */

    synchronized void setExternalCaptureState(boolean active) {

    void setExternalCaptureState(boolean active) {

        // We should never invoke callbacks while holding the lock, since this may deadlock with

        // forward calls. Thus, we first gather all the callbacks we need to invoke while holding

        // the lock, but invoke them after releasing it.

        List<Runnable> callbacks = new LinkedList<>();



        synchronized (this) {

            if (mProperties.concurrentCapture) {

                // If we support concurrent capture, we don't care about any of this.

                return;
@@ -163,9 +170,13 @@ class SoundTriggerModule implements IHwBinder.DeathRecipient { 
                // Our module does not support recognition while a capture is active -

                // need to abort all active recognitions.

                for (Session session : mActiveSessions) {

                session.abortActiveRecognitions();

                    session.abortActiveRecognitions(callbacks);

                }

            }

        }

        for (Runnable callback : callbacks) {

            callback.run();

        }

        for (Session session : mActiveSessions) {

            session.notifyRecognitionAvailability();

        }
@@ -329,9 +340,18 @@ class SoundTriggerModule implements IHwBinder.DeathRecipient { 


        @Override

        public void startRecognition(int modelHandle, @NonNull RecognitionConfig config) {

            // We should never invoke callbacks while holding the lock, since this may deadlock with

            // forward calls. Thus, we first gather all the callbacks we need to invoke while holding

            // the lock, but invoke them after releasing it.

            List<Runnable> callbacks = new LinkedList<>();



            synchronized (SoundTriggerModule.this) {

                checkValid();

                mLoadedModels.get(modelHandle).startRecognition(config);

                mLoadedModels.get(modelHandle).startRecognition(config, callbacks);

            }



            for (Runnable callback : callbacks) {

                callback.run();

            }

        }
@@ -377,10 +397,12 @@ class SoundTriggerModule implements IHwBinder.DeathRecipient { 


        /**

         * Abort all currently active recognitions.

         * @param callbacks Will be appended with a list of callbacks that need to be invoked

         *                  after this method returns, without holding the module lock.

         */

        private void abortActiveRecognitions() {

        private void abortActiveRecognitions(@NonNull List<Runnable> callbacks) {

            for (Model model : mLoadedModels.values()) {

                model.abortActiveRecognition();

                model.abortActiveRecognition(callbacks);

            }

        }
@@ -475,10 +497,11 @@ class SoundTriggerModule implements IHwBinder.DeathRecipient { 
                return mSession.mSessionHandle;

            }



            private void startRecognition(@NonNull RecognitionConfig config) {

            private void startRecognition(@NonNull RecognitionConfig config,

                    @NonNull List<Runnable> callbacks) {

                if (!mRecognitionAvailable) {

                    // Recognition is unavailable - send an abort event immediately.

                    notifyAbort();

                    callbacks.add(this::notifyAbort);

                    return;

                }

                android.hardware.soundtrigger.V2_3.RecognitionConfig hidlConfig =
@@ -525,8 +548,12 @@ class SoundTriggerModule implements IHwBinder.DeathRecipient { 
                                ConversionUtil.aidl2hidlModelParameter(modelParam)));

            }



            /** Abort the recognition, if active. */

            private void abortActiveRecognition() {

            /**

             * Abort the recognition, if active.

             * @param callbacks Will be appended with a list of callbacks that need to be invoked

             *                  after this method returns, without holding the module lock.

             */

            private void abortActiveRecognition(List<Runnable> callbacks) {

                // If we're inactive, do nothing.

                if (getState() != ModelState.ACTIVE) {

                    return;
@@ -535,7 +562,7 @@ class SoundTriggerModule implements IHwBinder.DeathRecipient { 
                stopRecognition();



                // Notify the client that recognition has been aborted.

                notifyAbort();

                callbacks.add(this::notifyAbort);

            }



            /** Notify the client that recognition has been aborted. */
@@ -577,42 +604,44 @@ class SoundTriggerModule implements IHwBinder.DeathRecipient { 
            public void recognitionCallback(

                    @NonNull ISoundTriggerHwCallback.RecognitionEvent recognitionEvent,

                    int cookie) {

                synchronized (SoundTriggerModule.this) {

                RecognitionEvent aidlEvent =

                        ConversionUtil.hidl2aidlRecognitionEvent(recognitionEvent);

                aidlEvent.captureSession = mSession.mSessionHandle;

                    try {

                        mCallback.onRecognition(mHandle, aidlEvent);

                    } catch (RemoteException e) {

                        // Dead client will be handled by binderDied() - no need to handle here.

                        // In any case, client callbacks are considered best effort.

                        Log.e(TAG, "Client callback execption.", e);

                    }

                synchronized (SoundTriggerModule.this) {

                    if (aidlEvent.status != RecognitionStatus.FORCED) {

                        setState(ModelState.LOADED);

                    }

                }

                // The callback must be invoked outside of the lock.

                try {

                    mCallback.onRecognition(mHandle, aidlEvent);

                } catch (RemoteException e) {

                    // We're not expecting any exceptions here.

                    throw e.rethrowAsRuntimeException();

                }

            }



            @Override

            public void phraseRecognitionCallback(

                    @NonNull ISoundTriggerHwCallback.PhraseRecognitionEvent phraseRecognitionEvent,

                    int cookie) {

                synchronized (SoundTriggerModule.this) {

                PhraseRecognitionEvent aidlEvent =

                        ConversionUtil.hidl2aidlPhraseRecognitionEvent(phraseRecognitionEvent);

                aidlEvent.common.captureSession = mSession.mSessionHandle;

                    try {

                        mCallback.onPhraseRecognition(mHandle, aidlEvent);

                    } catch (RemoteException e) {

                        // Dead client will be handled by binderDied() - no need to handle here.

                        // In any case, client callbacks are considered best effort.

                        Log.e(TAG, "Client callback execption.", e);

                    }



                synchronized (SoundTriggerModule.this) {

                    if (aidlEvent.common.status != RecognitionStatus.FORCED) {

                        setState(ModelState.LOADED);

                    }

                }



                // The callback must be invoked outside of the lock.

                try {

                    mCallback.onPhraseRecognition(mHandle, aidlEvent);

                } catch (RemoteException e) {

                    // We're not expecting any exceptions here.

                    throw e.rethrowAsRuntimeException();

                }

            }

        }

    }