From 3c7c2b3b85e5cf0d81e1c5c6e8ae3bd10653fe6e Mon Sep 17 00:00:00 2001 From: Neil Fuller Date: Fri, 9 Jul 2021 09:19:22 +0100 Subject: [PATCH] Move temporary failure logic up the hierarchy BinderLocationTimeZoneProvider can temporarily fail if the remote object dies. This was handled in BinderLocationTimeZoneProvider, but it is being moved to LocationTimeZoneProvider in this commit. This makes it easier to fix a bug (also fixed in this commit) where the initialization timeout was not cancelled if the remote peer died during initialization. This would later result in a logged warning when the timeout fired and the LocationTimeZoneProvider was no longer in the "initializing" state. This has only been witnessed during unit tests while debugging. Test: treehugger only Bug: 188780558 Change-Id: I76a22edaf9bea4287d07c408ad43fc3e5a8e49fc --- .../BinderLocationTimeZoneProvider.java | 46 +------------ .../location/LocationTimeZoneProvider.java | 69 +++++++++++++++---- 2 files changed, 58 insertions(+), 57 deletions(-) diff --git a/services/core/java/com/android/server/timezonedetector/location/BinderLocationTimeZoneProvider.java b/services/core/java/com/android/server/timezonedetector/location/BinderLocationTimeZoneProvider.java index f054c5756e20a..59df6ff3582e8 100644 --- a/services/core/java/com/android/server/timezonedetector/location/BinderLocationTimeZoneProvider.java +++ b/services/core/java/com/android/server/timezonedetector/location/BinderLocationTimeZoneProvider.java @@ -67,7 +67,7 @@ class BinderLocationTimeZoneProvider extends LocationTimeZoneProvider { @Override public void onProviderUnbound() { - handleProviderLost("onProviderUnbound()"); + handleTemporaryFailure("onProviderUnbound()"); } }); } @@ -77,50 +77,6 @@ class BinderLocationTimeZoneProvider extends LocationTimeZoneProvider { mProxy.destroy(); } - private void handleProviderLost(String reason) { - mThreadingDomain.assertCurrentThread(); - - synchronized (mSharedLock) { - ProviderState currentState = mCurrentState.get(); - switch (currentState.stateEnum) { - case PROVIDER_STATE_STARTED_INITIALIZING: - case PROVIDER_STATE_STARTED_UNCERTAIN: - case PROVIDER_STATE_STARTED_CERTAIN: { - // Losing a remote provider is treated as becoming uncertain. - String msg = "handleProviderLost reason=" + reason - + ", mProviderName=" + mProviderName - + ", currentState=" + currentState; - debugLog(msg); - // This is an unusual PROVIDER_STATE_STARTED_UNCERTAIN state because - // event == null - ProviderState newState = currentState.newState( - PROVIDER_STATE_STARTED_UNCERTAIN, null, - currentState.currentUserConfiguration, msg); - setCurrentState(newState, true); - break; - } - case PROVIDER_STATE_STOPPED: { - debugLog("handleProviderLost reason=" + reason - + ", mProviderName=" + mProviderName - + ", currentState=" + currentState - + ": No state change required, provider is stopped."); - break; - } - case PROVIDER_STATE_PERM_FAILED: - case PROVIDER_STATE_DESTROYED: { - debugLog("handleProviderLost reason=" + reason - + ", mProviderName=" + mProviderName - + ", currentState=" + currentState - + ": No state change required, provider is terminated."); - break; - } - default: { - throw new IllegalStateException("Unknown currentState=" + currentState); - } - } - } - } - private void handleOnProviderBound() { mThreadingDomain.assertCurrentThread(); diff --git a/services/core/java/com/android/server/timezonedetector/location/LocationTimeZoneProvider.java b/services/core/java/com/android/server/timezonedetector/location/LocationTimeZoneProvider.java index e116a8742208d..c2add2d85d260 100644 --- a/services/core/java/com/android/server/timezonedetector/location/LocationTimeZoneProvider.java +++ b/services/core/java/com/android/server/timezonedetector/location/LocationTimeZoneProvider.java @@ -592,9 +592,7 @@ abstract class LocationTimeZoneProvider implements Dumpable { PROVIDER_STATE_STOPPED, null, null, "stopUpdates() called"); setCurrentState(newState, false); - if (mInitializationTimeoutQueue.hasQueued()) { - mInitializationTimeoutQueue.cancel(); - } + cancelInitializationTimeoutIfSet(); onStopUpdates(); } @@ -656,9 +654,7 @@ abstract class LocationTimeZoneProvider implements Dumpable { ProviderState newState = currentState.newState( PROVIDER_STATE_PERM_FAILED, null, null, msg); setCurrentState(newState, true); - if (mInitializationTimeoutQueue.hasQueued()) { - mInitializationTimeoutQueue.cancel(); - } + cancelInitializationTimeoutIfSet(); return; } case EVENT_TYPE_SUGGESTION: @@ -691,9 +687,7 @@ abstract class LocationTimeZoneProvider implements Dumpable { ProviderState newState = currentState.newState( PROVIDER_STATE_PERM_FAILED, null, null, msg); setCurrentState(newState, true); - if (mInitializationTimeoutQueue.hasQueued()) { - mInitializationTimeoutQueue.cancel(); - } + cancelInitializationTimeoutIfSet(); return; } @@ -709,9 +703,7 @@ abstract class LocationTimeZoneProvider implements Dumpable { timeZoneProviderEvent, currentState.currentUserConfiguration, "handleTimeZoneProviderEvent() when started"); setCurrentState(newState, true); - if (mInitializationTimeoutQueue.hasQueued()) { - mInitializationTimeoutQueue.cancel(); - } + cancelInitializationTimeoutIfSet(); return; } default: { @@ -727,6 +719,52 @@ abstract class LocationTimeZoneProvider implements Dumpable { } } + /** For subclasses to invoke when needing to report a temporary failure. */ + final void handleTemporaryFailure(String reason) { + mThreadingDomain.assertCurrentThread(); + + synchronized (mSharedLock) { + ProviderState currentState = mCurrentState.get(); + switch (currentState.stateEnum) { + case PROVIDER_STATE_STARTED_INITIALIZING: + case PROVIDER_STATE_STARTED_UNCERTAIN: + case PROVIDER_STATE_STARTED_CERTAIN: { + // A temporary failure is treated as becoming uncertain. + String msg = "handleProviderLost reason=" + reason + + ", mProviderName=" + mProviderName + + ", currentState=" + currentState; + debugLog(msg); + // This is an unusual PROVIDER_STATE_STARTED_UNCERTAIN state because + // event == null + ProviderState newState = currentState.newState( + PROVIDER_STATE_STARTED_UNCERTAIN, null, + currentState.currentUserConfiguration, msg); + setCurrentState(newState, true); + cancelInitializationTimeoutIfSet(); + break; + } + case PROVIDER_STATE_STOPPED: { + debugLog("handleProviderLost reason=" + reason + + ", mProviderName=" + mProviderName + + ", currentState=" + currentState + + ": No state change required, provider is stopped."); + break; + } + case PROVIDER_STATE_PERM_FAILED: + case PROVIDER_STATE_DESTROYED: { + debugLog("handleProviderLost reason=" + reason + + ", mProviderName=" + mProviderName + + ", currentState=" + currentState + + ": No state change required, provider is terminated."); + break; + } + default: { + throw new IllegalStateException("Unknown currentState=" + currentState); + } + } + } + } + @GuardedBy("mSharedLock") private void assertIsStarted() { ProviderState currentState = mCurrentState.get(); @@ -751,6 +789,13 @@ abstract class LocationTimeZoneProvider implements Dumpable { } } + @GuardedBy("mSharedLock") + private void cancelInitializationTimeoutIfSet() { + if (mInitializationTimeoutQueue.hasQueued()) { + mInitializationTimeoutQueue.cancel(); + } + } + @VisibleForTesting Duration getInitializationTimeoutDelay() { synchronized (mSharedLock) {