Merge "Fix SoundTriggerMiddleware deadlock issues" into rvc-dev (9faccb66) · Commits · e / os / android_frameworks_base

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();
		}
		}
		}
		}