From dd7fd6b8305511d35a1e45acc73609216b993e02 Mon Sep 17 00:00:00 2001 From: David Luhmer Date: Tue, 21 Jan 2020 18:43:43 +0100 Subject: [PATCH 1/9] add more debugging output / destroy api when reconnect fails --- .../android/sso/api/AidlNetworkRequest.java | 32 ++++++++++++++++--- .../android/sso/api/NetworkRequest.java | 6 +++- .../sso/helper/ExponentialBackoff.java | 12 +++++-- 3 files changed, 41 insertions(+), 9 deletions(-) diff --git a/src/main/java/com/nextcloud/android/sso/api/AidlNetworkRequest.java b/src/main/java/com/nextcloud/android/sso/api/AidlNetworkRequest.java index 3b83bb5d..5ee40126 100644 --- a/src/main/java/com/nextcloud/android/sso/api/AidlNetworkRequest.java +++ b/src/main/java/com/nextcloud/android/sso/api/AidlNetworkRequest.java @@ -51,7 +51,7 @@ public class AidlNetworkRequest extends NetworkRequest { */ private ServiceConnection mConnection = new ServiceConnection() { public void onServiceConnected(ComponentName className, IBinder service) { - Log.v(TAG, "Nextcloud Single sign-on: onServiceConnected [" + Thread.currentThread().getName() + "]"); + Log.v(TAG, "onServiceConnected [" + Thread.currentThread().getName() + "]"); mService = IInputStreamService.Stub.asInterface(service); mBound.set(true); @@ -62,13 +62,14 @@ public void onServiceConnected(ComponentName className, IBinder service) { } public void onServiceDisconnected(ComponentName className) { - Log.e(TAG, "Nextcloud Single sign-on: ServiceDisconnected"); + Log.e(TAG, "ServiceDisconnected [" +className.toString() + "]"); // This is called when the connection with the service has been // unexpectedly disconnected -- that is, its process crashed. mService = null; mBound.set(false); if (!mDestroyed) { + Log.d(TAG, "Reconnecting lost service connection"); connectApiWithBackoff(); } } @@ -115,6 +116,15 @@ public void stop() { } } + public void reconnect() { + if (mContext != null) { + mContext.unbindService(mConnection); + } else { + Log.e(TAG, "Context was null, cannot unbind nextcloud single sign-on service connection!"); + } + // the callback "onServiceDisconnected" will trigger a reconnect automatically. No need to do anything here.. + } + private void waitForApi() throws NextcloudApiNotRespondingException { synchronized (mBound) { // If service is not bound yet.. wait @@ -123,8 +133,18 @@ private void waitForApi() throws NextcloudApiNotRespondingException { try { mBound.wait(10000); // wait up to 10 seconds - // If api is still not bound after 10 seconds.. throw an exception + // If api is still not bound after 10 seconds.. try reconnecting if(!mBound.get()) { + /* + // try one reconnect + reconnect(); + + mBound.wait(10000); // wait up to 10 seconds + // If api is still not bound after 10 seconds.. throw an exception + if(!mBound.get()) { + throw new NextcloudApiNotRespondingException(); + } + */ throw new NextcloudApiNotRespondingException(); } } catch (InterruptedException ex) { @@ -143,8 +163,6 @@ private void waitForApi() throws NextcloudApiNotRespondingException { * @throws Exception or SSOException */ public Response performNetworkRequestV2(NextcloudRequest request, InputStream requestBodyInputStream) throws Exception { - - ParcelFileDescriptor output = performAidlNetworkRequestV2(request, requestBodyInputStream); InputStream os = new ParcelFileDescriptor.AutoCloseInputStream(output); ExceptionResponse response = deserializeObjectV2(os); @@ -204,6 +222,10 @@ private ParcelFileDescriptor performAidlNetworkRequest(NextcloudRequest request, throw new NetworkOnMainThreadException(); } + if(mDestroyed) { + throw new IllegalStateException("Nextcloud API already destroyed. Please report this issue."); + } + // Wait for api to be initialized waitForApi(); diff --git a/src/main/java/com/nextcloud/android/sso/api/NetworkRequest.java b/src/main/java/com/nextcloud/android/sso/api/NetworkRequest.java index f3d1197b..087ef072 100644 --- a/src/main/java/com/nextcloud/android/sso/api/NetworkRequest.java +++ b/src/main/java/com/nextcloud/android/sso/api/NetworkRequest.java @@ -5,6 +5,7 @@ import android.util.Log; import com.nextcloud.android.sso.aidl.NextcloudRequest; +import com.nextcloud.android.sso.exceptions.NextcloudApiNotRespondingException; import com.nextcloud.android.sso.helper.ExponentialBackoff; import com.nextcloud.android.sso.model.SingleSignOnAccount; @@ -41,8 +42,11 @@ protected void connect(String type) { protected abstract Response performNetworkRequestV2(NextcloudRequest request, InputStream requestBodyInputStream) throws Exception; protected void connectApiWithBackoff() { - new ExponentialBackoff(1000, 10000, 2, 5, Looper.getMainLooper(), () -> { + new ExponentialBackoff(1000, 5000, 2, 5, Looper.getMainLooper(), () -> { connect(mAccount.type); + }, () -> { + Log.e(TAG, "Unable to recover API"); + stop(); }).start(); } diff --git a/src/main/java/com/nextcloud/android/sso/helper/ExponentialBackoff.java b/src/main/java/com/nextcloud/android/sso/helper/ExponentialBackoff.java index a6454bc8..1743b19e 100644 --- a/src/main/java/com/nextcloud/android/sso/helper/ExponentialBackoff.java +++ b/src/main/java/com/nextcloud/android/sso/helper/ExponentialBackoff.java @@ -34,6 +34,7 @@ public class ExponentialBackoff { private int mMaxRetries; private int mMultiplier; private final Runnable mRunnable; + private final Runnable mFailedCallback; private final Handler mHandler; /** @@ -64,8 +65,9 @@ public ExponentialBackoff( int multiplier, int maxRetries, @NonNull Looper looper, - @NonNull Runnable runnable) { - this(initialDelayMs, maximumDelayMs, multiplier, maxRetries, new Handler(looper), runnable); + @NonNull Runnable runnable, + @NonNull Runnable onErrorRunnable) { + this(initialDelayMs, maximumDelayMs, multiplier, maxRetries, new Handler(looper), runnable, onErrorRunnable); } private ExponentialBackoff( @@ -74,7 +76,8 @@ private ExponentialBackoff( int multiplier, int maxRetries, @NonNull Handler handler, - @NonNull Runnable runnable) { + @NonNull Runnable runnable, + @NonNull Runnable onErrorRunnable) { mRetryCounter = 0; mStartDelayMs = initialDelayMs; mMaximumDelayMs = maximumDelayMs; @@ -82,6 +85,7 @@ private ExponentialBackoff( mMaxRetries = maxRetries; mHandler = handler; mRunnable = new WrapperRunnable(runnable); + mFailedCallback = onErrorRunnable; if(initialDelayMs <= 0) { throw new InvalidParameterException("initialDelayMs should not be less or equal to 0"); @@ -99,12 +103,14 @@ public void start() { public void stop() { mRetryCounter = 0; mHandlerAdapter.removeCallbacks(mRunnable); + } /** Should call when the retry action has failed and we want to retry after a longer delay. */ private void notifyFailed() { if(mRetryCounter > mMaxRetries) { stop(); + mFailedCallback.run(); } else { mRetryCounter++; long temp = Math.min( From f0a5a060f28486bb6f5fe54e6b10e7a295a725d5 Mon Sep 17 00:00:00 2001 From: David Luhmer Date: Tue, 18 Feb 2020 16:46:36 +0100 Subject: [PATCH 2/9] remove duplicate code --- .../com/nextcloud/android/sso/api/AidlNetworkRequest.java | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/main/java/com/nextcloud/android/sso/api/AidlNetworkRequest.java b/src/main/java/com/nextcloud/android/sso/api/AidlNetworkRequest.java index 5ee40126..27645e69 100644 --- a/src/main/java/com/nextcloud/android/sso/api/AidlNetworkRequest.java +++ b/src/main/java/com/nextcloud/android/sso/api/AidlNetworkRequest.java @@ -76,10 +76,6 @@ public void onServiceDisconnected(ComponentName className) { }; public void connect(String type) { - if (mDestroyed) { - throw new IllegalStateException("API already stopped"); - } - super.connect(type); String componentName = Constants.PACKAGE_NAME_PROD; From 62fc98205f68cdc71eb28c2bab0582bff699acef Mon Sep 17 00:00:00 2001 From: David Luhmer Date: Tue, 18 Feb 2020 18:25:52 +0100 Subject: [PATCH 3/9] unbind and re-bind service when service connection is lost / add more logging Signed-off-by: David Luhmer --- .../android/sso/api/AidlNetworkRequest.java | 69 ++++++++++--------- .../android/sso/api/NetworkRequest.java | 9 +-- .../sso/helper/ExponentialBackoff.java | 11 ++- 3 files changed, 50 insertions(+), 39 deletions(-) diff --git a/src/main/java/com/nextcloud/android/sso/api/AidlNetworkRequest.java b/src/main/java/com/nextcloud/android/sso/api/AidlNetworkRequest.java index 27645e69..38bb57cb 100644 --- a/src/main/java/com/nextcloud/android/sso/api/AidlNetworkRequest.java +++ b/src/main/java/com/nextcloud/android/sso/api/AidlNetworkRequest.java @@ -11,6 +11,8 @@ import android.os.RemoteException; import android.util.Log; +import androidx.annotation.NonNull; + import com.google.gson.Gson; import com.google.gson.internal.LinkedTreeMap; import com.nextcloud.android.sso.Constants; @@ -31,8 +33,6 @@ import java.util.ArrayList; import java.util.concurrent.atomic.AtomicBoolean; -import androidx.annotation.NonNull; - import static com.nextcloud.android.sso.exceptions.SSOException.parseNextcloudCustomException; public class AidlNetworkRequest extends NetworkRequest { @@ -51,7 +51,7 @@ public class AidlNetworkRequest extends NetworkRequest { */ private ServiceConnection mConnection = new ServiceConnection() { public void onServiceConnected(ComponentName className, IBinder service) { - Log.v(TAG, "onServiceConnected [" + Thread.currentThread().getName() + "]"); + Log.d(TAG, "[onServiceConnected] called from Thread: [" + Thread.currentThread().getName() + "] with IBinder [" + className.toString() + "]: " + service); mService = IInputStreamService.Stub.asInterface(service); mBound.set(true); @@ -62,20 +62,25 @@ public void onServiceConnected(ComponentName className, IBinder service) { } public void onServiceDisconnected(ComponentName className) { - Log.e(TAG, "ServiceDisconnected [" +className.toString() + "]"); + Log.w(TAG, "[onServiceDisconnected] [" + className.toString() + "]"); // This is called when the connection with the service has been - // unexpectedly disconnected -- that is, its process crashed. - mService = null; - mBound.set(false); + // unexpectedly disconnected -- that is, its process crashed or the service was + // terminated due to an update (e.g. google play store) if (!mDestroyed) { - Log.d(TAG, "Reconnecting lost service connection"); - connectApiWithBackoff(); + // In case we're currently not reconnecting + Log.d(TAG, "[onServiceDisconnected] Reconnecting lost service connection to component: [" + className.toString() + "]"); + reconnect(); + } else { + // API was destroyed on purpose + mService = null; + mBound.set(false); } } }; public void connect(String type) { + Log.d(TAG, "[connect] Binding to AccountManagerService for type [" + type + "]"); super.connect(type); String componentName = Constants.PACKAGE_NAME_PROD; @@ -83,42 +88,50 @@ public void connect(String type) { componentName = Constants.PACKAGE_NAME_DEV; } + Log.d(TAG, "[connect] Component name is: [" + componentName+ "]"); + try { Intent intentService = new Intent(); intentService.setComponent(new ComponentName(componentName, "com.owncloud.android.services.AccountManagerService")); - if (!mContext.bindService(intentService, mConnection, Context.BIND_AUTO_CREATE)) { - Log.d(TAG, "Binding to AccountManagerService returned false"); + // https://developer.android.com/reference/android/content/Context#BIND_ABOVE_CLIENT + if (!mContext.bindService(intentService, mConnection, Context.BIND_AUTO_CREATE | Context.BIND_ABOVE_CLIENT)) { + Log.d(TAG, "[connect] Binding to AccountManagerService returned false"); throw new IllegalStateException("Binding to AccountManagerService returned false"); + } else { + Log.d(TAG, "[connect] Bound to AccountManagerService successfully"); } } catch (SecurityException e) { - Log.e(TAG, "can't bind to AccountManagerService, check permission in Manifest"); + Log.e(TAG, "[connect] can't bind to AccountManagerService, check permission in Manifest"); mCallback.onError(e); } } + public void reconnect() { + Log.d(TAG, "[reconnect] called"); + unbindService(); + connectApiWithBackoff(); + } + public void stop() { super.stop(); + unbindService(); + mContext = null; + } + + private void unbindService() { // Unbind from the service if (mBound.get()) { if (mContext != null) { + Log.d(TAG, "[unbindService] Unbinding AccountManagerService"); mContext.unbindService(mConnection); } else { - Log.e(TAG, "Context was null, cannot unbind nextcloud single sign-on service connection!"); + Log.e(TAG, "[unbindService] Context was null, cannot unbind nextcloud single sign-on service connection!"); } mBound.set(false); - mContext = null; - } - } - - public void reconnect() { - if (mContext != null) { - mContext.unbindService(mConnection); - } else { - Log.e(TAG, "Context was null, cannot unbind nextcloud single sign-on service connection!"); + mService = null; } - // the callback "onServiceDisconnected" will trigger a reconnect automatically. No need to do anything here.. } private void waitForApi() throws NextcloudApiNotRespondingException { @@ -131,16 +144,6 @@ private void waitForApi() throws NextcloudApiNotRespondingException { // If api is still not bound after 10 seconds.. try reconnecting if(!mBound.get()) { - /* - // try one reconnect - reconnect(); - - mBound.wait(10000); // wait up to 10 seconds - // If api is still not bound after 10 seconds.. throw an exception - if(!mBound.get()) { - throw new NextcloudApiNotRespondingException(); - } - */ throw new NextcloudApiNotRespondingException(); } } catch (InterruptedException ex) { diff --git a/src/main/java/com/nextcloud/android/sso/api/NetworkRequest.java b/src/main/java/com/nextcloud/android/sso/api/NetworkRequest.java index 087ef072..4781f4ff 100644 --- a/src/main/java/com/nextcloud/android/sso/api/NetworkRequest.java +++ b/src/main/java/com/nextcloud/android/sso/api/NetworkRequest.java @@ -4,15 +4,14 @@ import android.os.Looper; import android.util.Log; +import androidx.annotation.NonNull; + import com.nextcloud.android.sso.aidl.NextcloudRequest; -import com.nextcloud.android.sso.exceptions.NextcloudApiNotRespondingException; import com.nextcloud.android.sso.helper.ExponentialBackoff; import com.nextcloud.android.sso.model.SingleSignOnAccount; import java.io.InputStream; -import androidx.annotation.NonNull; - public abstract class NetworkRequest { private static final String TAG = NetworkRequest.class.getCanonicalName(); @@ -31,7 +30,7 @@ protected NetworkRequest(@NonNull Context context, @NonNull SingleSignOnAccount protected void connect(String type) { - Log.v(TAG, "Nextcloud Single sign-on connect() called [" + Thread.currentThread().getName() + "] Account-Type: [" + type + "]"); + Log.d(TAG, "[connect] connect() called [" + Thread.currentThread().getName() + "] Account-Type: [" + type + "]"); if (mDestroyed) { throw new IllegalStateException("API already destroyed! You cannot reuse a stopped API instance"); } @@ -42,7 +41,9 @@ protected void connect(String type) { protected abstract Response performNetworkRequestV2(NextcloudRequest request, InputStream requestBodyInputStream) throws Exception; protected void connectApiWithBackoff() { + Log.d(TAG, "[connectApiWithBackoff] connectApiWithBackoff() called from Thread: [" + Thread.currentThread().getName() + "]"); new ExponentialBackoff(1000, 5000, 2, 5, Looper.getMainLooper(), () -> { + Log.d(TAG, "[connectApiWithBackoff] trying to connect.."); connect(mAccount.type); }, () -> { Log.e(TAG, "Unable to recover API"); diff --git a/src/main/java/com/nextcloud/android/sso/helper/ExponentialBackoff.java b/src/main/java/com/nextcloud/android/sso/helper/ExponentialBackoff.java index 1743b19e..e8b86bc3 100644 --- a/src/main/java/com/nextcloud/android/sso/helper/ExponentialBackoff.java +++ b/src/main/java/com/nextcloud/android/sso/helper/ExponentialBackoff.java @@ -18,6 +18,8 @@ import android.os.Handler; import android.os.Looper; +import android.util.Log; + import androidx.annotation.NonNull; import androidx.annotation.VisibleForTesting; @@ -27,6 +29,8 @@ /** The implementation of exponential backoff with jitter applied. */ public class ExponentialBackoff { + private static final String TAG = ExponentialBackoff.class.getCanonicalName(); + private int mRetryCounter; private long mStartDelayMs; private long mMaximumDelayMs; @@ -107,8 +111,10 @@ public void stop() { } /** Should call when the retry action has failed and we want to retry after a longer delay. */ - private void notifyFailed() { + private void notifyFailed(Exception ex) { + Log.d(TAG, "[notifyFailed] Error: [" + ex.getMessage() + "]"); if(mRetryCounter > mMaxRetries) { + Log.d(TAG, "[notifyFailed] Retries exceeded, ending now"); stop(); mFailedCallback.run(); } else { @@ -116,6 +122,7 @@ private void notifyFailed() { long temp = Math.min( mMaximumDelayMs, (long) (mStartDelayMs * Math.pow(mMultiplier, mRetryCounter))); mCurrentDelayMs = (long) (((1 + Math.random()) / 2) * temp); + Log.d(TAG, "[notifyFailed] retrying in: [" + mCurrentDelayMs + "ms]"); mHandlerAdapter.removeCallbacks(mRunnable); mHandlerAdapter.postDelayed(mRunnable, mCurrentDelayMs); } @@ -134,7 +141,7 @@ public void run() { try { runnable.run(); } catch (Exception ex) { - notifyFailed(); + notifyFailed(ex); } } } From 5652e31dfe667473dbe754c3a7e4adff8ab77727 Mon Sep 17 00:00:00 2001 From: David Luhmer Date: Tue, 18 Feb 2020 18:29:10 +0100 Subject: [PATCH 4/9] fix typo --- .../java/com/nextcloud/android/sso/api/AidlNetworkRequest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/com/nextcloud/android/sso/api/AidlNetworkRequest.java b/src/main/java/com/nextcloud/android/sso/api/AidlNetworkRequest.java index 38bb57cb..868775b8 100644 --- a/src/main/java/com/nextcloud/android/sso/api/AidlNetworkRequest.java +++ b/src/main/java/com/nextcloud/android/sso/api/AidlNetworkRequest.java @@ -142,7 +142,7 @@ private void waitForApi() throws NextcloudApiNotRespondingException { try { mBound.wait(10000); // wait up to 10 seconds - // If api is still not bound after 10 seconds.. try reconnecting + // If api is still not bound after 10 seconds.. throw an exception if(!mBound.get()) { throw new NextcloudApiNotRespondingException(); } From 1302ea66b65e436503d7aad59873c712acb4b1ca Mon Sep 17 00:00:00 2001 From: stefan-niedermann Date: Sun, 12 Jan 2020 11:37:05 +0100 Subject: [PATCH 5/9] VersionCheckHelper::getNextcloudFilesVersionCode only needs Context Signed-off-by: David Luhmer --- .../com/nextcloud/android/sso/helper/VersionCheckHelper.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/main/java/com/nextcloud/android/sso/helper/VersionCheckHelper.java b/src/main/java/com/nextcloud/android/sso/helper/VersionCheckHelper.java index fb78c5ea..d2dba609 100644 --- a/src/main/java/com/nextcloud/android/sso/helper/VersionCheckHelper.java +++ b/src/main/java/com/nextcloud/android/sso/helper/VersionCheckHelper.java @@ -32,8 +32,8 @@ public static boolean verifyMinVersion(Activity activity, int minVersion) { return true; } - public static int getNextcloudFilesVersionCode(Activity activity) throws PackageManager.NameNotFoundException { - PackageInfo pInfo = activity.getPackageManager().getPackageInfo("com.nextcloud.client", 0); + public static int getNextcloudFilesVersionCode(Context context) throws PackageManager.NameNotFoundException { + PackageInfo pInfo = context.getPackageManager().getPackageInfo("com.nextcloud.client", 0); int verCode = pInfo.versionCode; Log.e("VersionCheckHelper", "Version Code: " + verCode); return verCode; From 94bc955c612392fed6f531649d756516bf3a3313 Mon Sep 17 00:00:00 2001 From: Niedermann IT-Dienstleistungen Date: Sun, 19 Jan 2020 18:51:06 +0100 Subject: [PATCH 6/9] Add Context import statement Signed-off-by: David Luhmer --- .../com/nextcloud/android/sso/helper/VersionCheckHelper.java | 1 + 1 file changed, 1 insertion(+) diff --git a/src/main/java/com/nextcloud/android/sso/helper/VersionCheckHelper.java b/src/main/java/com/nextcloud/android/sso/helper/VersionCheckHelper.java index d2dba609..dccc7537 100644 --- a/src/main/java/com/nextcloud/android/sso/helper/VersionCheckHelper.java +++ b/src/main/java/com/nextcloud/android/sso/helper/VersionCheckHelper.java @@ -1,6 +1,7 @@ package com.nextcloud.android.sso.helper; import android.app.Activity; +import android.content.Context; import android.content.pm.PackageInfo; import android.content.pm.PackageManager; import android.util.Log; From 84e778aff9deed2b17854ed23d359cf4596471fb Mon Sep 17 00:00:00 2001 From: David Luhmer Date: Tue, 21 Jan 2020 18:43:43 +0100 Subject: [PATCH 7/9] add more debugging output / destroy api when reconnect fails Signed-off-by: David Luhmer --- .../android/sso/api/AidlNetworkRequest.java | 32 ++++++++++++++++--- .../android/sso/api/NetworkRequest.java | 6 +++- .../sso/helper/ExponentialBackoff.java | 12 +++++-- 3 files changed, 41 insertions(+), 9 deletions(-) diff --git a/src/main/java/com/nextcloud/android/sso/api/AidlNetworkRequest.java b/src/main/java/com/nextcloud/android/sso/api/AidlNetworkRequest.java index 3b83bb5d..5ee40126 100644 --- a/src/main/java/com/nextcloud/android/sso/api/AidlNetworkRequest.java +++ b/src/main/java/com/nextcloud/android/sso/api/AidlNetworkRequest.java @@ -51,7 +51,7 @@ public class AidlNetworkRequest extends NetworkRequest { */ private ServiceConnection mConnection = new ServiceConnection() { public void onServiceConnected(ComponentName className, IBinder service) { - Log.v(TAG, "Nextcloud Single sign-on: onServiceConnected [" + Thread.currentThread().getName() + "]"); + Log.v(TAG, "onServiceConnected [" + Thread.currentThread().getName() + "]"); mService = IInputStreamService.Stub.asInterface(service); mBound.set(true); @@ -62,13 +62,14 @@ public void onServiceConnected(ComponentName className, IBinder service) { } public void onServiceDisconnected(ComponentName className) { - Log.e(TAG, "Nextcloud Single sign-on: ServiceDisconnected"); + Log.e(TAG, "ServiceDisconnected [" +className.toString() + "]"); // This is called when the connection with the service has been // unexpectedly disconnected -- that is, its process crashed. mService = null; mBound.set(false); if (!mDestroyed) { + Log.d(TAG, "Reconnecting lost service connection"); connectApiWithBackoff(); } } @@ -115,6 +116,15 @@ public void stop() { } } + public void reconnect() { + if (mContext != null) { + mContext.unbindService(mConnection); + } else { + Log.e(TAG, "Context was null, cannot unbind nextcloud single sign-on service connection!"); + } + // the callback "onServiceDisconnected" will trigger a reconnect automatically. No need to do anything here.. + } + private void waitForApi() throws NextcloudApiNotRespondingException { synchronized (mBound) { // If service is not bound yet.. wait @@ -123,8 +133,18 @@ private void waitForApi() throws NextcloudApiNotRespondingException { try { mBound.wait(10000); // wait up to 10 seconds - // If api is still not bound after 10 seconds.. throw an exception + // If api is still not bound after 10 seconds.. try reconnecting if(!mBound.get()) { + /* + // try one reconnect + reconnect(); + + mBound.wait(10000); // wait up to 10 seconds + // If api is still not bound after 10 seconds.. throw an exception + if(!mBound.get()) { + throw new NextcloudApiNotRespondingException(); + } + */ throw new NextcloudApiNotRespondingException(); } } catch (InterruptedException ex) { @@ -143,8 +163,6 @@ private void waitForApi() throws NextcloudApiNotRespondingException { * @throws Exception or SSOException */ public Response performNetworkRequestV2(NextcloudRequest request, InputStream requestBodyInputStream) throws Exception { - - ParcelFileDescriptor output = performAidlNetworkRequestV2(request, requestBodyInputStream); InputStream os = new ParcelFileDescriptor.AutoCloseInputStream(output); ExceptionResponse response = deserializeObjectV2(os); @@ -204,6 +222,10 @@ private ParcelFileDescriptor performAidlNetworkRequest(NextcloudRequest request, throw new NetworkOnMainThreadException(); } + if(mDestroyed) { + throw new IllegalStateException("Nextcloud API already destroyed. Please report this issue."); + } + // Wait for api to be initialized waitForApi(); diff --git a/src/main/java/com/nextcloud/android/sso/api/NetworkRequest.java b/src/main/java/com/nextcloud/android/sso/api/NetworkRequest.java index f3d1197b..087ef072 100644 --- a/src/main/java/com/nextcloud/android/sso/api/NetworkRequest.java +++ b/src/main/java/com/nextcloud/android/sso/api/NetworkRequest.java @@ -5,6 +5,7 @@ import android.util.Log; import com.nextcloud.android.sso.aidl.NextcloudRequest; +import com.nextcloud.android.sso.exceptions.NextcloudApiNotRespondingException; import com.nextcloud.android.sso.helper.ExponentialBackoff; import com.nextcloud.android.sso.model.SingleSignOnAccount; @@ -41,8 +42,11 @@ protected void connect(String type) { protected abstract Response performNetworkRequestV2(NextcloudRequest request, InputStream requestBodyInputStream) throws Exception; protected void connectApiWithBackoff() { - new ExponentialBackoff(1000, 10000, 2, 5, Looper.getMainLooper(), () -> { + new ExponentialBackoff(1000, 5000, 2, 5, Looper.getMainLooper(), () -> { connect(mAccount.type); + }, () -> { + Log.e(TAG, "Unable to recover API"); + stop(); }).start(); } diff --git a/src/main/java/com/nextcloud/android/sso/helper/ExponentialBackoff.java b/src/main/java/com/nextcloud/android/sso/helper/ExponentialBackoff.java index a6454bc8..1743b19e 100644 --- a/src/main/java/com/nextcloud/android/sso/helper/ExponentialBackoff.java +++ b/src/main/java/com/nextcloud/android/sso/helper/ExponentialBackoff.java @@ -34,6 +34,7 @@ public class ExponentialBackoff { private int mMaxRetries; private int mMultiplier; private final Runnable mRunnable; + private final Runnable mFailedCallback; private final Handler mHandler; /** @@ -64,8 +65,9 @@ public ExponentialBackoff( int multiplier, int maxRetries, @NonNull Looper looper, - @NonNull Runnable runnable) { - this(initialDelayMs, maximumDelayMs, multiplier, maxRetries, new Handler(looper), runnable); + @NonNull Runnable runnable, + @NonNull Runnable onErrorRunnable) { + this(initialDelayMs, maximumDelayMs, multiplier, maxRetries, new Handler(looper), runnable, onErrorRunnable); } private ExponentialBackoff( @@ -74,7 +76,8 @@ private ExponentialBackoff( int multiplier, int maxRetries, @NonNull Handler handler, - @NonNull Runnable runnable) { + @NonNull Runnable runnable, + @NonNull Runnable onErrorRunnable) { mRetryCounter = 0; mStartDelayMs = initialDelayMs; mMaximumDelayMs = maximumDelayMs; @@ -82,6 +85,7 @@ private ExponentialBackoff( mMaxRetries = maxRetries; mHandler = handler; mRunnable = new WrapperRunnable(runnable); + mFailedCallback = onErrorRunnable; if(initialDelayMs <= 0) { throw new InvalidParameterException("initialDelayMs should not be less or equal to 0"); @@ -99,12 +103,14 @@ public void start() { public void stop() { mRetryCounter = 0; mHandlerAdapter.removeCallbacks(mRunnable); + } /** Should call when the retry action has failed and we want to retry after a longer delay. */ private void notifyFailed() { if(mRetryCounter > mMaxRetries) { stop(); + mFailedCallback.run(); } else { mRetryCounter++; long temp = Math.min( From 985e3320ca0788733ef1f264f8100f2b5e374e99 Mon Sep 17 00:00:00 2001 From: David Luhmer Date: Tue, 18 Feb 2020 16:46:36 +0100 Subject: [PATCH 8/9] remove duplicate code Signed-off-by: David Luhmer --- .../com/nextcloud/android/sso/api/AidlNetworkRequest.java | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/main/java/com/nextcloud/android/sso/api/AidlNetworkRequest.java b/src/main/java/com/nextcloud/android/sso/api/AidlNetworkRequest.java index 5ee40126..27645e69 100644 --- a/src/main/java/com/nextcloud/android/sso/api/AidlNetworkRequest.java +++ b/src/main/java/com/nextcloud/android/sso/api/AidlNetworkRequest.java @@ -76,10 +76,6 @@ public void onServiceDisconnected(ComponentName className) { }; public void connect(String type) { - if (mDestroyed) { - throw new IllegalStateException("API already stopped"); - } - super.connect(type); String componentName = Constants.PACKAGE_NAME_PROD; From 5a7d933be1c96cb321668963b3b736a9be86f98e Mon Sep 17 00:00:00 2001 From: David Luhmer Date: Tue, 18 Feb 2020 18:25:52 +0100 Subject: [PATCH 9/9] unbind and re-bind service when service connection is lost / add more logging Signed-off-by: David Luhmer --- .../android/sso/api/AidlNetworkRequest.java | 69 ++++++++++--------- .../android/sso/api/NetworkRequest.java | 9 +-- .../sso/helper/ExponentialBackoff.java | 11 ++- 3 files changed, 50 insertions(+), 39 deletions(-) diff --git a/src/main/java/com/nextcloud/android/sso/api/AidlNetworkRequest.java b/src/main/java/com/nextcloud/android/sso/api/AidlNetworkRequest.java index 27645e69..38bb57cb 100644 --- a/src/main/java/com/nextcloud/android/sso/api/AidlNetworkRequest.java +++ b/src/main/java/com/nextcloud/android/sso/api/AidlNetworkRequest.java @@ -11,6 +11,8 @@ import android.os.RemoteException; import android.util.Log; +import androidx.annotation.NonNull; + import com.google.gson.Gson; import com.google.gson.internal.LinkedTreeMap; import com.nextcloud.android.sso.Constants; @@ -31,8 +33,6 @@ import java.util.ArrayList; import java.util.concurrent.atomic.AtomicBoolean; -import androidx.annotation.NonNull; - import static com.nextcloud.android.sso.exceptions.SSOException.parseNextcloudCustomException; public class AidlNetworkRequest extends NetworkRequest { @@ -51,7 +51,7 @@ public class AidlNetworkRequest extends NetworkRequest { */ private ServiceConnection mConnection = new ServiceConnection() { public void onServiceConnected(ComponentName className, IBinder service) { - Log.v(TAG, "onServiceConnected [" + Thread.currentThread().getName() + "]"); + Log.d(TAG, "[onServiceConnected] called from Thread: [" + Thread.currentThread().getName() + "] with IBinder [" + className.toString() + "]: " + service); mService = IInputStreamService.Stub.asInterface(service); mBound.set(true); @@ -62,20 +62,25 @@ public void onServiceConnected(ComponentName className, IBinder service) { } public void onServiceDisconnected(ComponentName className) { - Log.e(TAG, "ServiceDisconnected [" +className.toString() + "]"); + Log.w(TAG, "[onServiceDisconnected] [" + className.toString() + "]"); // This is called when the connection with the service has been - // unexpectedly disconnected -- that is, its process crashed. - mService = null; - mBound.set(false); + // unexpectedly disconnected -- that is, its process crashed or the service was + // terminated due to an update (e.g. google play store) if (!mDestroyed) { - Log.d(TAG, "Reconnecting lost service connection"); - connectApiWithBackoff(); + // In case we're currently not reconnecting + Log.d(TAG, "[onServiceDisconnected] Reconnecting lost service connection to component: [" + className.toString() + "]"); + reconnect(); + } else { + // API was destroyed on purpose + mService = null; + mBound.set(false); } } }; public void connect(String type) { + Log.d(TAG, "[connect] Binding to AccountManagerService for type [" + type + "]"); super.connect(type); String componentName = Constants.PACKAGE_NAME_PROD; @@ -83,42 +88,50 @@ public void connect(String type) { componentName = Constants.PACKAGE_NAME_DEV; } + Log.d(TAG, "[connect] Component name is: [" + componentName+ "]"); + try { Intent intentService = new Intent(); intentService.setComponent(new ComponentName(componentName, "com.owncloud.android.services.AccountManagerService")); - if (!mContext.bindService(intentService, mConnection, Context.BIND_AUTO_CREATE)) { - Log.d(TAG, "Binding to AccountManagerService returned false"); + // https://developer.android.com/reference/android/content/Context#BIND_ABOVE_CLIENT + if (!mContext.bindService(intentService, mConnection, Context.BIND_AUTO_CREATE | Context.BIND_ABOVE_CLIENT)) { + Log.d(TAG, "[connect] Binding to AccountManagerService returned false"); throw new IllegalStateException("Binding to AccountManagerService returned false"); + } else { + Log.d(TAG, "[connect] Bound to AccountManagerService successfully"); } } catch (SecurityException e) { - Log.e(TAG, "can't bind to AccountManagerService, check permission in Manifest"); + Log.e(TAG, "[connect] can't bind to AccountManagerService, check permission in Manifest"); mCallback.onError(e); } } + public void reconnect() { + Log.d(TAG, "[reconnect] called"); + unbindService(); + connectApiWithBackoff(); + } + public void stop() { super.stop(); + unbindService(); + mContext = null; + } + + private void unbindService() { // Unbind from the service if (mBound.get()) { if (mContext != null) { + Log.d(TAG, "[unbindService] Unbinding AccountManagerService"); mContext.unbindService(mConnection); } else { - Log.e(TAG, "Context was null, cannot unbind nextcloud single sign-on service connection!"); + Log.e(TAG, "[unbindService] Context was null, cannot unbind nextcloud single sign-on service connection!"); } mBound.set(false); - mContext = null; - } - } - - public void reconnect() { - if (mContext != null) { - mContext.unbindService(mConnection); - } else { - Log.e(TAG, "Context was null, cannot unbind nextcloud single sign-on service connection!"); + mService = null; } - // the callback "onServiceDisconnected" will trigger a reconnect automatically. No need to do anything here.. } private void waitForApi() throws NextcloudApiNotRespondingException { @@ -131,16 +144,6 @@ private void waitForApi() throws NextcloudApiNotRespondingException { // If api is still not bound after 10 seconds.. try reconnecting if(!mBound.get()) { - /* - // try one reconnect - reconnect(); - - mBound.wait(10000); // wait up to 10 seconds - // If api is still not bound after 10 seconds.. throw an exception - if(!mBound.get()) { - throw new NextcloudApiNotRespondingException(); - } - */ throw new NextcloudApiNotRespondingException(); } } catch (InterruptedException ex) { diff --git a/src/main/java/com/nextcloud/android/sso/api/NetworkRequest.java b/src/main/java/com/nextcloud/android/sso/api/NetworkRequest.java index 087ef072..4781f4ff 100644 --- a/src/main/java/com/nextcloud/android/sso/api/NetworkRequest.java +++ b/src/main/java/com/nextcloud/android/sso/api/NetworkRequest.java @@ -4,15 +4,14 @@ import android.os.Looper; import android.util.Log; +import androidx.annotation.NonNull; + import com.nextcloud.android.sso.aidl.NextcloudRequest; -import com.nextcloud.android.sso.exceptions.NextcloudApiNotRespondingException; import com.nextcloud.android.sso.helper.ExponentialBackoff; import com.nextcloud.android.sso.model.SingleSignOnAccount; import java.io.InputStream; -import androidx.annotation.NonNull; - public abstract class NetworkRequest { private static final String TAG = NetworkRequest.class.getCanonicalName(); @@ -31,7 +30,7 @@ protected NetworkRequest(@NonNull Context context, @NonNull SingleSignOnAccount protected void connect(String type) { - Log.v(TAG, "Nextcloud Single sign-on connect() called [" + Thread.currentThread().getName() + "] Account-Type: [" + type + "]"); + Log.d(TAG, "[connect] connect() called [" + Thread.currentThread().getName() + "] Account-Type: [" + type + "]"); if (mDestroyed) { throw new IllegalStateException("API already destroyed! You cannot reuse a stopped API instance"); } @@ -42,7 +41,9 @@ protected void connect(String type) { protected abstract Response performNetworkRequestV2(NextcloudRequest request, InputStream requestBodyInputStream) throws Exception; protected void connectApiWithBackoff() { + Log.d(TAG, "[connectApiWithBackoff] connectApiWithBackoff() called from Thread: [" + Thread.currentThread().getName() + "]"); new ExponentialBackoff(1000, 5000, 2, 5, Looper.getMainLooper(), () -> { + Log.d(TAG, "[connectApiWithBackoff] trying to connect.."); connect(mAccount.type); }, () -> { Log.e(TAG, "Unable to recover API"); diff --git a/src/main/java/com/nextcloud/android/sso/helper/ExponentialBackoff.java b/src/main/java/com/nextcloud/android/sso/helper/ExponentialBackoff.java index 1743b19e..e8b86bc3 100644 --- a/src/main/java/com/nextcloud/android/sso/helper/ExponentialBackoff.java +++ b/src/main/java/com/nextcloud/android/sso/helper/ExponentialBackoff.java @@ -18,6 +18,8 @@ import android.os.Handler; import android.os.Looper; +import android.util.Log; + import androidx.annotation.NonNull; import androidx.annotation.VisibleForTesting; @@ -27,6 +29,8 @@ /** The implementation of exponential backoff with jitter applied. */ public class ExponentialBackoff { + private static final String TAG = ExponentialBackoff.class.getCanonicalName(); + private int mRetryCounter; private long mStartDelayMs; private long mMaximumDelayMs; @@ -107,8 +111,10 @@ public void stop() { } /** Should call when the retry action has failed and we want to retry after a longer delay. */ - private void notifyFailed() { + private void notifyFailed(Exception ex) { + Log.d(TAG, "[notifyFailed] Error: [" + ex.getMessage() + "]"); if(mRetryCounter > mMaxRetries) { + Log.d(TAG, "[notifyFailed] Retries exceeded, ending now"); stop(); mFailedCallback.run(); } else { @@ -116,6 +122,7 @@ private void notifyFailed() { long temp = Math.min( mMaximumDelayMs, (long) (mStartDelayMs * Math.pow(mMultiplier, mRetryCounter))); mCurrentDelayMs = (long) (((1 + Math.random()) / 2) * temp); + Log.d(TAG, "[notifyFailed] retrying in: [" + mCurrentDelayMs + "ms]"); mHandlerAdapter.removeCallbacks(mRunnable); mHandlerAdapter.postDelayed(mRunnable, mCurrentDelayMs); } @@ -134,7 +141,7 @@ public void run() { try { runnable.run(); } catch (Exception ex) { - notifyFailed(); + notifyFailed(ex); } } }