fix : Guard against all uncaught exception in the serial executor block#970
fix : Guard against all uncaught exception in the serial executor block#970pmathew92 wants to merge 4 commits into
Conversation
| val expiresAt = storage.retrieveLong(KEY_EXPIRES_AT) | ||
| val storedScope = storage.retrieveString(KEY_SCOPE) | ||
| val hasEmptyCredentials = | ||
| TextUtils.isEmpty(accessToken) && TextUtils.isEmpty(idToken) || expiresAt == null |
| val willAccessTokenExpire = willExpire(expiresAt, minTtl.toLong()) | ||
| if (willAccessTokenExpire) { | ||
| val tokenLifetime = | ||
| (expiresAt - currentTimeInMillis - minTtl * 1000) / -1000 |
| val willAccessTokenExpire = willExpire(expiresAt, minTtl.toLong()) | ||
| if (willAccessTokenExpire) { | ||
| val tokenLifetime = | ||
| (expiresAt - currentTimeInMillis - minTtl * 1000) / -1000 |
| val willAccessTokenExpire = willExpire(expiresAt, minTtl.toLong()) | ||
| if (willAccessTokenExpire) { | ||
| val tokenLifetime = | ||
| (expiresAt - currentTimeInMillis - minTtl * 1000) / -1000 |
| val willAccessTokenExpire = willExpire(expiresAt, minTtl.toLong()) | ||
| if (willAccessTokenExpire) { | ||
| val tokenLifetime = | ||
| (expiresAt - currentTimeInMillis - minTtl * 1000) / -1000 |
| ) { | ||
| try { | ||
| block() | ||
| } catch (t: Throwable) { |
There was a problem hiding this comment.
atching Throwable here means we'd also swallow OutOfMemoryError, StackOverflowError, etc. — situations where the JVM is in a bad state and the app really should crash. The old code only caught RuntimeException. Could we narrow this to Exception instead? Or re-throw Error subtypes so fatal problems don't get silently eaten:
} catch (e: Exception) {
callback.onFailure(
CredentialsManagerException(CredentialsManagerException.Code.UNKNOWN_ERROR, e)
)
There was a problem hiding this comment.
Good point on catching too broadly. However, I'm intentionally keeping Throwable here — this is an SDK running on a background executor, and the entire purpose of this fix (#968) is to prevent unhandled exceptions from silently killing the host app. The rationale is that a library should never be the reason an app force-closes without explanation — the host app should own its crash policy.
| } catch (t: Throwable) { | ||
| callback.onFailure( | ||
| CredentialsManagerException(CredentialsManagerException.Code.UNKNOWN_ERROR, t) | ||
| ) |
There was a problem hiding this comment.
can we add log
line here? Something like:
} catch (t: Throwable) {
Log.e("BaseCredentialsManager", "Unexpected error in executor block", t)
callback.onFailure(...)
}
will be helpful if a customer reports a weird failure
There was a problem hiding this comment.
Good point. Added log to record the error
Changes
callback.onFailure(UNKNOWN_ERROR)
Problem
Closes #968
Test plan
Testing
Please describe how this can be tested by reviewers. Be specific about anything not tested and reasons why. Since this library has unit testing, tests should be added for new functionality and existing tests should complete without errors.
This change adds unit test coverage
This change adds integration test coverage
This change has been tested on the latest version of the platform/language or why not
Checklist
I have read the Auth0 general contribution guidelines
I have read the Auth0 Code of Conduct
All existing and new tests complete without errors