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.

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

NPE while using by inject, due to lazy initialization in JobIntentService(koin v2.1.5) #797

Closed
adiamartya opened this issue May 4, 2020 · 25 comments
Labels
android important 🔥 status:checking currently in analysis - discussion or need more detailed specs type:issue

Comments

@adiamartya
Copy link

I am using joinAll inside a jobintentservice. Also, i m initializing global objects via inject. This was working fine on v2.0.1. When i upgraded it to v2.1.5, two crashes have been logged in crashlytics

Fatal Exception: kotlin.KotlinNullPointerException
at kotlin.UnsafeLazyImpl.getValue(UnsafeLazyImpl.java:81)
at com.android.david.onboarding.services.SuggestionsService.getSplashRepo(SuggestionsService.java:1)
at com.android.david.onboarding.services.SuggestionsService.access$getSplashRepo$p(SuggestionsService.java:16)
at com.android.david.onboarding.services.SuggestionsService$onHandleWork$1$2.invokeSuspend(SuggestionsService.java:41)
at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(BaseContinuationImpl.java:33)
at kotlinx.coroutines.DispatchedTask.run(DispatchedTask.java:56)
at kotlinx.coroutines.scheduling.CoroutineScheduler.runSafely(CoroutineScheduler.java:571)
at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.executeTask(CoroutineScheduler.java:738)
at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.runWorker(CoroutineScheduler.java:678)
at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.run(CoroutineScheduler.java:665)

@arnaudgiuliani arnaudgiuliani added the question Usage question label May 25, 2020
@lateplate
Copy link

Seeing something similar with 2.1.5, though I'm not entirely sure if it's Koin or something else. We are doing a fairly straightforward inject in a class that extends KoinComponent, and for some reason we are hitting an NPE when trying to inject that object.

Crashes seem to be random, especially for a class that is constantly in use (used in every Retrofit/API call we make). If it were something more deep rooted, I would think we would see thousands of crashes, but only a couple dozen here.

Stack:

kotlin.KotlinNullPointerException: null
    at kotlin.UnsafeLazyImpl.getValue(Lazy.kt:81)
    at com.basecamp.hey.api.AuthInterceptor.getSession
    at com.basecamp.hey.api.AuthInterceptor.setRefreshInProgress
    at com.basecamp.hey.api.AuthInterceptor.getSession(AuthInterceptor.kt:0)
    at com.basecamp.hey.api.AuthInterceptor.getResponse(AuthInterceptor.kt:60)
    at com.basecamp.hey.api.AuthInterceptor.intercept(AuthInterceptor.kt:32)
    at okhttp3.internal.http.RealInterceptorChain.proceed(RealInterceptorChain.kt:100)
    at okhttp3.internal.connection.RealCall.getResponseWithInterceptorChain$okhttp(RealCall.kt:197)
    at okhttp3.internal.connection.RealCall$AsyncCall.run(RealCall.kt:502)
    at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1167)
    at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:641)
    at java.lang.Thread.run(Thread.java:919)

Line 60 of AuthInterceptor is a reference to a Session object that is created like so:

    private val session by inject<Session>()

Any help is appreciated!

@arnaudgiuliani arnaudgiuliani added android status:checking currently in analysis - discussion or need more detailed specs type:issue and removed question Usage question labels Jul 21, 2020
@arnaudgiuliani
Copy link
Member

do you have more stack?

@RamiTrabelsiAndroid
Copy link

RamiTrabelsiAndroid commented Jul 28, 2020

We're encountering a very similar problem, just after upgrading Koin from v1.0.2 to v2.1.6, new crashes has popped up on our Crashlytics. Crashes seems to be at the lines using inject() like this :

private val database: AppDatabase? by inject()

The 2 stack traces that we have, are happening when injecting databases or repositories with Dao classes (constructor injected) -don't know if it's coincidence for this-.

We're using Room, I paste 3 crashs stack traces below :

Fatal Exception: kotlin.KotlinNullPointerException

kotlin.UnsafeLazyImpl.getValue (UnsafeLazyImpl.java:81)

a.b.c.d$XXX$.getDatabase (XXX.java:3)

a.b.c.d$XXX$p (XXX.java:23)

a.b.c.d$XXX$method$1.invokeSuspend (XXX.java:122)

a.b.c.d$XXX$$loadFromDb$1.invoke (XXX.java:10)
Fatal Exception: kotlin.KotlinNullPointerException

kotlin.UnsafeLazyImpl.getValue (UnsafeLazyImpl.java:81)

a.b.c.d$YYY$.getUserSessionRepository (YYY.java:2)

a.b.c.d$YYY$.restoreSessionSync (YYY.java:158)

a.b.c.d$YYY$restoreSession$2.invokeSuspend (YYY.java:137)

a.b.c.d$YYY$restoreSession$2.invoke (YYY.java:10)

Thank you in advance for taking a look at this serious problem affecting big number of our users.

@lateplate
Copy link

do you have more stack?

@arnaudgiuliani That's about all we have. It's coming from a crash log and I can't find a way to locally reproduce it where I might be able to grab more logs.

@shaileshmota
Copy link

shaileshmota commented Jul 29, 2020

@arnaudgiuliani for the last trace : #797 (comment)

We are creating a module for Room DB like :

val databaseModule = module {
    single {
        Room.databaseBuilder(get(), MyDb::class.java, "my_db")
            .fallbackToDestructiveMigration()
            .build()
    }
}

Further in a ViewModel we inject it like :

class MyViewModel : ViewModel(), KoinComponent {
    private val database by inject<MyDb>()
    .
    .
    .
}

It seems like further accessing the instance database produces NPE.
Crash is seen just after upgrading Koin from v1.0.2 to v2.1.6

@shaileshmota
Copy link

shaileshmota commented Aug 19, 2020

This crash is quite considerable on our crash stats. Due to lack of activity on this thread we will simply downgrade the version to see where it works. @lateplate can you tell us from what version did you upgrade to 2.1.5 ?
It seems like it is working fine for @adiamartya for version 2.0.1

We have tried downgrading from 2.1.6 -> 2.1.4, but still see the crash.

@lateplate
Copy link

@shaileshmota We kept pretty up to date, so I believe we were on 2.1.4 previously. But do note that our crashes are pretty sporadic and not super frequent.

@RamiTrabelsiAndroid
Copy link

Hello @lateplate, you already had crashes with version 2.1.4 or everything was fine with that version ?

@lateplate
Copy link

lateplate commented Aug 20, 2020 via email

@ilya-shknaj
Copy link

ilya-shknaj commented Sep 25, 2020

After update from 2.0.0 to 2.1.5 we see a lot of crashes in production.
During last 30 days ~ 5000 crashes. Unfortunately, I don't have steps for reproducing.

Screen Shot 2020-09-25 at 10 17 19

But, I've reviewed code and found a critical change that possibly leads to this issue
45ce7d2
It appeared in 2.1.0-aplha9
@lateplate , @arnaudgiuliani Could you check that?

@egorikftp
Copy link

@arnaudgiuliani Any updates?

@arnaudgiuliani
Copy link
Member

I need your stacktrace and code usage. Hard to say like that :/

@egorikftp
Copy link

@arnaudgiuliani The stacktrace the same as mentions above in the issue. We have made the research and found that one of the reason of this bug is making lazy(LazyThreadSafetyMode.NONE).

Could you please explain the reason of this change?

@arnaudgiuliani
Copy link
Member

lazy(LazyThreadSafetyMode.NONE) optimisation proposal I guess. 🤔

It's something I can look at.

@Qoxis
Copy link

Qoxis commented Oct 29, 2020

We have the same errors since we use Koin to init Room and our DAOs, It's now the first error in crashlytics.
Here is the init code:

            modules(
                module {
                    single {
                        Room.databaseBuilder(
                            androidApplication(),
                            AppDatabase::class.java,
                            "my-db"
                        ).fallbackToDestructiveMigration().build()
                    }
                    single { get<AppDatabase>().offerDao() }
                    single { get<AppDatabase>().userDao() }
                    single { get<AppDatabase>().websitesDao() }
                }
            ) 

And we get;

Fatal Exception: java.lang.NullPointerException

kotlin.UnsafeLazyImpl.getValue (UnsafeLazyImpl.java:81)
getOfferDao (OffersRepository.java:2)
OffersRepository.access$getOfferDao$p (OffersRepository.java:14)
OffersRepository$metaMosaic$1.invoke (OffersRepository.java:45)
OffersRepository$metaMosaic$1.invoke (OfferscRepository.java:14)
ResultKt$resultLiveData$1.invokeSuspend (ResultKt.java:26)
kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith (BaseContinuationImpl.java:33)
kotlinx.coroutines.DispatchedTask.run (DispatchedTask.java:56)
kotlinx.coroutines.scheduling.CoroutineScheduler.runSafely (CoroutineScheduler.java:571)
kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.executeTask (CoroutineScheduler.java:738)
kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.runWorker (CoroutineScheduler.java:678)
kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.run (CoroutineScheduler.java:665)

Looking forward an update on this error, thanks :)

@arnaudgiuliani
Copy link
Member

I will push a rc-4 with my last fix, can anyone test it?

@Qoxis
Copy link

Qoxis commented Oct 30, 2020

Yes we can make an internal build and then deploy it to 5% of our base and check if it still happen.

@arnaudgiuliani
Copy link
Member

rc-4 is online!

@trashkalmar
Copy link
Contributor

trashkalmar commented Nov 2, 2020

Please, consider adding extensions, say threadUnsafeInject(), to use inside UI, or when it's known that there are no threading problems, so no need in synchronization overhead.

@arnaudgiuliani
Copy link
Member

to people that have lazy evaluation problem, is it mostly located in Android Service components?

Not sure, it's super impecting for now to have standard lazy resolution. But finally, it seems to be there only on Service. Then I would propose to come back to Lazy(NONE) on UI extensions. Service would keep standard Lazy evaluation.

@arnaudgiuliani
Copy link
Member

arnaudgiuliani commented Nov 3, 2020

The trade-off I can propose is :

  • have Lazy(None) on Android UI extensions (inject, viewModel ...)
  • keep default Lazy strategy for everything else as default (Service, Worker, KoinComponent ...)

Android UI Thread is one of the rare place where we have only 1 thread.

What do you think?

@trashkalmar
Copy link
Contributor

trashkalmar commented Nov 3, 2020

It may be confusing to have the tool that works differently in different places. Actually, it'd be nice to have a manual control on the threading while injecting anyway. For my projects, I've made two extension methods for this purpose, so I'm always sure about the threading:

// App's Koin
@OptIn(KoinApiExtension::class)
interface AppKoin : KoinComponent {
  override fun getKoin() = ...
}

// Manual threading control with NONE as default
inline fun <reified T : Any> AppKoin.inject(qualifier: Qualifier? = null, mode: LazyThreadSafetyMode = LazyThreadSafetyMode.NONE) =
    lazy(mode) {
      getKoin().get<T>(qualifier)
    }

// Thread-safe injection
inline fun <reified T : Any> AppKoin.safeInject(qualifier: Qualifier? = null) =
    inject<T>(qualifier, LazyThreadSafetyMode.SYNCHRONIZED)

P.S. In my projects, the vast majority of injections is made without synchronization, as I'm sure what threads access the injected resources. It may not be true for everybody though.

@arnaudgiuliani
Copy link
Member

arnaudgiuliani commented Nov 3, 2020

If we check Android sources, they don't use Lazy - https://cs.android.com/androidx/platform/frameworks/support/+/androidx-master-dev:lifecycle/lifecycle-viewmodel-ktx/src/main/java/androidx/lifecycle/ViewModelProvider.kt;l=41?q=%20ViewModelLazy

it's just a cached response. As in UI thread, we don't need more.

For now, pessimist approach help everyone to get the right data.

As an improvement, UI API could have something inspired by ViewModelLazy

@arnaudgiuliani
Copy link
Member

closing this issue for now. Let's keep in touch for better Lazy implementation for UI.

@Qoxis
Copy link

Qoxis commented Nov 10, 2020

On our side, no more errors since the rc-4. Thanks again !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
android important 🔥 status:checking currently in analysis - discussion or need more detailed specs type:issue
Projects
None yet
Development

No branches or pull requests

9 participants