Fix: Investigate initializer errors #69

AsimRibo · 2024-07-01T18:48:10Z

📷 Screenshots

📄 Context

When user wouldn't include all tools into build.gradle file, it would cause crash "WorkManager is already initialized". It seems problem was in how AndroidManifests were merged.
I am adding Antun as optional reviewer since he was there when crashes happened 😄 .

📝 Changes

By removing 2 startup providers and merging them under one with separate meta-data, crashes are gone. Same provider had to be put inside of no-op module as well, otherwise it would cause NPE. Other modules didn't have to be touched.

📎 Related PR

🚫 Breaking

🛠️ How to test

⏱️ Next steps

antunflas · 2024-07-02T05:29:57Z

sentinel/src/main/AndroidManifest.xml

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


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.

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

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.

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?

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 😄 .

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.

…manifest" This reverts commit 75fe055.

To bypass restriction of now passing workerFactory to WorkManager configuration

KCeh · 2024-07-07T13:43:30Z

I made a fix based on https://developer.squareup.com/blog/workmanager-for-background-work-in-libraries/

I changed 2 things:

Sentinel will initialize WorkManager only if it hasn't been initialized
In Sentinel now we invoke DelegateWorker (instead of CertificateCheckWorker) to do work. DelegateWorker will create an appropriate worker and delegate work to it. The only additional trick is to remember WorkerFactory for our CertificateCheckWorker inside of DelegateWorker. Why do we need to do that?
Because CertificateCheckWorker (and related factory) have custom dependencies and WorkManager won't be able to create them without setting Configuration (which we don't set up if app initializes WorkManager)

How I tested:

I added Sentinel to New Project Templated (only main Sentinel lib without extra tools). The old version of Sentinel crashed the app, while the new one (with this PR) doesn't.
I have also tested the sample app and added a log to see if doWork is invoked properly in CertificateCheckWorker. Also, I managed to get notifications from Sentinel regarding the invalid certificate (notifications have to be enabled!). So I would say it works! (CertificateCheckWorker was also properly invoked in project where Sentinel doesn't initialize WorkManager)

AsimRibo

Looks good 🔥 .
Do we need to have 2 different provider elements in AndroidManifest? I think meta data can still be under one provider element.
Also, I can't approve this because I am still author 😂 . Should we ping Flas or someone?

sentinel/src/main/kotlin/com/infinum/sentinel/ui/certificates/observer/DelegateWorker.kt

KCeh · 2024-07-09T08:12:23Z

Do we need to have 2 different provider elements in AndroidManifest? I think meta data can still be under one provider element

I haven't look into that to be honest, I will explore

Also, I can't approve this because I am still author 😂 . Should we ping Flas or someone?

Well, I can approve PR for myself? But, yes, if someone else can take a look why not?

KCeh · 2024-07-16T14:37:55Z

Are we good with this PR?

AsimRibo · 2024-07-16T14:40:22Z

Are we good with this PR?

From my side good. I pinged Antun today but I believe he is busy. I think we can merge due to release.

antunflas · 2024-07-16T15:08:20Z

sentinel/src/main/kotlin/com/infinum/sentinel/ui/certificates/observer/DelegateWorker.kt

+            object : ListenableFuture<Result> {
+                override fun isDone(): Boolean = true
+                override fun get(): Result = Result.failure(errorData)
+                override fun get(timeout: Long, unit: TimeUnit): Result = Result.failure(errorData)
+                override fun cancel(mayInterruptIfRunning: Boolean): Boolean = false
+                override fun isCancelled(): Boolean = false
+                override fun addListener(listener: Runnable, executor: Executor) = listener.run()
+            }


Why do we return an "empty" future instead of throwing an error? Shouldn't throwing an error be safer?

Well, it is not "empty". It is failure with errorData. We need to return ListenableFuture<Result>

Yeah, but I'm exploring less "silent" ways of this failing. Realistically, it could happen only if we forget to register a worker factory, but that's something we shouldn't allow to end up in a release. I'm trying to figure out how we could make sure to not fail fast and loud :D

Do you think just exception will do the job?

I'm open for discussions, but an exception (if it would crash the app) would definitely be more noticeable.

If we throw exception in this place it will just produce logs, similar to Result.Failure. Because it is running on background thread

Work [ id=42358918-83db-4c07-847b-6020296c3d41, tags={ com.infinum.sentinel.ui.certificates.observer.DelegateWorker } ] failed because it threw an exception/error java.util.concurrent.ExecutionException: java.lang.IllegalStateException: No delegateWorker available for com.infinum.sentinel.ui.certificates.observer.CertificateCheckWorker with workerClassName of com.infinum.sentinel.ui.certificates.observer.CertificateCheckWorker. Is the DelegateWorker.workerFactories populated correctly? at androidx.work.impl.utils.futures.AbstractFuture.getDoneValue(AbstractFuture.java:515) at androidx.work.impl.utils.futures.AbstractFuture.get(AbstractFuture.java:474) at androidx.work.impl.WorkerWrapper$2.run(WorkerWrapper.java:316) at androidx.work.impl.utils.SerialExecutorImpl$Task.run(SerialExecutorImpl.java:96) at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1137) at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:637) at java.lang.Thread.run(Thread.java:1012) Caused by: java.lang.IllegalStateException: No delegateWorker available for com.infinum.sentinel.ui.certificates.observer.CertificateCheckWorker with workerClassName of com.infinum.sentinel.ui.certificates.observer.CertificateCheckWorker. Is the RouterWorker.workerFactories populated correctly? at com.infinum.sentinel.ui.certificates.observer.DelegateWorker.startWork(DelegateWorker.kt:40) at androidx.work.impl.WorkerWrapper$1.run(WorkerWrapper.java:302) at android.os.Handler.handleCallback(Handler.java:942) at android.os.Handler.dispatchMessage(Handler.java:99) at android.os.Looper.loopOnce(Looper.java:201) at android.os.Looper.loop(Looper.java:288) at android.app.ActivityThread.main(ActivityThread.java:7872) at java.lang.reflect.Method.invoke(Native Method) at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:548) at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:936) 2024-07-17 16:57:47.222 15350-15401 WM-WorkerWrapper com.infinum.sentinel.sample.debug I Worker result FAILURE for Work [ id=42358918-83db-4c07-847b-6020296c3d41, tags={ com.infinum.sentinel.ui.certificates.observer.DelegateWorker } ]

But error is defiantly more noticeable in logs

If things go 💥 at least people will see errors and open issues 😅
So I will go with exception

sentinel/src/main/kotlin/com/infinum/sentinel/ui/certificates/observer/SentinelWorkManager.kt

sentinel/src/main/kotlin/com/infinum/sentinel/di/component/DomainComponent.kt

…into fix/investigate-initializer-errors

KCeh · 2024-07-17T15:42:14Z

All points should be improved upon now

@AsimRibo recheck one more time. Just don't forget to bump version in config.gradle and libs.toml publish to mavenLocal and used that new version. Because tools have references to published version and if you don't update that before test it could cause errors.

sonarcloud · 2024-07-17T15:42:37Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

KCeh · 2024-07-17T15:42:56Z

sentinel/src/main/kotlin/com/infinum/sentinel/di/LibraryComponents.kt

@@ -42,6 +42,7 @@ internal object LibraryComponents {
            tools.filterIsInstance<CertificateTool>().firstOrNull()?.userCertificates.orEmpty(),
            onTriggered
        )
+        WorkManagerInitializer.init(domainComponent)


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

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

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 🍝

Okay, thanks for the explanation 👍

AsimRibo · 2024-07-18T01:17:42Z

All points should be improved upon now

@AsimRibo recheck one more time. Just don't forget to bump version in config.gradle and libs.toml publish to mavenLocal and used that new version. Because tools have references to published version and if you don't update that before test it could cause errors.

Looking good amigo.

Remove 2 provider setups and include the same setup in no-op manifest

75fe055

AsimRibo added the bug Something isn't working label Jul 1, 2024

AsimRibo requested review from antunflas and KCeh July 1, 2024 18:48

antunflas reviewed Jul 2, 2024

View reviewed changes

KCeh added 3 commits July 5, 2024 12:55

Revert "Remove 2 provider setups and include the same setup in no-op …

8c0568a

…manifest" This reverts commit 75fe055.

Add check if WorkManager is initialized

4b64129

Add delegate worker for certificate check

d10925c

To bypass restriction of now passing workerFactory to WorkManager configuration

Fix lint

b2bb4e7

AsimRibo commented Jul 9, 2024

View reviewed changes

sentinel/src/main/kotlin/com/infinum/sentinel/ui/certificates/observer/DelegateWorker.kt Outdated Show resolved Hide resolved

KCeh added 3 commits July 14, 2024 15:40

Add annotation to UI thread only method

e80a050

Remove redundant manifest tags

f7666a1

Merge branch 'develop' into fix/investigate-initializer-errors

ee6f2e0

antunflas reviewed Jul 16, 2024

View reviewed changes

sentinel/src/main/kotlin/com/infinum/sentinel/ui/certificates/observer/SentinelWorkManager.kt Outdated Show resolved Hide resolved

antunflas reviewed Jul 16, 2024

View reviewed changes

sentinel/src/main/kotlin/com/infinum/sentinel/di/component/DomainComponent.kt Outdated Show resolved Hide resolved

KCeh added 5 commits July 17, 2024 15:23

Merge branch 'develop' of https://github.com/infinum/android-sentinel …

c023224

…into fix/investigate-initializer-errors

Remove unnecessary work manager initialization

8e5998d

Typo fix

337d91f

Replace failure result with exception

fbdf3a9

Improve DI setup for work manager

bbd0d8a

KCeh reviewed Jul 17, 2024

View reviewed changes

antunflas approved these changes Jul 17, 2024

View reviewed changes

KCeh merged commit 0aa90fa into develop Jul 18, 2024
6 checks passed

KCeh deleted the fix/investigate-initializer-errors branch August 20, 2024 19:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: Investigate initializer errors #69

Fix: Investigate initializer errors #69

AsimRibo commented Jul 1, 2024 •

edited

Loading

antunflas Jul 2, 2024

AsimRibo Jul 2, 2024

AsimRibo Jul 2, 2024

antunflas Jul 2, 2024

AsimRibo Jul 2, 2024 •

edited

Loading

antunflas Jul 2, 2024

KCeh commented Jul 7, 2024 •

edited

Loading

AsimRibo left a comment •

edited

Loading

KCeh commented Jul 9, 2024

KCeh commented Jul 16, 2024

AsimRibo commented Jul 16, 2024 •

edited

Loading

antunflas Jul 16, 2024

KCeh Jul 16, 2024

antunflas Jul 16, 2024

KCeh Jul 16, 2024

antunflas Jul 16, 2024

KCeh Jul 17, 2024

KCeh Jul 17, 2024 •

edited

Loading

KCeh commented Jul 17, 2024

sonarcloud bot commented Jul 17, 2024

KCeh Jul 17, 2024

antunflas Jul 17, 2024

KCeh Jul 17, 2024 •

edited

Loading

antunflas Jul 17, 2024

AsimRibo commented Jul 18, 2024

Fix: Investigate initializer errors #69

Fix: Investigate initializer errors #69

Conversation

AsimRibo commented Jul 1, 2024 • edited Loading

📷 Screenshots

📄 Context

📝 Changes

📎 Related PR

🚫 Breaking

🛠️ How to test

⏱️ Next steps

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AsimRibo Jul 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KCeh commented Jul 7, 2024 • edited Loading

AsimRibo left a comment • edited Loading

Choose a reason for hiding this comment

KCeh commented Jul 9, 2024

KCeh commented Jul 16, 2024

AsimRibo commented Jul 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KCeh Jul 17, 2024 • edited Loading

Choose a reason for hiding this comment

KCeh commented Jul 17, 2024

sonarcloud bot commented Jul 17, 2024

Quality Gate passed

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KCeh Jul 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AsimRibo commented Jul 18, 2024

AsimRibo commented Jul 1, 2024 •

edited

Loading

AsimRibo Jul 2, 2024 •

edited

Loading

KCeh commented Jul 7, 2024 •

edited

Loading

AsimRibo left a comment •

edited

Loading

AsimRibo commented Jul 16, 2024 •

edited

Loading

KCeh Jul 17, 2024 •

edited

Loading

KCeh Jul 17, 2024 •

edited

Loading