Skip to content

Commit bc91777

Browse files
committed
recover from a stalled user in the OperationRepo
Commit 7fe2010 fixed a bug with login operations being drop in some scenarios, however it doesn't fix those already in the bad state. This comment get us out of the bad state by generating a replacement LoginUserOperation. See the comments in the code for more details. This comment was manually tested, ensuring it gets us out of the bad state, and updates works afterwords.
1 parent cbe6f16 commit bc91777

File tree

5 files changed

+128
-0
lines changed

5 files changed

+128
-0
lines changed

OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/operations/IOperationRepo.kt

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
11
package com.onesignal.core.internal.operations
22

3+
import kotlin.reflect.KClass
4+
35
/**
46
* The operation queue provides a mechanism to queue one or more [Operation] with the promise
57
* it will be executed in a background thread at some point in the future. Operations are
@@ -31,4 +33,13 @@ interface IOperationRepo {
3133
operation: Operation,
3234
flush: Boolean = false,
3335
): Boolean
36+
37+
/**
38+
* Check if the queue contains a specific operation type
39+
*/
40+
fun <T : Operation> containsInstanceOf(type: KClass<T>): Boolean
3441
}
42+
43+
// Extension function so the syntax containsInstanceOf<Operation>() can be used over
44+
// containsInstanceOf(Operation::class)
45+
inline fun <reified T : Operation> IOperationRepo.containsInstanceOf(): Boolean = containsInstanceOf(T::class)

OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/operations/impl/OperationRepo.kt

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ import com.onesignal.debug.internal.logging.Logging
1515
import kotlinx.coroutines.delay
1616
import kotlinx.coroutines.withTimeoutOrNull
1717
import java.util.UUID
18+
import kotlin.reflect.KClass
1819

1920
internal class OperationRepo(
2021
executors: List<IOperationExecutor>,
@@ -52,6 +53,12 @@ internal class OperationRepo(
5253
}
5354
}
5455

56+
override fun <T : Operation> containsInstanceOf(type: KClass<T>): Boolean {
57+
synchronized(queue) {
58+
return queue.any { type.isInstance(it.operation) }
59+
}
60+
}
61+
5562
override fun start() {
5663
paused = false
5764
suspendifyOnThread(name = "OpRepo") {

OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/UserModule.kt

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ import com.onesignal.user.internal.backend.impl.UserBackendService
1515
import com.onesignal.user.internal.builduser.IRebuildUserService
1616
import com.onesignal.user.internal.builduser.impl.RebuildUserService
1717
import com.onesignal.user.internal.identity.IdentityModelStore
18+
import com.onesignal.user.internal.migrations.RecoverFromDroppedLoginBug
1819
import com.onesignal.user.internal.operations.impl.executors.IdentityOperationExecutor
1920
import com.onesignal.user.internal.operations.impl.executors.LoginUserFromSubscriptionOperationExecutor
2021
import com.onesignal.user.internal.operations.impl.executors.LoginUserOperationExecutor
@@ -65,5 +66,7 @@ internal class UserModule : IModule {
6566
builder.register<UserManager>().provides<IUserManager>()
6667

6768
builder.register<UserRefreshService>().provides<IStartableService>()
69+
70+
builder.register<RecoverFromDroppedLoginBug>().provides<IStartableService>()
6871
}
6972
}
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,85 @@
1+
package com.onesignal.user.internal.migrations
2+
3+
import com.onesignal.common.IDManager
4+
import com.onesignal.core.internal.config.ConfigModelStore
5+
import com.onesignal.core.internal.operations.IOperationRepo
6+
import com.onesignal.core.internal.operations.containsInstanceOf
7+
import com.onesignal.core.internal.startup.IStartableService
8+
import com.onesignal.debug.internal.logging.Logging
9+
import com.onesignal.user.internal.identity.IdentityModelStore
10+
import com.onesignal.user.internal.operations.LoginUserOperation
11+
12+
/**
13+
* Purpose: Automatically recovers a stalled User in the OperationRepo due
14+
* to a bug in the SDK from 5.0.0 to 5.1.7.
15+
*
16+
* Issue: Some calls to OneSignal.login() would not be reflected on the
17+
* backend and would stall the the queue for that User. This would result
18+
* in User and Subscription operations to not be processed by
19+
* OperationRepo.
20+
* See PR #2046 for more details.
21+
*
22+
* Even if the developer called OneSignal.login() again with the same
23+
* externalId it would not correct the stalled User.
24+
* - Only calling logout() or login() with different externalId would
25+
* have un-stalled the OperationRepo. And then only after logging
26+
* back to the stalled user would it have recover all the unsent
27+
* operations they may exist.
28+
*/
29+
class RecoverFromDroppedLoginBug(
30+
private val _operationRepo: IOperationRepo,
31+
private val _identityModelStore: IdentityModelStore,
32+
private val _configModelStore: ConfigModelStore,
33+
) : IStartableService {
34+
override fun start() {
35+
if (isInBadState()) {
36+
Logging.warn(
37+
"User with externalId:" +
38+
"${_identityModelStore.model.externalId} " +
39+
"was in a bad state, causing it to not update on OneSignal's " +
40+
"backend! We are recovering and replaying all unsent " +
41+
"operations now.",
42+
)
43+
recoverByAddingBackDroppedLoginOperation()
44+
}
45+
}
46+
47+
// We are in the bad state if ALL are true:
48+
// 1. externalId is set (because OneSignal.login was called)
49+
// 2. We don't have a real yet onesignalId
50+
// - We haven't made a successful user create call yet.
51+
// 3. There is no attempt to create the User left in the
52+
// OperationRepo's queue.
53+
private fun isInBadState(): Boolean {
54+
val externalId = _identityModelStore.model.externalId
55+
val onesignalId = _identityModelStore.model.onesignalId
56+
57+
// NOTE: We are not accounting a more rare (and less important case)
58+
// where a previously logged in User was never created.
59+
// That is, if another user already logged in successfully, but
60+
// the last user still has stuck pending operations due to the
61+
// User never being created on the OneSignal's backend.
62+
return externalId != null &&
63+
IDManager.isLocalId(onesignalId) &&
64+
!_operationRepo.containsInstanceOf<LoginUserOperation>()
65+
}
66+
67+
private fun recoverByAddingBackDroppedLoginOperation() {
68+
// This is the operation that was dropped by mistake in some cases,
69+
// once it is added to the queue all and it gets executed, all
70+
// operations waiting on it will be sent.
71+
72+
// This enqueues at the end, however this is ok, since
73+
// the OperationRepo is designed find the first operation that is
74+
// executable.
75+
_operationRepo.enqueue(
76+
LoginUserOperation(
77+
_configModelStore.model.appId,
78+
_identityModelStore.model.onesignalId,
79+
_identityModelStore.model.externalId,
80+
null,
81+
),
82+
true,
83+
)
84+
}
85+
}

OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/operations/OperationRepoTests.kt

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,28 @@ class OperationRepoTests : FunSpec({
6262
Logging.logLevel = LogLevel.NONE
6363
}
6464

65+
test("containsInstanceOf") {
66+
// Given
67+
val operationRepo = Mocks().operationRepo
68+
69+
open class MyOperation : Operation("MyOp") {
70+
override val createComparisonKey = ""
71+
override val modifyComparisonKey = ""
72+
override val groupComparisonType = GroupComparisonType.NONE
73+
override val canStartExecute = false
74+
}
75+
76+
class MyOperation2 : MyOperation()
77+
78+
// When
79+
operationRepo.start()
80+
operationRepo.enqueue(MyOperation())
81+
82+
// Then
83+
operationRepo.containsInstanceOf<MyOperation>() shouldBe true
84+
operationRepo.containsInstanceOf<MyOperation2>() shouldBe false
85+
}
86+
6587
// Ensures we are not continuously waking the CPU
6688
test("ensure processQueueForever suspends when queue is empty") {
6789
// Given

0 commit comments

Comments
 (0)