Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix: Investigate initializer errors #69

Merged
merged 13 commits into from
Jul 18, 2024
Merged

Fix: Investigate initializer errors #69

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
75fe055
Remove 2 provider setups and include the same setup in no-op manifest
AsimRibo Jul 1, 2024
8c0568a
Revert "Remove 2 provider setups and include the same setup in no-op …
KCeh Jul 5, 2024
4b64129
Add check if WorkManager is initialized
KCeh Jul 5, 2024
d10925c
Add delegate worker for certificate check
KCeh Jul 7, 2024
b2bb4e7
Fix lint
KCeh Jul 7, 2024
e80a050
Add annotation to UI thread only method
KCeh Jul 14, 2024
f7666a1
Remove redundant manifest tags
KCeh Jul 14, 2024
ee6f2e0
Merge branch 'develop' into fix/investigate-initializer-errors
KCeh Jul 14, 2024
c023224
Merge branch 'develop' of https://github.com/infinum/android-sentinel…
KCeh Jul 17, 2024
8e5998d
Remove unnecessary work manager initialization
KCeh Jul 17, 2024
337d91f
Typo fix
KCeh Jul 17, 2024
fbdf3a9
Replace failure result with exception
KCeh Jul 17, 2024
bbd0d8a
Improve DI setup for work manager
KCeh Jul 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 3 additions & 14 deletions sentinel/src/main/AndroidManifest.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,10 @@
android:name=".ui.main.SentinelActivity"
android:exported="true"
android:label="@string/sentinel_name"
android:permission="com.infinum.sentinel.permission.ACCESS_SENTINEL"
android:taskAffinity="com.infinum.sentinel"
android:theme="@style/Sentinel.Theme"
android:permission="com.infinum.sentinel.permission.ACCESS_SENTINEL">
<meta-data
android:theme="@style/Sentinel.Theme">
<meta-data
android:name="@string/sentinel_infinum_monitored"
android:value="false" />
</activity>
Expand Down Expand Up @@ -119,17 +119,6 @@
android:value="androidx.startup" />
</provider>

<provider
android:name="androidx.startup.InitializationProvider"
android:authorities="${applicationId}.androidx-startup"
android:exported="false"
tools:node="merge">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is Sentinel removing the work manager initializer? Does it even use anything from the work manager?

I'm worried about this because if the app add sentinel they would "hiddenly" remove the initialization of the work manager. Everything would be fine for the apps using custom work manager initialization, but for the apps using the default one, work manager wouldn't be initialized anymore.

I'd be calmer if we would remove this code completely, therefore not removing the work manager initialization, and therefore leaving the work manager setup as it is defined in the apps.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will look into that option as well. It should work.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to double check. You are suggesting to remove this whole part?

<meta-data
                android:name="androidx.work.WorkManagerInitializer"
                android:value="androidx.startup"
                tools:node="remove" />

Do you then think we should add this to our README as a reminder for users that they will have to do it in their projects?
I will leave the decision to Karlo as he is the owner.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I'm suggesting that, but not in case users would need to add this to their projects only because they have Sentinel.
I'm suggesting removing this if it is redundant.
The only thing when users should add this is in case they use a work manager in their project.

Does Sentinel even use a work manager? What for?

Copy link
Contributor Author

@AsimRibo AsimRibo Jul 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think only usage is in SentinelWorkManager class. It is used for certificates checking. Interestingly enough, this is the second time certificates are causing problems 😄 .

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And I think that is a problem. Since WorkManager can be initialized only once (next time it throws), any application using Work Manager would fail to supply their own workers and initialize the WorkManager. That means, the application dev has to choose, either to use WorkManager and no Sentinel, or vice-versa.

That's the problem we should investigate further and see how to avoid the clash.

<meta-data
android:name="androidx.work.WorkManagerInitializer"
android:value="androidx.startup"
tools:node="remove" />
</provider>

</application>

</manifest>
1 change: 1 addition & 0 deletions sentinel/src/main/kotlin/com/infinum/sentinel/di/LibraryComponents.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ internal object LibraryComponents {
tools.filterIsInstance<CertificateTool>().firstOrNull()?.userCertificates.orEmpty(),
onTriggered
)
WorkManagerInitializer.init(domainComponent)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs to be called here, else tools won't be visible in UI

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why can't it be called from initialize method? Why the tools won't be visible in the UI?

Copy link
Collaborator

@KCeh KCeh Jul 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great questions!
It seems that due to SentinelWorkerFactory dependencies. Collectors.Tools & Factories.Collector are created before they are configured in DataComponent.setup(). DataComponent.setup() remembers tools that user passed when they started lib and passes them to Collectors.Tools. DataComponent.setup() is invoked when user starts lib (they pass set of tools).

Collectors.Tools is used to fetch tools for UI.

Current solution is to put WorkManagerInitializer in setup() method. Which is directly invoked in Sentinel.watch() (entry point for lib). And it is invoked after DataComponent.setup().
Meaning: Collectors.Tools and related objects will be correctly configured

TLDR: DI/configuration 🍝

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, thanks for the explanation 👍

domainComponent.setup()
}

Expand Down
14 changes: 14 additions & 0 deletions sentinel/src/main/kotlin/com/infinum/sentinel/di/WorkManagerInitializer.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
package com.infinum.sentinel.di

import com.infinum.sentinel.di.component.DomainComponent
import com.infinum.sentinel.ui.certificates.observer.CertificateCheckWorker
import com.infinum.sentinel.ui.certificates.observer.DelegateWorker
import com.infinum.sentinel.ui.certificates.observer.SentinelWorkerFactory

internal object WorkManagerInitializer {

fun init(domainComponent: DomainComponent) {
DelegateWorker.workerFactories[CertificateCheckWorker.NAME] =
SentinelWorkerFactory(domainComponent.collectors, domainComponent.notificationFactory)
}
}
10 changes: 1 addition & 9 deletions sentinel/src/main/kotlin/com/infinum/sentinel/di/component/DomainComponent.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,6 @@ import com.infinum.sentinel.ui.bundles.callbacks.BundleMonitorNotificationCallba
import com.infinum.sentinel.ui.bundles.details.BundleDetailsActivity
import com.infinum.sentinel.ui.certificates.observer.CertificatesObserver
import com.infinum.sentinel.ui.certificates.observer.SentinelWorkManager
import com.infinum.sentinel.ui.certificates.observer.SentinelWorkerFactory
import com.infinum.sentinel.ui.crash.anr.SentinelAnrObserver
import com.infinum.sentinel.ui.crash.anr.SentinelAnrObserverRunnable
import com.infinum.sentinel.ui.crash.anr.SentinelUiAnrObserver
Expand Down Expand Up @@ -91,8 +90,6 @@ internal abstract class DomainComponent(

abstract val sentinelAnrObserver: SentinelAnrObserver

abstract val sentinelWorkerFactory: SentinelWorkerFactory

abstract val sentinelWorkManager: SentinelWorkManager

abstract val sentinelAnrObserverRunnable: SentinelAnrObserverRunnable
Expand Down Expand Up @@ -157,15 +154,10 @@ internal abstract class DomainComponent(
fun sentinelAnrObserverRunnable(dao: CrashesDao): SentinelAnrObserverRunnable =
SentinelAnrObserverRunnable(context, notificationFactory, dao)

@Provides
@DomainScope
fun sentinelWorkerFactory(): SentinelWorkerFactory =
SentinelWorkerFactory(collectors, notificationFactory)

@Provides
@DomainScope
fun sentinelWorkManager(): SentinelWorkManager =
SentinelWorkManager(context, sentinelWorkerFactory)
SentinelWorkManager(context)

@Provides
@DomainScope
Expand Down
54 changes: 54 additions & 0 deletions sentinel/src/main/kotlin/com/infinum/sentinel/ui/certificates/observer/DelegateWorker.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
package com.infinum.sentinel.ui.certificates.observer

import android.content.Context
import androidx.annotation.UiThread
import androidx.work.ListenableWorker
import androidx.work.WorkerFactory
import androidx.work.WorkerParameters
import com.google.common.util.concurrent.ListenableFuture
import com.infinum.sentinel.ui.shared.Constants.Keys.WORKER_CLASS_NAME
import com.infinum.sentinel.ui.shared.Constants.Keys.WORKER_ID

/**
* A worker to delegate work requests from within the library to workers
* that require factories with custom dependencies.
*/
internal class DelegateWorker(
appContext: Context,
parameters: WorkerParameters,
) : ListenableWorker(appContext, parameters) {

private val workerClassName =
parameters.inputData.getString(WORKER_CLASS_NAME) ?: ""
private val workerId = parameters.inputData.getString(WORKER_ID)
private val delegateWorkerFactory = workerFactories[workerId]
private val delegatedWorker = delegateWorkerFactory?.createWorker(appContext, workerClassName, parameters)

override fun startWork(): ListenableFuture<Result> {
return if (delegatedWorker != null) {
delegatedWorker.startWork()
} else {
val errorMessage = "No delegateWorker available for $workerId" +
" with workerClassName of $workerClassName. Is the " +
"DelegateWorker.workerFactories populated correctly?"
throw IllegalStateException(errorMessage)
}
}

companion object {
const val DELEGATE_WORKER_ID = "com.infinum.sentinel.ui.certificates.observer.CertificateCheckWorker"

val workerFactories = object : AbstractMutableMap<String, WorkerFactory>() {

private val backingWorkerMap = mutableMapOf<String, WorkerFactory>()

@UiThread
override fun put(key: String, value: WorkerFactory): WorkerFactory? {
return backingWorkerMap.put(key, value)
}

override val entries: MutableSet<MutableMap.MutableEntry<String, WorkerFactory>>
get() = backingWorkerMap.entries
}
}
}
41 changes: 16 additions & 25 deletions ...inel/src/main/kotlin/com/infinum/sentinel/ui/certificates/observer/SentinelWorkManager.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,21 +4,21 @@ import android.content.Context
import android.os.Build
import androidx.annotation.RequiresApi
import androidx.lifecycle.asFlow
import androidx.work.Configuration
import androidx.work.Constraints
import androidx.work.ExistingPeriodicWorkPolicy
import androidx.work.NetworkType
import androidx.work.PeriodicWorkRequestBuilder
import androidx.work.WorkInfo
import androidx.work.WorkManager
import androidx.work.WorkerFactory
import androidx.work.workDataOf
import com.infinum.sentinel.BuildConfig
import com.infinum.sentinel.data.models.local.CertificateMonitorEntity
import com.infinum.sentinel.ui.shared.Constants.Keys.EXPIRE_IN_AMOUNT
import com.infinum.sentinel.ui.shared.Constants.Keys.EXPIRE_IN_UNIT
import com.infinum.sentinel.ui.shared.Constants.Keys.NOTIFY_INVALID_NOW
import com.infinum.sentinel.ui.shared.Constants.Keys.NOTIFY_TO_EXPIRE
import com.infinum.sentinel.ui.shared.Constants.Keys.WORKER_CLASS_NAME
import com.infinum.sentinel.ui.shared.Constants.Keys.WORKER_ID
import java.time.Duration
import kotlinx.coroutines.flow.Flow
import kotlinx.coroutines.flow.flowOf
Expand All @@ -28,44 +28,34 @@ import me.tatarka.inject.annotations.Inject
@Inject
internal class SentinelWorkManager(
private val context: Context,
private val workerFactory: WorkerFactory
) {

companion object {
private const val DEBUG_INTERVAL = 15L
private const val RELEASE_INTERVAL = 1440L
}

init {
WorkManager.initialize(
context,
Configuration.Builder()
.setMinimumLoggingLevel(android.util.Log.INFO)
.setWorkerFactory(workerFactory)
.build()
)
}

@RequiresApi(Build.VERSION_CODES.O)
fun startCertificatesCheck(entity: CertificateMonitorEntity) =
fun startCertificatesCheck(entity: CertificateMonitorEntity) {

val delegatedWorkData = workDataOf(
WORKER_CLASS_NAME to CertificateCheckWorker::class.qualifiedName,
WORKER_ID to CertificateCheckWorker.NAME,
NOTIFY_INVALID_NOW to entity.notifyInvalidNow,
NOTIFY_TO_EXPIRE to entity.notifyToExpire,
EXPIRE_IN_AMOUNT to entity.expireInAmount,
EXPIRE_IN_UNIT to entity.expireInUnit.name
)
WorkManager.getInstance(context)
.enqueueUniquePeriodicWork(
CertificateCheckWorker.NAME,
DelegateWorker.DELEGATE_WORKER_ID,
ExistingPeriodicWorkPolicy.CANCEL_AND_REENQUEUE,
PeriodicWorkRequestBuilder<CertificateCheckWorker>(
PeriodicWorkRequestBuilder<DelegateWorker>(
antunflas marked this conversation as resolved.
Show resolved Hide resolved
when (BuildConfig.DEBUG) {
true -> Duration.ofMinutes(DEBUG_INTERVAL)
false -> Duration.ofMinutes(RELEASE_INTERVAL)
}
)
.setInputData(
workDataOf(
NOTIFY_INVALID_NOW to entity.notifyInvalidNow,
NOTIFY_TO_EXPIRE to entity.notifyToExpire,
EXPIRE_IN_AMOUNT to entity.expireInAmount,
EXPIRE_IN_UNIT to entity.expireInUnit.name
)
)
).setInputData(delegatedWorkData)
.setConstraints(
Constraints.Builder()
.setRequiredNetworkType(NetworkType.NOT_REQUIRED)
Expand All @@ -80,6 +70,7 @@ internal class SentinelWorkManager(
)
.build()
)
}

fun certificatesCheckState(): Flow<Boolean> =
WorkManager.getInstance(context)
Expand Down
2 changes: 2 additions & 0 deletions sentinel/src/main/kotlin/com/infinum/sentinel/ui/shared/Constants.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,5 +14,7 @@ internal object Constants {
const val NOTIFY_TO_EXPIRE: String = "KEY_NOTIFY_TO_EXPIRE"
const val EXPIRE_IN_AMOUNT: String = "KEY_EXPIRE_IN_AMOUNT"
const val EXPIRE_IN_UNIT: String = "KEY_EXPIRE_IN_UNIT"
const val WORKER_CLASS_NAME = "WORKER_CLASS_NAME"
const val WORKER_ID = "WORKER_ID"
}
}
Loading