diff options
author | 2024-12-30 14:02:14 -0800 | |
---|---|---|
committer | 2025-01-02 10:09:09 -0800 | |
commit | c72b88f3a0a4b823b5b94058cb2876806f5fdab7 (patch) | |
tree | 6fa3a5367fcf7a20f11746c0a19b3147c96e8e57 | |
parent | 6b4d2a3c309cb0781f80efb9b5d202f900192d83 (diff) |
Retry property set failures
SystemProperties.set() is not a reliable method, for reasons that are
unknown. This change adds the utility class SystemPropertySetter with
a static setWithRetry() method. There are two overloads: one which
takes the maximum number of retries and the delay between retries, and
one which uses new system constants.
Clients should use the new method if set() failures are fatal.
Note that some failures cannot be recovered by retrying. SELinux
denials are one example. However, such errors are almost always
coding errors and not operational errors, and so should be discovered
during development.
The code, along with the constants, is ported from
PropertyInvalidatedCache, where it has been use successfully for
several years.
The new method is used in AudioService and in PIC.
The new methods are not system APIs and do not have CTS tests.
Flag: EXEMPT bug-fix
Bug: 383751329
Test: atest
* CtsVirtualDevicesAudioTestCases
* android.media.audio.cts.AudioFocusTest
* android.app.appops.cts.AppOpsLoggingTest
Change-Id: Ifb8e2bf42e582cf77c7c74d3e3dfb7cd4abb7710
-rw-r--r-- | core/java/android/app/PropertyInvalidatedCache.java | 47 | ||||
-rw-r--r-- | core/java/android/util/SystemPropertySetter.java | 103 | ||||
-rw-r--r-- | services/core/java/com/android/server/audio/AudioService.java | 3 |
3 files changed, 107 insertions, 46 deletions
diff --git a/core/java/android/app/PropertyInvalidatedCache.java b/core/java/android/app/PropertyInvalidatedCache.java index 5567c085d205..e7e9b0027812 100644 --- a/core/java/android/app/PropertyInvalidatedCache.java +++ b/core/java/android/app/PropertyInvalidatedCache.java @@ -36,6 +36,7 @@ import android.util.ArraySet; import android.util.Log; import android.util.SparseArray; import android.util.SparseBooleanArray; +import android.util.SystemPropertySetter; import com.android.internal.annotations.GuardedBy; import com.android.internal.annotations.VisibleForTesting; @@ -157,21 +158,6 @@ public class PropertyInvalidatedCache<Query, Result> { public static final String MODULE_TELEPHONY = "telephony"; /** - * Constants that affect retries when the process is unable to write the property. - * The first constant is the number of times the process will attempt to set the - * property. The second constant is the delay between attempts. - */ - - /** - * Wait 200ms between retry attempts and the retry limit is 5. That gives a total possible - * delay of 1s, which should be less than ANR timeouts. The goal is to have the system crash - * because the property could not be set (which is a condition that is easily recognized) and - * not crash because of an ANR (which can be confusing to debug). - */ - private static final int PROPERTY_FAILURE_RETRY_DELAY_MILLIS = 200; - private static final int PROPERTY_FAILURE_RETRY_LIMIT = 5; - - /** * Construct a system property that matches the rules described above. The module is * one of the permitted values above. The API is a string that is a legal Java simple * identifier. The api is modified to conform to the system property style guide by @@ -958,37 +944,8 @@ public class PropertyInvalidatedCache<Query, Result> { */ @Override void setNonceInternal(long value) { - // Failing to set the nonce is a fatal error. Failures setting a system property have - // been reported; given that the failure is probably transient, this function includes - // a retry. final String str = Long.toString(value); - RuntimeException failure = null; - for (int attempt = 0; attempt < PROPERTY_FAILURE_RETRY_LIMIT; attempt++) { - try { - SystemProperties.set(mName, str); - if (attempt > 0) { - // This log is not guarded. Based on known bug reports, it should - // occur once a week or less. The purpose of the log message is to - // identify the retries as a source of delay that might be otherwise - // be attributed to the cache itself. - Log.w(TAG, "Nonce set after " + attempt + " tries"); - } - return; - } catch (RuntimeException e) { - if (failure == null) { - failure = e; - } - try { - Thread.sleep(PROPERTY_FAILURE_RETRY_DELAY_MILLIS); - } catch (InterruptedException x) { - // Ignore this exception. The desired delay is only approximate and - // there is no issue if the sleep sometimes terminates early. - } - } - } - // This point is reached only if SystemProperties.set() fails at least once. - // Rethrow the first exception that was received. - throw failure; + SystemPropertySetter.setWithRetry(mName, str); } } diff --git a/core/java/android/util/SystemPropertySetter.java b/core/java/android/util/SystemPropertySetter.java new file mode 100644 index 000000000000..bf18f753231e --- /dev/null +++ b/core/java/android/util/SystemPropertySetter.java @@ -0,0 +1,103 @@ +/* + * Copyright (C) 2025 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package android.util; + +import android.annotation.NonNull; +import android.annotation.Nullable; +import android.os.SystemProperties; + +/** + * This class provides a single, static function to set a system property. The function retries on + * error. System properties should be reliable but there have been reports of failures in the set() + * command on lower-end devices. Clients may want to use this method instead of calling + * {@link SystemProperties.set} directly. + * @hide + */ +public class SystemPropertySetter { + + /** + * The default retryDelayMs for {@link #setWithRetry}. This value has been found to give + * reasonable behavior in the field. + */ + public static final int PROPERTY_FAILURE_RETRY_DELAY_MILLIS = 200; + + /** + * The default retryLimit for {@link #setWithRetry}. This value has been found to give + * reasonable behavior in the field. + */ + public static final int PROPERTY_FAILURE_RETRY_LIMIT = 5; + + /** + * Set the value for the given {@code key} to {@code val}. This method retries using the + * standard parameters, above, if the native method throws a RuntimeException. + * + * @param key The name of the property to be set. + * @param val The new string value of the property. + * @throws IllegalArgumentException for non read-only properties if the {@code val} exceeds + * 91 characters. + * @throws RuntimeException if the property cannot be set, for example, if it was blocked by + * SELinux. libc will log the underlying reason. + */ + public static void setWithRetry(@NonNull String key, @Nullable String val) { + setWithRetry(key, val,PROPERTY_FAILURE_RETRY_DELAY_MILLIS, PROPERTY_FAILURE_RETRY_LIMIT); + } + + /** + * Set the value for the given {@code key} to {@code val}. This method retries if the native + * method throws a RuntimeException. If the {@code maxRetry} count is exceeded, the method + * throws the first RuntimeException that was seen. + * + * @param key The name of the property to be set. + * @param val The new string value of the property. + * @param maxRetry The maximum number of times; must be non-negative. + * @param retryDelayMs The number of milliseconds to wait between retries; must be positive. + * @throws IllegalArgumentException for non read-only properties if the {@code val} exceeds + * 91 characters, or if the retry parameters are invalid. + * @throws RuntimeException if the property cannot be set, for example, if it was blocked by + * SELinux. libc will log the underlying reason. + */ + public static void setWithRetry(@NonNull String key, @Nullable String val, int maxRetry, + long retryDelayMs) { + if (maxRetry < 0) { + throw new IllegalArgumentException("invalid retry count: " + maxRetry); + } + if (retryDelayMs <= 0) { + throw new IllegalArgumentException("invalid retry delay: " + retryDelayMs); + } + + RuntimeException failure = null; + for (int attempt = 0; attempt < maxRetry; attempt++) { + try { + SystemProperties.set(key, val); + return; + } catch (RuntimeException e) { + if (failure == null) { + failure = e; + } + try { + Thread.sleep(retryDelayMs); + } catch (InterruptedException x) { + // Ignore this exception. The desired delay is only approximate and + // there is no issue if the sleep sometimes terminates early. + } + } + } + // This point is reached only if SystemProperties.set() fails at least once. + // Rethrow the first exception that was received. + throw failure; + } +} diff --git a/services/core/java/com/android/server/audio/AudioService.java b/services/core/java/com/android/server/audio/AudioService.java index 0fd47169122b..bd2714211796 100644 --- a/services/core/java/com/android/server/audio/AudioService.java +++ b/services/core/java/com/android/server/audio/AudioService.java @@ -242,6 +242,7 @@ import android.util.PrintWriterPrinter; import android.util.Slog; import android.util.SparseArray; import android.util.SparseIntArray; +import android.util.SystemPropertySetter; import android.view.Display; import android.view.KeyEvent; import android.view.accessibility.AccessibilityManager; @@ -11032,7 +11033,7 @@ public class AudioService extends IAudioService.Stub @GuardedBy("mLock") private void updateLocked() { String n = Long.toString(mToken++); - SystemProperties.set(PermissionManager.CACHE_KEY_PACKAGE_INFO_NOTIFY, n); + SystemPropertySetter.setWithRetry(PermissionManager.CACHE_KEY_PACKAGE_INFO_NOTIFY, n); } private void trigger() { |