koin icon indicating copy to clipboard operation
koin copied to clipboard

Coroutine support

Open Pitel opened this issue 5 years ago • 29 comments

Is your feature request related to a problem? Please describe. I can't write

single {
  someSuspenfingFunctionReturningObject() // ERROR, can't call outside coroutine scope
}

Describe the solution you'd like Allow the single's lambda to be suspending. By adding coroutines to Koin, it might also speed up graph generation by being multithreaded.

Describe alternatives you've considered Workaround I use is to use GlobalScope.async(). But it returns Deferred<Something> and if you use it multiple times, you also has to deal with generics and use names. And you also has to call .await() every f**king time you want to get the Something instance.

Another possible workaround might be to use runBlocking, but it'll degrade performance.

Target Koin project It might be big change, so I suggest 2.x

Pitel avatar Mar 06 '19 09:03 Pitel

@arnaudgiuliani Resolved or wontfix?

Pitel avatar Jun 12 '19 08:06 Pitel

Would make it wont-fix but if you have a clear use-case, let check that

arnaudgiuliani avatar Jun 12 '19 10:06 arnaudgiuliani

Alright. We use MQTT and for some reasons, we have the server url and credentials accesible via internal REST API, so in case of some change, we won't have to re-release our apps.

Now, for REST, we use Retrofit. Retrofit recently got support for coroutines, so my Retrofit interface looks like this:

suspend fun getMqttSettings(): Settings

In order to call the suspend function, you have to be inside some coroutine builder. And because Koin doen't support that directly, we have to use async. So let's make a singleton:

single {
    GlobalScope.async() {
        getMqttSettings()
    }
}

We can also make a singleton for our MQTT client. But we can't do that directly! In order to get result from async, we have to use await. And in order to use await, we have to be in a coroutine. So wrap it in async again. But now we have another problem, because async returns Deferred<T> and Koin isn't very good with generics (U understand why, don't take it as a complaint). So we have to use named, ending up with this:

single(named("settings")) {
    GlobalScope.async() {
        getMqttSettings()
    }
}

single(named("mqtt")) {
    GlobalScope.async() {
        MqttClient(
            get<Deferred<Settings>>(named("settings")).await()
        )
    }
}

And you have to this named/async/await/Deferred dance every time you start using coroutines.

Wouldn't it be nice, if you could just write something this:

coSingle {
    getMqttSettings()
}

coSingle {
    MqttClient(
        get()
    )
}

(The co prefix is what MockK is using for it's coroutine supported functions.)

Pitel avatar Jun 12 '19 10:06 Pitel

IMHO, this is out of the scope of a dependency injection library.

Swordsman-Inaction avatar Dec 19 '19 11:12 Swordsman-Inaction

This is how I deal with this (feedback much appreciated):

single(named("instanceProvider")) {
    suspendableLazy {
        createAndInitInstance() // returns instance of MyClass
    }
}

and then:

private val instanceProvider by lazy {
   get<SuspendableProvider<MyClass>>(named("instanceProvider"))
}
suspend fun getInstance(): MyClass? = try {
    instanceProvider.get()
} catch (e: Exception) {
    Log.e("di", "unable to create instance", e)
    null
}

where using some common code:

interface SuspendableProvider<T> {
    suspend fun get(): T
    val isCompleted: Boolean
}
fun <T> suspendableLazy(provider: suspend () -> T) = object : SuspendableProvider<T> {
    private val computed = GlobalScope.async(start = CoroutineStart.LAZY) { provider() }
    override val isCompleted: Boolean
        get() = computed.isCompleted
    override suspend fun get() = computed.await()
}

marcardar avatar Feb 07 '20 05:02 marcardar

I think, this is really not the responsibility of Koin. In this case, coroutine need is because of complexity of object building, but Koin deals with another activities: object resolving and injecting. First of all, the object building depends on app logic and may be handled by different ways and vary from app to app. DI library shouldn't know about such details. So, as example, you may initially preconstruct your complex object and then load Koin feature module that provides required object without any coroutine usage in Koin.

gorgexec avatar Mar 06 '20 10:03 gorgexec

@gorgexec Ok, that's a valid point, but...

You can't preconstruct them for Koin, because the construction is asynchronous. And if any other object would try to get instance of the object before it's constructed, it'll fail and crash.

What I think is, that K in Koin means Kotlin. And coroutines are now part of the Koltin's stdlib. So it would be nice if Koin would support them. Heck, you can even make the whole startKoin(), get()/inject()/etc. methods suspending (and asynchronous), so the whole dependency graph creation and crawling won't block the main thread. But I understand it's not an easy task.

Pitel avatar Mar 06 '20 10:03 Pitel

I didn't really understand @marcardar suggestion, but it looks reasonable to me that something like that could be inside koin lib and proof-read by other developers instead of sitting inside my project and "working on my test devices".

I'd bring my problem that could use a similar solution. I have an object that has a bunch of ThreeTenAbp date formatters, and they are sloooow to instantiate. In a previous shorter version it took 90ms to instantiate. It's just a handful of

 val weekdayFormatter: DateTimeFormatter = DateTimeFormatter.ofPattern("EEEE")

that I put together in a dedicated FixtureDateFormatterUtil class, and I'd love to make something like

    class FixtureDateFormatterUtil {

        private constructor()

        companion object {
            suspend
            fun factory(): FixtureDateFormatterUtil =
                  withContext(Dispatchers.Default) {
                       FixtureDateFormatterUtil()
                }
        }

But then I can't easily use it with koin.

fmatosqg avatar Jul 30 '20 07:07 fmatosqg

@marcardar I have a proof of concept code (still needs quite some cleaning at https://github.com/fmatosqg/koin/tree/factory_suspend).

The basic idea is having a new dsl factorySlow (yep it needs a better name, or it could be like suspendableLazy suggested above) that will tag that Bean with options.coroutine = true, in a similar fashion as options.isCreatedAtStart for eager modules. Then all of those beans will be initialized in parallel when the app starts.

I made a number of assumptions so far

  • only singletons are created. We don't want to incurr in double penalties, right?
  • Not worring how long it takes. What should we do if it takes too long? At the same time, what's the current standing on functions that take too long on our current lib version?
  • once they're created they are indistinguishable from other singletons
  • it's initialized about the same time as eager instances. Optionally we could start before eager and finish after eager
  • until we're done creating them we wait. I'm not sure what would happen if we get circular dependencies and complicated graphs, but that needs some investigation. Any information about graph resolution and order of object instantiation would help me a lot.
  • I didn't worry yet about which Dispatchers to use, but I guess we can let app developers do withContext and choose themselves.

also

  • nothing prevents us from initializing eager instances in parallel as well, but that's for later

The heart of it is at a variation of ::createEagerInstances that looks for options.coroutine and runs things in parallel. get2 here is a suspending sibling of get, who calls a suspending version of create and so forth.

        instances.values.filterIsInstance<SingleInstanceFactory<*>>()
            .filter { instance -> instance.beanDefinition.options.coroutine }
            .map { instance ->
                CoroutineScope(Dispatchers.Default)
                    .async {
                        instance.get2(InstanceContext(_koin, _scope))
                    }
            }
            .let {

                runBlocking {
                    it.forEach {
                        it.await()
                    }

                }	                }
            }

The current version adds a typealias SuspendDefinition<T> = suspend Scope.(DefinitionParameters) -> T that gets passed as a parameter along the current definition object through all the layers from module to InstanceFactory. This def has lots of room for improvement, but not worried about that now.

I'm interested in knowing about the performance of running runBlocking lots of times, that may help simplify the internal API. But again, not worring about this yet.

Keen to hear ideas or feedback.

fmatosqg avatar Aug 07 '20 09:08 fmatosqg

Great, I see it moves forward! :+1:

  • Be careful with runBlocking, as it is, obviously, blocking, and it should not be run on main/UI thread.
  • it.forEach { it.await() } -> it.awaitAll() and you also get nice list of results.

Pitel avatar Aug 10 '20 07:08 Pitel

Yes, the runBlocking and the await or awaitAll is at the heart of the controversy. They may disapear from code, but another form of waiting would need to replace them.

fmatosqg avatar Aug 11 '20 00:08 fmatosqg

We do the following in our kotlin-js app:

GlobalScope.launch {
   startKoin {
      modules(searchModule)
   }
   // asynchronously initializes our localization, which does some IO
   GlobalContext.get().declare(LocalizationUtil.load())
   ...
   // render the UI
}

Basically, this creates a co routine scope. You then initialize koin the normal way, and then you declare some new objects via suspend functions. This works because inside co-routines, you have before/after semantics. So the localization is guaranteed to finish loading before the UI renders. runBlocking is not actually a thing in kotlin-js so you would not be able to use that. And threads are not a thing either so you can't await a co-routine from the main thread without blocking that thread (which is why runBlocking is not a thing.

This works fine but it's a bit klunky; IMHO it shouldn't be that hard to support a nicer version of this in Koin. UIs are inherently asynchronous and so is their lazy initialization.

jillesvangurp avatar Jul 26 '22 09:07 jillesvangurp

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stale[bot] avatar Jan 22 '23 03:01 stale[bot]

@arnaudgiuliani it seems the stale bot just added the wontfix label. Seems a bit harsh?

jillesvangurp avatar Jan 22 '23 09:01 jillesvangurp

yes weird.

arnaudgiuliani avatar Jan 23 '23 14:01 arnaudgiuliani

Building on the example I posted above, what's probably needed is a few suspending extension functions that allow people to call suspending logic. I'm not familiar enough with Android to know how you'd do that exactly in that context. I think with ktor and browser based things a variation of what I did should work fine. Either way, the challenge is avoiding duplicating all the functionality in suspending and non suspending ways. Or just make suspending logic the default.

jillesvangurp avatar Jan 23 '23 15:01 jillesvangurp

I don't think that sample will work for Android, at best I'd expect a race condition that "works".

Android doesn't need to ensure the global scope is finished by the time the first frame is rendered (and if we were to enforce that we may be killing the value of the feature),, - what it needs is whatever part of the graph is instantiated when it wants. But if it did, I would do it with run Blocking right after launch. Upside: spin a particular function in a particular dispatcher. But I don't see a strong need for this, I'd let the app developer do all of this inside his own written method.

It's been very long since I started looking into this and honestly don't remember the use case I was aiming for.

On Tue, 24 Jan 2023, 02:07 Jilles van Gurp, @.***> wrote:

Building on the example I posted above, what's probably needed is a few suspending extension functions that allow people to call suspending logic. I'm not familiar enough with Android to know how you'd do that exactly in that context. I think with ktor and browser based things a variation of what I did should work fine. Either way, the challenge is avoiding duplicating all the functionality in suspending and non suspending ways. Or just make suspending logic the default.

— Reply to this email directly, view it on GitHub https://github.com/InsertKoinIO/koin/issues/388#issuecomment-1400503720, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABI4Y4OJPGOUHPHYD3TNSBLWT2NDBANCNFSM4G4B2WOA . You are receiving this because you commented.Message ID: @.***>

fmatosqg avatar Jan 24 '23 08:01 fmatosqg

I think it's not so much a matter of blocking but just guaranteeing that certain things happen before other things. I just looked at the docs, and it seems on Android you just initialize the whole context in an onCreate handler (not suspending). Effectively the application does not start until that finishes and koin has been initialized. So, I don't think it would be a big deal to surround the koin start with a runBlocking {}. Nothing happens in any case before onCreate finishes. All runBlocking would do there is give you the chance to call some suspending things. Of course you do have to be careful with doing slow things there. But that is true regardless. Likely people are possibly still using some blocking IO on Android still? That would make sense given the Java legacy.

The issue is that the various lambdas in the koin dsl are not suspending. That's what I work around in the code above by calling declare. So, a few variations of the DSL functions that can work with suspending functions would be helpful. In the browser we don't actually have runBlocking, so we just 'launch' the whole application in the GlobalScope, which works fine. It's still sequential but it gives us the possibility to do non blocking IO as part of our initialization.

jillesvangurp avatar Jan 24 '23 08:01 jillesvangurp

Another common use case for having coroutine support in Koin would be instantiating SQLDriver for JS in SQLDelight. Ref: https://cashapp.github.io/sqldelight/1.5.4/js_sqlite/

Code sample in the ref link above:

// As a Promise
val promise: Promise<SqlDriver> = initSqlDriver(Database.Schema)
promise.then { driver -> /* ... */ }

// In a coroutine
suspend fun createDriver() {
    val driver: SqlDriver = initSqlDriver(Database.Schema).await()
    /* ... */
}

shubhamsinghshubham777 avatar Jan 26 '23 06:01 shubhamsinghshubham777

Nothing happens in any case before onCreate finishes. All runBlocking would do there is give you the chance to call some suspending things. Of course you do have to be careful with doing slow things there. But that is true regardless.

true. Though for android we'd need to use at least 2 CPU cores to take advantage of it, or else there's no upsides.

Likely people are possibly still using some blocking IO on Android still?

Let's keep blocking IO out of scope. Android is multi-threaded, and best practice is not do long operations on main thread, IO or not, whether that's coroutines, RxJava or bare threads.

fmatosqg avatar Jan 31 '23 07:01 fmatosqg

One of the my point here, is to help also introduce coroutines in the core engine architecture of Koin to avoid pessimist lock under thread (like currently).

If I wrap up the need of coroutines :

  • suspend call to definition/ definition using coroutines (ktor)
  • internal engine

This will land under the version 4.0 of Koin. I will need beta testers ;)

arnaudgiuliani avatar Feb 01 '23 16:02 arnaudgiuliani

One first proposal would be for example to help load koin on another thread than the main one. The idea: unlock the problem of having more and more definitions & modules to read at start. This way, this is not blocking anymore.

At least we need to have some minimum definitions ready. Then 2 possibilities:

  • load sync way (classic way of loading) - required for the minimum start
  • load async on other thread - the rest of the app

arnaudgiuliani avatar Mar 06 '23 08:03 arnaudgiuliani

The point of co-routines isn't necessarily doing things on threads but doing asynchronous/non blocking things. The browser actually has no threads (well, it has workers but that's a bit of a niche thing). But everything is promise based in a browser, which is why co-routines are a really nice abstraction there.

And if you have a lot of initialization that does things with promises and co-routines, having support for that in Koin is nice.

The issue that I had with koin was simply that the koin dsl doesn't support suspending code blocks. I worked around it by calling things on the context directly in a GlobalScope.launch {...} ugly but effective.

jillesvangurp avatar Mar 07 '23 08:03 jillesvangurp

The issue that I had with koin was simply that the koin dsl doesn't support suspending code blocks

We also need API to request dependencies in async way then, to have the whole "suspend" stuff working

arnaudgiuliani avatar Mar 08 '23 08:03 arnaudgiuliani

I've already added support for the Koin extension for modules loading via coroutines. This is a beginning 👍

arnaudgiuliani avatar May 10 '23 15:05 arnaudgiuliani

Neat support for suspending is available in kotlin-inject. Maybe it can be helpful.

amal avatar Aug 22 '23 15:08 amal

yes, they offer the possibility to create a definition within a coroutine context then 🤔

For the first proposal above, it is more about loading content in a coroutine context

arnaudgiuliani avatar Aug 29 '23 13:08 arnaudgiuliani

@arnaudgiuliani any updates on your work here?

wreedamz avatar May 21 '24 16:05 wreedamz

I am interested in the current progress and the suggested pattern in the meantime

haydenkaizeta avatar Jun 03 '24 20:06 haydenkaizeta