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

Commit 8e65afd5 authored by Neil Fuller's avatar Neil Fuller
Browse files

Follow best practice for listener notification 

When notifying listeners, it's best practice not to hold any locks.
Holding locks during synchronous notifications can cause deadlocks.

One case has been caught where:

A flag affecting the ConfigurationInternal managed by
timezonedector.ServiceConfigAccessorImpl was updated: this caused the
main thread to hold the ServiceConfigAccessorImpl "this" monitor while
notifying the TimeZoneDetectorStrategyImpl (requiring the
TimeZoneDetectorStrategyImpl "this" lock).

Simultaneously, another (binder) thread was calling into
TimeZoneDetectorStrategyImpl to update user configuration, thus holding
the TimeZoneDetectorStrategyImpl "this" lock but attempting to read the
ConfigurationInternal from the ServiceConfigAccessorImpl, and requiring
the ServiceConfigAccessorImpl "this" monitor. Classic deadlock.

This commit modifies (or comments) listener notification code to prevent
deadlocks.

Bug: 259562789
Test: Treehugger
Change-Id: I93b1d9d82ca96062f760c77043e6837398f03cc5
parent 266adbb2

services/core/java/com/android/server/timedetector/ServerFlags.java

+11 −1
Original line number Diff line number Diff line
@@ -35,7 +35,9 @@ import java.lang.annotation.Target; 
import java.time.DateTimeException;

import java.time.Duration;

import java.time.Instant;

import java.util.ArrayList;

import java.util.HashSet;

import java.util.List;

import java.util.Map;

import java.util.Objects;

import java.util.Optional;
@@ -211,7 +213,11 @@ public final class ServerFlags { 
    }



    private void handlePropertiesChanged(@NonNull DeviceConfig.Properties properties) {

        // Copy the listeners to notify under the "mListeners" lock but don't hold the lock while

        // delivering the notifications to avoid deadlocks.

        List<StateChangeListener> listenersToNotify;

        synchronized (mListeners) {

            listenersToNotify = new ArrayList<>(mListeners.size());

            for (Map.Entry<StateChangeListener, HashSet<String>> listenerEntry

                    : mListeners.entrySet()) {

                // It's unclear which set of the following two Sets is going to be larger in the
@@ -225,10 +231,14 @@ public final class ServerFlags { 
                HashSet<String> monitoredKeys = listenerEntry.getValue();

                Iterable<String> modifiedKeys = properties.getKeyset();

                if (containsAny(monitoredKeys, modifiedKeys)) {

                    listenerEntry.getKey().onChange();

                    listenersToNotify.add(listenerEntry.getKey());

                }

            }

        }



        for (StateChangeListener listener : listenersToNotify) {

            listener.onChange();

        }

    }



    private static boolean containsAny(

services/core/java/com/android/server/timedetector/ServiceConfigAccessorImpl.java

+8 −2
Original line number Diff line number Diff line
@@ -166,8 +166,14 @@ final class ServiceConfigAccessorImpl implements ServiceConfigAccessor { 
        }

    }



    private synchronized void handleConfigurationInternalChangeOnMainThread() {

        for (StateChangeListener changeListener : mConfigurationInternalListeners) {

    private void handleConfigurationInternalChangeOnMainThread() {

        // Copy the listeners holding the "this" lock but don't hold the lock while delivering the

        // notifications to avoid deadlocks.

        List<StateChangeListener> configurationInternalListeners;

        synchronized (this) {

            configurationInternalListeners = new ArrayList<>(this.mConfigurationInternalListeners);

        }

        for (StateChangeListener changeListener : configurationInternalListeners) {

            changeListener.onChange();

        }

    }

services/core/java/com/android/server/timedetector/TimeDetectorService.java

+2 −0
Original line number Diff line number Diff line
@@ -266,6 +266,8 @@ public final class TimeDetectorService extends ITimeDetectorService.Stub 
            for (int listenerIndex = 0; listenerIndex < listenerCount; listenerIndex++) {

                ITimeDetectorListener listener = mListeners.valueAt(listenerIndex);

                try {

                    // No need to surrender the mListeners lock while doing this:

                    // ITimeDetectorListener is declared "oneway".

                    listener.onChange();

                } catch (RemoteException e) {

                    Slog.w(TAG, "Unable to notify listener=" + listener, e);

services/core/java/com/android/server/timezonedetector/DeviceActivityMonitorImpl.java

+8 −2
Original line number Diff line number Diff line
@@ -77,12 +77,18 @@ class DeviceActivityMonitorImpl implements DeviceActivityMonitor { 
        mListeners.add(listener);

    }



    private synchronized void notifyFlightComplete() {

    private void notifyFlightComplete() {

        if (DBG) {

            Slog.d(LOG_TAG, "notifyFlightComplete");

        }



        for (Listener listener : mListeners) {

        // Copy the listeners holding the "this" lock but don't hold the lock while delivering the

        // notifications to avoid deadlocks.

        List<Listener> listeners;

        synchronized (this) {

            listeners = new ArrayList<>(mListeners);

        }

        for (Listener listener : listeners) {

            listener.onFlightComplete();

        }

    }

services/core/java/com/android/server/timezonedetector/ServiceConfigAccessorImpl.java

+8 −2
Original line number Diff line number Diff line
@@ -205,8 +205,14 @@ public final class ServiceConfigAccessorImpl implements ServiceConfigAccessor { 
        }

    }



    private synchronized void handleConfigurationInternalChangeOnMainThread() {

        for (StateChangeListener changeListener : mConfigurationInternalListeners) {

    private void handleConfigurationInternalChangeOnMainThread() {

        // Copy the listeners holding the "this" lock but don't hold the lock while delivering the

        // notifications to avoid deadlocks.

        List<StateChangeListener> configurationInternalListeners;

        synchronized (this) {

            configurationInternalListeners = new ArrayList<>(this.mConfigurationInternalListeners);

        }

        for (StateChangeListener changeListener : configurationInternalListeners) {

            changeListener.onChange();

        }

    }