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

Coroutine support #388

Open
Pitel opened this issue Mar 6, 2019 · 29 comments
Open

Coroutine support #388

Pitel opened this issue Mar 6, 2019 · 29 comments
Labels
Milestone

Comments

@Pitel
Copy link
Contributor

Pitel commented Mar 6, 2019

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
Copy link
Contributor Author

Pitel commented Jun 12, 2019

@arnaudgiuliani Resolved or wontfix?

@arnaudgiuliani arnaudgiuliani added status:checking currently in analysis - discussion or need more detailed specs type:feature-proposal labels Jun 12, 2019
@arnaudgiuliani
Copy link
Member

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

@Pitel
Copy link
Contributor Author

Pitel commented Jun 12, 2019

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

@arnaudgiuliani arnaudgiuliani added this to Feature proposal or requests in Koin Dev Board Oct 10, 2019
@arnaudgiuliani arnaudgiuliani added status:waiting and removed status:checking currently in analysis - discussion or need more detailed specs status:waiting labels Oct 10, 2019
@arnaudgiuliani arnaudgiuliani moved this from New to Identified in Koin Dev Board Dec 17, 2019
@Swordsman-Inaction
Copy link

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

@marcardar
Copy link

marcardar commented Feb 7, 2020

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()
}

@gorgexec
Copy link

gorgexec commented Mar 6, 2020

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.

@Pitel
Copy link
Contributor Author

Pitel commented Mar 6, 2020

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

@fmatosqg
Copy link

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
Copy link

fmatosqg commented Aug 7, 2020

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

@Pitel
Copy link
Contributor Author

Pitel commented Aug 10, 2020

Great, I see it moves forward! 👍

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

@fmatosqg
Copy link

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.

@jillesvangurp
Copy link

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.

@arnaudgiuliani arnaudgiuliani added this to the 4.0.0 milestone Aug 24, 2022
@stale
Copy link

stale bot commented Jan 22, 2023

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 stale bot added the status:wontfix label Jan 22, 2023
@jillesvangurp
Copy link

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

@arnaudgiuliani arnaudgiuliani added status:accepted accepted to be developed and removed status:wontfix labels Jan 23, 2023
@arnaudgiuliani
Copy link
Member

yes weird.

@jillesvangurp
Copy link

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.

@fmatosqg
Copy link

fmatosqg commented Jan 24, 2023 via email

@jillesvangurp
Copy link

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.

@shubhamsinghshubham777
Copy link

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()
    /* ... */
}

@fmatosqg
Copy link

fmatosqg commented Jan 31, 2023

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.

@arnaudgiuliani
Copy link
Member

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
Copy link
Member

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

@jillesvangurp
Copy link

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.

@arnaudgiuliani
Copy link
Member

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
Copy link
Member

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

@amal
Copy link

amal commented Aug 22, 2023

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

@arnaudgiuliani
Copy link
Member

arnaudgiuliani commented Aug 29, 2023

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

@wreedamz
Copy link

@arnaudgiuliani any updates on your work here?

@haydenkaizeta
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests