Skip to content

Commit 00d3380

Browse files
jkasten2jkasten2
committed
fix: remove throwing on init timeout
Starting in 5.4.0 calls such as OneSignal.Notification or OneSignal.Location would throw if they waited on the SDK into to long. This comment changes this to wait forever until init is complete, and log instead if the time is taking longer than expected. If these calls were done from the main thread an ANR is the better of two evils and the app can recover, where an uncaught throw it can not. This commit will bring the behavior the similar behavior in 5.1.x. The only difference is you will now never see an ANR with initWithContext, however if you call other parts of the SDK from the main thread, such as OneSignal.Notification, you will continue to see ANRs, just at a different stacktrace. To avoid these ANRs completely call all other SDK methods outside of the main thread.
1 parent 9d81deb commit 00d3380

File tree

4 files changed

+43
-96
lines changed

4 files changed

+43
-96
lines changed

‎OneSignalSDK/onesignal/core/src/main/java/com/onesignal/common/threading/CompletionAwaiter.kt‎

Lines changed: 15 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -57,27 +57,26 @@ class CompletionAwaiter(
5757
}
5858

5959
/**
60-
* Wait for completion using blocking approach with an optional timeout.
60+
* Wait for completion using blocking approach
6161
*
62-
* @param timeoutMs Timeout in milliseconds, defaults to context-appropriate timeout
63-
* @return true if completed before timeout, false otherwise.
62+
* @param timeoutToLog Timeout in milliseconds to log a message if not
63+
* completed in time. Does NOT have any effect on logic.
6464
*/
65-
fun await(timeoutMs: Long = getDefaultTimeout()): Boolean {
66-
val completed =
65+
fun awaitAndLogIfOverTimeout(timeoutToLog: Long = getDefaultTimeout()) {
66+
logIfOverTimeout(timeoutToLog)
67+
latch.await()
68+
}
69+
70+
private fun logIfOverTimeout(timeoutMs: Long) {
71+
launchOnDefault {
6772
try {
6873
latch.await(timeoutMs, TimeUnit.MILLISECONDS)
6974
} catch (e: InterruptedException) {
70-
Logging.warn("Interrupted while waiting for $componentName", e)
75+
Logging.warn("$componentName is taking longer that normal!", e)
7176
logAllThreads()
72-
false
77+
Logging.warn(createTimeoutMessage(timeoutMs))
7378
}
74-
75-
if (!completed) {
76-
val message = createTimeoutMessage(timeoutMs)
77-
Logging.warn(message)
7879
}
79-
80-
return completed
8180
}
8281

8382
/**
@@ -88,18 +87,15 @@ class CompletionAwaiter(
8887
suspendCompletion.await()
8988
}
9089

91-
private fun getDefaultTimeout(): Long {
92-
return if (AndroidUtils.isRunningOnMainThread()) ANDROID_ANR_TIMEOUT_MS else DEFAULT_TIMEOUT_MS
93-
}
90+
private fun getDefaultTimeout(): Long = if (AndroidUtils.isRunningOnMainThread()) ANDROID_ANR_TIMEOUT_MS else DEFAULT_TIMEOUT_MS
9491

95-
private fun createTimeoutMessage(timeoutMs: Long): String {
96-
return if (AndroidUtils.isRunningOnMainThread()) {
92+
private fun createTimeoutMessage(timeoutMs: Long): String =
93+
if (AndroidUtils.isRunningOnMainThread()) {
9794
"Timeout waiting for $componentName after ${timeoutMs}ms on the main thread. " +
9895
"This can cause ANRs. Consider calling from a background thread."
9996
} else {
10097
"Timeout waiting for $componentName after ${timeoutMs}ms."
10198
}
102-
}
10399

104100
private fun logAllThreads(): String {
105101
val sb = StringBuilder()

‎OneSignalSDK/onesignal/core/src/main/java/com/onesignal/internal/OneSignalImp.kt‎

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -334,10 +334,7 @@ internal class OneSignalImp(
334334
override fun <T> getAllServices(c: Class<T>): List<T> = services.getAllServices(c)
335335

336336
private fun waitForInit() {
337-
val completed = initAwaiter.await()
338-
if (!completed) {
339-
throw IllegalStateException("initWithContext was not called or timed out")
340-
}
337+
initAwaiter.awaitAndLogIfOverTimeout()
341338
}
342339

343340
/**

‎OneSignalSDK/onesignal/core/src/test/java/com/onesignal/common/threading/CompletionAwaiterTests.kt‎

Lines changed: 22 additions & 67 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ import kotlinx.coroutines.delay
1717
import kotlinx.coroutines.joinAll
1818
import kotlinx.coroutines.launch
1919
import kotlinx.coroutines.runBlocking
20+
import kotlin.concurrent.thread
2021

2122
class CompletionAwaiterTests : FunSpec({
2223

@@ -39,11 +40,10 @@ class CompletionAwaiterTests : FunSpec({
3940

4041
// When
4142
val startTime = System.currentTimeMillis()
42-
val completed = awaiter.await(1000)
43+
awaiter.awaitAndLogIfOverTimeout(1000)
4344
val duration = System.currentTimeMillis() - startTime
4445

4546
// Then
46-
completed shouldBe true
4747
duration shouldBeLessThan 50L // Should be very fast
4848
}
4949

@@ -59,57 +59,37 @@ class CompletionAwaiterTests : FunSpec({
5959
awaiter.complete()
6060
}
6161

62-
val result = awaiter.await(timeoutMs)
62+
awaiter.awaitAndLogIfOverTimeout(timeoutMs)
6363
val duration = System.currentTimeMillis() - startTime
6464

65-
result shouldBe true
6665
duration shouldBeGreaterThan (completionDelay - 50)
6766
duration shouldBeLessThan (completionDelay + 150) // buffer
6867
}
6968

70-
test("await returns false when timeout expires") {
69+
test("timeout input should only effect logging") {
7170
mockkObject(AndroidUtils)
7271
every { AndroidUtils.isRunningOnMainThread() } returns false
7372

74-
val timeoutMs = 200L
75-
val startTime = System.currentTimeMillis()
76-
77-
val completed = awaiter.await(timeoutMs)
78-
val duration = System.currentTimeMillis() - startTime
79-
80-
completed shouldBe false
81-
duration shouldBeGreaterThan (timeoutMs - 50)
82-
duration shouldBeLessThan (timeoutMs + 150)
83-
}
73+
val timeoutMs = 1L
8474

85-
test("await timeout of 0 returns false immediately when not completed") {
86-
// Mock AndroidUtils to avoid Looper.getMainLooper() issues
87-
mockkObject(AndroidUtils)
88-
every { AndroidUtils.isRunningOnMainThread() } returns false
75+
val thread = thread { awaiter.awaitAndLogIfOverTimeout(timeoutMs) }
8976

90-
val startTime = System.currentTimeMillis()
91-
val completed = awaiter.await(0)
92-
val duration = System.currentTimeMillis() - startTime
77+
delay(100)
9378

94-
completed shouldBe false
95-
duration shouldBeLessThan 20L
79+
thread.isAlive shouldBe true
9680

97-
unmockkObject(AndroidUtils)
81+
thread.interrupt()
9882
}
9983

10084
test("multiple blocking callers are all unblocked") {
10185
val numCallers = 5
102-
val results = mutableListOf<Boolean>()
10386
val jobs = mutableListOf<Thread>()
10487

10588
// Start multiple blocking callers
106-
repeat(numCallers) { index ->
89+
repeat(numCallers) {
10790
val thread =
10891
Thread {
109-
val result = awaiter.await(2000)
110-
synchronized(results) {
111-
results.add(result)
112-
}
92+
awaiter.awaitAndLogIfOverTimeout(2000)
11393
}
11494
thread.start()
11595
jobs.add(thread)
@@ -122,11 +102,8 @@ class CompletionAwaiterTests : FunSpec({
122102
awaiter.complete()
123103

124104
// Wait for all threads to complete
125-
jobs.forEach { it.join(1000) }
126-
127-
// All should have completed successfully
128-
results.size shouldBe numCallers
129-
results.all { it } shouldBe true
105+
// If this stalls on this test then it is considered failed
106+
jobs.forEach { it.join() }
130107
}
131108
}
132109

@@ -245,14 +222,10 @@ class CompletionAwaiterTests : FunSpec({
245222
awaiter = CompletionAwaiter("TestComponent")
246223

247224
// Test blocking callers
248-
val blockingResults = mutableListOf<Boolean>()
249225
val blockingThreads =
250-
(1..2).map { index ->
226+
(1..2).map {
251227
Thread {
252-
val result = awaiter.await(2000)
253-
synchronized(blockingResults) {
254-
blockingResults.add(result)
255-
}
228+
awaiter.awaitAndLogIfOverTimeout(2000)
256229
}
257230
}
258231
blockingThreads.forEach { it.start() }
@@ -264,10 +237,8 @@ class CompletionAwaiterTests : FunSpec({
264237
awaiter.complete()
265238

266239
// Wait for all to complete
267-
blockingThreads.forEach { it.join(1000) }
268-
269-
// All should have completed
270-
blockingResults shouldBe arrayOf(true, true)
240+
// Test considered failed if we hang here forever
241+
blockingThreads.forEach { it.join() }
271242
}
272243
}
273244

@@ -280,8 +251,7 @@ class CompletionAwaiterTests : FunSpec({
280251
awaiter.complete()
281252

282253
// Should still work normally
283-
val completed = awaiter.await(100)
284-
completed shouldBe true
254+
awaiter.awaitAndLogIfOverTimeout(100)
285255
}
286256

287257
test("waiting after completion returns immediately") {
@@ -331,33 +301,18 @@ class CompletionAwaiterTests : FunSpec({
331301

332302
context("timeout behavior") {
333303

334-
test("uses shorter timeout on main thread") {
304+
test("uses shorter timeout on main thread").config(enabled = false) {
335305
mockkObject(AndroidUtils)
336306
every { AndroidUtils.isRunningOnMainThread() } returns true
337307

338-
val startTime = System.currentTimeMillis()
339-
val completed = awaiter.await() // Default timeout
340-
val duration = System.currentTimeMillis() - startTime
341-
342-
completed shouldBe false
343-
// Should use ANDROID_ANR_TIMEOUT_MS (4800ms) instead of DEFAULT_TIMEOUT_MS (30000ms)
344-
duration shouldBeLessThan 6000L // Much less than 30 seconds
345-
duration shouldBeGreaterThan 4000L // But around 4.8 seconds
308+
// NOTE: Reintroduce test once we use Otel with non-fatal errors
346309
}
347310

348-
test("uses longer timeout on background thread") {
311+
test("uses longer timeout on background thread").config(enabled = false) {
349312
mockkObject(AndroidUtils)
350313
every { AndroidUtils.isRunningOnMainThread() } returns false
351314

352-
// We can't actually wait 30 seconds in a test, so just verify it would use the longer timeout
353-
// by checking the timeout logic doesn't kick in quickly
354-
val startTime = System.currentTimeMillis()
355-
val completed = awaiter.await(1000) // Force shorter timeout for test
356-
val duration = System.currentTimeMillis() - startTime
357-
358-
completed shouldBe false
359-
duration shouldBeGreaterThan 900L
360-
duration shouldBeLessThan 1200L
315+
// NOTE: Reintroduce test once we use Otel with non-fatal errors
361316
}
362317
}
363318
})

‎OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/application/SDKInitTests.kt‎

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -91,7 +91,7 @@ class SDKInitTests : FunSpec({
9191
// block SharedPreference before calling init
9292
val trigger = CompletionAwaiter("Test")
9393
val context = getApplicationContext<Context>()
94-
val blockingPrefContext = BlockingPrefsContext(context, trigger, 2000)
94+
val blockingPrefContext = BlockingPrefsContext(context, trigger)
9595
val os = OneSignalImp()
9696
var initSuccess = true
9797

@@ -137,7 +137,7 @@ class SDKInitTests : FunSpec({
137137
// block SharedPreference before calling init
138138
val trigger = CompletionAwaiter("Test")
139139
val context = getApplicationContext<Context>()
140-
val blockingPrefContext = BlockingPrefsContext(context, trigger, 1000)
140+
val blockingPrefContext = BlockingPrefsContext(context, trigger)
141141
val os = OneSignalImp()
142142

143143
// When
@@ -160,7 +160,7 @@ class SDKInitTests : FunSpec({
160160
// block SharedPreference before calling init
161161
val trigger = CompletionAwaiter("Test")
162162
val context = getApplicationContext<Context>()
163-
val blockingPrefContext = BlockingPrefsContext(context, trigger, 2000)
163+
val blockingPrefContext = BlockingPrefsContext(context, trigger)
164164
val os = OneSignalImp()
165165

166166
val accessorThread =
@@ -204,7 +204,7 @@ class SDKInitTests : FunSpec({
204204
// block SharedPreference before calling init
205205
val trigger = CompletionAwaiter("Test")
206206
val context = getApplicationContext<Context>()
207-
val blockingPrefContext = BlockingPrefsContext(context, trigger, 2000)
207+
val blockingPrefContext = BlockingPrefsContext(context, trigger)
208208
val os = OneSignalImp()
209209
val externalId = "testUser"
210210

@@ -438,14 +438,13 @@ class SDKInitTests : FunSpec({
438438
class BlockingPrefsContext(
439439
context: Context,
440440
private val unblockTrigger: CompletionAwaiter,
441-
private val timeoutInMillis: Long,
442441
) : ContextWrapper(context) {
443442
override fun getSharedPreferences(
444443
name: String,
445444
mode: Int,
446445
): SharedPreferences {
447446
try {
448-
unblockTrigger.await(timeoutInMillis)
447+
unblockTrigger.awaitAndLogIfOverTimeout()
449448
} catch (e: InterruptedException) {
450449
throw e
451450
} catch (e: TimeoutCancellationException) {

0 commit comments

Comments
 (0)