feat(SDK-520): replace magic 1000ms timeouts with named, configurable…#882
feat(SDK-520): replace magic 1000ms timeouts with named, configurable…#882jferrao-itrbl wants to merge 1 commit into
Conversation
… constants - Add IterableConfig.androidWakeDelayMs and authCallbackTimeoutMs to expose the two hardcoded timeouts as tunable, documented values. - Replace the fixed-window auth callback gate with an event-driven Promise latch; the timer survives only as a safety-net fallback.
|
|
Qlty doesn't post analysis or coverage comments for contributors without a seat. An authorized user can grant @jferrao-itrbl a seat from this pull request's page in Qlty. |
| Promise.race<AuthLatchResult>([nativeLatch, timeoutLatch]).then( | ||
| (result) => { | ||
| // Clear the resolver so a late native event after the timeout | ||
| // is a no-op. | ||
| authLatchResolver = null; | ||
| pendingAuthResult = null; | ||
| if (result === IterableAuthResponseResult.SUCCESS) { | ||
| promiseResult.successCallback?.(); | ||
| } else if (result === IterableAuthResponseResult.FAILURE) { | ||
| // We are currently only reporting JWT related errors. In | ||
| // the future, we should handle other types of errors as | ||
| // well. | ||
| promiseResult.failureCallback?.(); | ||
| } else { | ||
| IterableLogger?.log('No callback received from native layer'); | ||
| } | ||
| } else { | ||
| IterableLogger?.log('No callback received from native layer'); | ||
| } | ||
| }, 1000); | ||
| ); |
There was a problem hiding this comment.
authLatchResolver and pendingAuthResult are shared across every handleAuthCalled, so overlapping auth invocations can wipe each other's state.
Concrete failure:
- Auth # 1:
handleAuthCalledresets,authHandlerresolves, latch # 1 created,authLatchResolver = resolve1, race # 1 starts. - Auth # 2:
handleAuthCalledfires while # 1 is still racing. Reset nullsauthLatchResolver,authHandler# 2 resolves, latch # 2 created,authLatchResolver = resolve2. - Race # 1's safety-net timer fires, resolves race # 1 with
NO_CALLBACK. The.thenhandler runsauthLatchResolver = nullandpendingAuthResult = null, wiping resolve2 and any buffered result belonging to # 2. - Native
handleAuthSuccessCalledfor either invocation now has nowhere to land. Native completes the request, JS silently drops thesuccessCallback/failureCallback.
In production this looks like JWT refresh flows that succeed on the server but the app never learns about it.
Please scope this state per invocation, e.g. capture the resolver/buffer inside a per-call object and only null out that object's fields in its own .then. A test that fires two handleAuthCalled events back to back and asserts both successCallbacks are invoked would lock this down.
| * auth success/failure event arrives. Overridable via | ||
| * {@link IterableConfig.authCallbackTimeoutMs}. | ||
| */ | ||
| const AUTH_CALLBACK_TIMEOUT_DEFAULT_MS = 1000; |
There was a problem hiding this comment.
Default of 1000ms defeats the "safety net" framing.
Native success/failure only fires after passAlongAuthToken and a full HTTP round-trip against Iterable, which routinely exceeds 1s on real mobile networks. With this default, the timer wins the race first, the resolver-clearing branch runs, and the late native event ends up hitting the null-resolver path so successCallback / failureCallback gets dropped.
Native waits up to 30s for us on both platforms (CountDownLatch.await(30, TimeUnit.SECONDS) on Android, authHandlerSemaphore.wait(timeout: 30.0) on iOS), so JS closing the loop at 1s is asymmetric.
My suggestion would be bumping the default to 5000-10000ms so the timer is genuinely a fallback for stuck native, not a race the timer usually wins.
2 participants
🔹 JIRA Ticket(s) if any
✏️ Description
Replace hardcoded magic timeouts: