[Koin 3.0] Multiplatform

[drieks]

Is your feature request related to a problem? Please describe.
Koin Suppor for this platforms can be discussed in this Ticket:

• Kotlin/Native Linux/OSX/Windows
• Kotlin/Native iOS
• Kotlin/Native Android
• Kotlin/JS Browser
• Kotlin/JS Node

Describe the solution you'd like
Koin should support for other platforms.

Describe alternatives you've considered
A clear and concise description of any alternative solutions or features you've considered.

Target Koin project
Koin Core and Koin Test, maybe others

[drieks]

Hi @kpgalligan, I think the best place to discuss is here.

[drieks]

In PR #519, I already implemented multiplatform support for koin-core. It is working fine for Kotlin/JVM and Kotlin/LinuxX64. It should be easy to add other platforms. Currently, I have no time to work on it. Maybe in August I can find some time to add Kotlin/JS Browser support.

Next steps are:

• more platforms (just copy everything in src/linuxX64Main to src/newPlatformMain)
• multiplatform support for koin-test
• I added some changes to readme.md, that should be moved to changelog.md
• Someone should review my changes. :-)
• maybe there are other changes, that should be added to changelog.md

[kpgalligan]

It works if you only use one thread. It very much does not if you use multiple, which is what I'm looking at now. I did a POC of this a few months ago, and you need fairly significant internal changes, but it's doable.

[kpgalligan]

Here's the current work in progress: https://github.com/drieks/koin/compare/3.0.0...touchlab:kpg/multiplatform?expand=1

[kpgalligan]

To start unwinding what I'm talking about, take KoinMultiPlatform. Pulling collections from there is good, but any mutable collection for native needs to be "freezable", so they kind of all need to come from there. Your implementation of emptyMutableMap returned a HashMap on native, but that won't work once you need to freeze things. Alternatively, all Koin access could happen in the main thread, but I assume that's not how Koin is designed to be used.

[kpgalligan]

Part of my resistance here is I'm not really a Koin user. I've been doing Kotlin Native for the last 1.5 years pretty much full time, so if something doesn't yet exist in that world, there's a good chance I'm not super experienced with it.
Anyway, for collections, I'm using Stately collections, which freeze anything put in them. I have a much more performant version of them in the works, but that version will act the same (freeze everything).
In places with var I'm generally replacing with AtomicReference (sometimes by way of a delegate). Locks are somewhat complicated because you need to close them when done, and it looks like Scope needs that. It's there, but I'm not super happy about how it works now. If 2 threads call into it and one of them closes the lock, bad things might happen.

[kpgalligan]

Cross cutting all of this is Kotlin Native has a "relaxed" memory mode coming, but I'm not sure when, and I'm not sure how "relaxed". It may mean you don't need to freeze everything, but it just seems unlikely that Kotlin Native's runtime will simply dump all of the concurrency work, but stranger things have happened.

[kpgalligan]

Assuming Kotlin's thread/state model doesn't change significantly, I think we should also talk about another scope type. single is global, factory is new each time, scope is scoped. I feel like there's maybe a threadLocal in there. Creates instances shared but thread confined? Maybe. Certainly not a "now" thing, but something to think about in Kotlin Native land.

[drieks]

Yes, I also thought about a ThreadLocal, that is also easy to implement for JVM. For JavaScript this can be an implementation using a normal var, because js is always single threaded.

Like you, I'm not a real koin user. I'm writing an application with JVM and Native port, later maybe js/browser and want to include dependency injection.

Releated to #505: When we change koin to be generally immutable, this would be much easier for native support. But lasy creation is not possible with an immutable state. Another option may be function that will take an immutable koin state and will return a new immutable state in combination with a (maybe lasy created) bean. This may solve problems like this one, but it also makes things more complicated: ce0587d

I'm not sure if @SharedImmutable for the global logger is a good selection, maybe the logger implementation does not work when frozen. Maybe a thread local is a better choise?

In the same commit, var app in GlobalContext should be a thread local, what do you think?

[drieks]

Maybe the easyest solution is to support only thread local koin instances in 3.0.0 for native?

Generally, I think the memory model "mutable XOR global" (https://github.com/JetBrains/kotlin-native/blob/master/IMMUTABILITY.md) is a good selection to prevent two threads accessing the same variable.

I don't understand the use case for stately, why do you want to freeze a collection? I thought, as soon as I pass a memory structure (like a collection) to a global variable, it is automatically frozen? I thought, that it is not possible that two threads can access the same mutable data? But I'm not very familiar with kotlin native.

I think we should be very careful when introducing new dependencies, a JVM or android developer, for example, is maybe not very happy when we enforce them to use an auto freeze library. Especially, when freezing is not a normal behavior on this platforms. But, as I already mentioned, I'm not very familiar with native.. Maybe we can include this dependency only for kotlin/native?

[kpgalligan]

Answers to both comments:

"Yes, I also thought about a ThreadLocal".

I'd put this in the "later" box. Not super important.

"Like you, I'm not a real koin user. I'm writing an application with JVM and Native port"

We should get a koin expert into the conversation I think. I'm not a koin expert, but at the risk of sounding full of myself, I am a Kotlin Native expert.

"Releated to #505: When we change koin to be generally immutable"

Is that happening or is that just a feature proposal? When? I know this sounds a little aggressive, but I want to let my team play with something like next week, so anything that would need to wait on a major re-architecture of Koin is of less immediate interest to me. Happy to discuss, though!

"I'm not sure if @SharedImmutable for the global logger is a good selection, maybe the logger implementation does not work when frozen. Maybe a thread local is a better choise?"

Frozen only affects native, and an experienced native dev should know how to deal with this. However, if thread local was desired, that would technically work, but you'd need to configure your logger with a factory rather than setting it directly, because you can't set the logger on each thread (unless you loop through all threads up front and set them, and don't create any more, but that would be a disaster), and the factory, presumably a lambda, would itself need to be frozen, because the factory would need to be globally visible.

"Maybe the easyest solution is to support only thread local koin instances in 3.0.0 for native?"

I would imagine this would confuse and upset many. "single" wouldn't be really single, right? I also don't think it's particularly easy. You'd either be locked to the main thread, or need to configure koin with a factory. Native acting differently than the JVM on a fundamental level is a non-started (to me).

"But I'm not very familiar with kotlin native."

Kotlin native state and concurrency will be completely foreign to you. It'll be very difficult to explain my changes if you don't understand it. The kotlin native docs are a good intro, but you need to spend some time with it. I have some supplemental blog posts.

https://medium.com/@kpgalligan/kotlin-native-stranger-threads-ep-1-1ccccdfe0c99
https://medium.com/@kpgalligan/kotlin-native-stranger-threads-ep-2-208523d63c8f

"I don't understand the use case for stately" Stately exists to facilitate common code with kotlin native. It is especially helpful for creating service objects that need to live globally (like what Koin presumably manages). There's a 3rd blog post in that series I had intended to write explaining stately, but I haven't gotten around to it.

"why do you want to freeze a collection?" Well, I don't. That's why stately collections exist. If you want to be able to let multiple threads access the same collection, then the collection is frozen. Stately's collections use AtomicReference to allow mutability inside of a frozen structure. If you use collections like koin does now, they will be frozen automatically, and crash when you attempt to mutate them. If Koin was immutable after initialized it wouldn't be a problem, but it is not internally.

The thing I think you need to understand about Kotin Native's "immutability" is that while frozen structures are immutable, AtomicReference is a special case. It allows you to mutate it's state, even when frozen.

"I think we should be very careful when introducing new dependencies" Of course. This is a work in progress.

"a JVM or android developer, for example, is maybe not very happy when we enforce them to use an auto freeze library" that's not how it works. JVM is a noop. There's no freezing on the JVM.

"Maybe we can include this dependency only for kotlin/native?" That is generally the plan. We did similar in Sqldelight. However, I'm experimenting with making the JVM side disappear with typealiase, so it would be invisible, but I'm not sure if that'll actually work, in which case it would be mostly copy/paste.

I've expanded the testing to test threads far more, and should have 'koin-test' done tonight as well. I'll just make a "proof of concept" and everybody can discuss I guess?

[kpgalligan]

Now, having said all that, there is the new thing called “relaxed mode“. It will be more classical concurrency and state model. That’s a work in progress but will significantly impact the ecosystem. I am still digesting what it will mean generally, but I think the native implementation of Koin can be created to work in both without too much differentiation.

Some mention in kotlin slack: https://kotlinlang.slack.com/archives/C3SGXARS6/p1562490132443500?thread_ts=1562410455.425000&channel=C3SGXARS6&message_ts=1562490132.443500

[kpgalligan]

Updated. I moved 'stately' to native only. As mentioned, freeze does nothing to the jvm (or js), but native needs it in places.

I've added Apple-based (Darwin) and Linux-based targets. I have windows in there too, but not complete yet. I have a Parallels setup for that, but need to figure out how to get system properties.

I've mangled the tests quite a bit to force config and running to be done on different threads. That can all be rolled back, but native needs to be concerned with this. The JVM doesn't seem to test for concurrent access and I'm a little concerned there are spots for possible race conditions. This is why native has such restrictive rules. They make most cases of casual race conditions impossible, but I digress.

On that note, there are places where JVM was previously a var and is now an AtomicReference. Similarly, collections are more concurrent. These changes may have an impact on JVM performance. I've been focused on getting everything to work. Looking at JVM changes and rolling back issues is next, although mutable state isn't great. See above.

The mutexes used with native need a close call. I'm still working this out. In tests, there seem to be waiting calls to the mutex in Scope after close is called, which was causing a seg fault, but that may have been unrelated. Just mentioning it as I need to return to it.

JS is also implemented, but I can't get the tests to run. This is a Kotlin config thing, not a Koin thing. I'm far more experienced with native than JS, and it's a little lower priority for me this week, but will get back to it.

As for 3.x, other than a source reorg, this doesn't feel like a radically different architecture. I'd advocate for sooner rather than later. IE 2.1 instead of 3.0. For 3.0 I'd explore more explicit immutability, different scopes and extensions for native, etc. My sense of urgency for multiplatform libs is quite high, but that's also 100% of what I do right now. Thoughts?

I'm going to publish a version so we can play with it. To be clear, the goal is not to maintain a forked version, just see how native works in apps. We're also working and shipping client apps, so I want to be able to use this asap.

[drieks]

Hi @kpgalligan,

unfortunately, I have almost no time for the next few weeks... But I try to help if I can. A more immutable koin implementation can also be Version 4.0, I think the Number is not so important. I think native and js support is a big addition to koin that justifies a major version update. I don't know what @arnaudgiuliani plan are, maybe he will directly start to work on 3.1 and koin 2.1 will never be created?

We can also try to release a 3.0 version of koin without digging so much about performance. JVM/Android only koin users can still use 2.0 until we release a 3.0.1 or 3.1 version with better performance? Nevertheless, we will need to publish a preview version I think, so that other koin users can try out the changes in 3.0.

Did you see my comments about close here? drieks#1
Maybe not, as I have just seen a few seconds ago, I opened this PR and not you...
I think we don't need a close function, we can call the required code automatically in invoke using try/finally. Requesting manually mutex management is to error prone, we should not expose stuff like this I think.

I'm also don't plan to start a fork, let's see what @arnaudgiuliani thinks about all of this :-)

[kpgalligan]

"We can also try to release a 3.0 version of koin without digging so much about performance. JVM/Android only koin users can still use 2.0" I'm OK with this. Just don't want multiplatform to go in a many months holding pattern.

"Did you see my comments about close here? drieks#1"

I don't see any comments in the PR. I'm guessing they're not visible to me.

"I think we don't need a close function, we can call the required code automatically in invoke using try/finally. Requesting manually mutex management is to error prone, we should not expose stuff like this I think."

Not sure what you mean here. The close is on the mutex ultimately, which shares the lifecycle of whatever holds it. The problematic case is Scope. When the scope is closed, we need to also close the mutex. I'm not clear on what you're thinking with regards to try/finally. If we dealloc the memory for the mutex and somebody tries to access it, the try block will not help you, just FYI. This is native pthread stuff now.

"Requesting manually mutex management is to error prone, we should not expose stuff like this I think." It's not really being "exposed" per say, but we need a mutex, and it needs to be closed. I have thoughts on how to manage the lifecycle of that alternatively, but at the end of the day, if we need to synchronize something, that's a mutex.

[arnaudgiuliani]

Hi,

first, clearly a big PR @drieks! Good job 👍. I need a bit of time to check that those days :) (lots of files impacted)

Apart from the main "JVM" class usages (KClass/Class, collections ...), one of the main problems is the graph mutability & multithreading of the core?

We use static holder to keep it somewhere, in the GlobalContext by default. The entire graph is purely dynamic as we don't know yet what we will executed.

We also have to keep in mind that we have to preserve Koin for the JVM "as is" for any users. 95% of the users come from the JVM.

Tell me what components you need to change to adapt it for Kotlin MP?

[drieks]

This PR contains mainly compile fixes that are required by the common/jvm/native split, no structual changes.

The first part contains only some updates, warning fixes and auto code format cleanups. This can also be "backported" to koin 2.0 (PR #518).

This commits of the first real MP step are prefixed with "MP step 1". Everything is just moved from src/main/kotlin and src/test/kotlin to src/jvmMain/kotlin and src/jvmTest/kotlin. Gradle was changed to MP build with just jvm support.

Commits labeled "MP step 2" will add support for a common module, anything that is possible is moved to src/commonMain/kotlin and src/commonTest/kotlin. There is still only JVM support, but all JVM-specific dependencies will cause a compile error in commonMain. This errors are fixed with an JVM-specific implementation in src/jvmMain/kotlin.

In the last step in this PR, prefixed with "MP step 3", there was kotlin-native support added. everything already implemented for jvm is reimplemented for native. also the test framework was changed from junit to kotlin.test to be able to execute the tests for jvm and native.

There are some native multi threading issues not covered in this PR, I would suggest that we talk about this issues after this PR is merged :)

[erickok]

I think @drieks did a great job, especially with the step by step moving around of code to keep it undserstandable. Certainly the PR is already big enough to warrant merging before going into the multi-threading issues. The kotlin-native version might not be usable as-is then in a multithreaded app, but all works, including tests, as Koin works on JVM right now. This also satisfies @kpgalligan 's wish for a swift development towards some intermediate (alpha?) release.

As for the native multithreading part, it seems important and @kpgalligan clearly has a wealth of knowledge on it. There is still no multi-threaded coroutines on native, but there will be off course, and people will jump on it.

On JVM Koin makes no threading safety claims at all. I am not sure it should but it is not unreasonable of a request.

[kpgalligan]

Some thoughts

"This also satisfies @kpgalligan 's wish for a swift development towards some intermediate (alpha?) release."

No rush on my account. I've made my changes based off #519 and have published a version we'll be using. It's thread safe for native. We'll just track updates and occasionally republish if threading issues aren't a priority for the main repo.

I think merging 519 is fine, but I wouldn't release a version until threading issues are sorted out. The version we're using has 2 broad additions. kpg/multiplatform adds most other native targets as well as JS, and multithreaded-capable object graphs (Windows pending). kpg/mp_test makes the test module multiplatform, and splits mockito to a separate module (jvm-only).

I had a much longer explanation of the threading considerations, but I pulled it out. It's really difficult to discuss if you don't understand how native threading works. It's very easy to say "just access data from one thread", but it's also really easy to mess that up in practice, so I'd do something like the changes I've made. This will also be easy to modify if/when the native platform has "relaxed mode".

Some thoughts, in no order:

"On JVM Koin makes no threading safety claims at all"

Seems like is should be clear about threading, if making no promises. Mutable internal state should not be thread-questionable. Either internal state is immutable after config, mutation is guarded, or you can only access from the main thread.

That is the crux of why the Kotlin native code has restrictions. The JVM lets you do bad things. It's somewhat reasonable for individual developers to do bad things, but I think frameworks, especially of Koin's type, should probably have a higher bar. Just my 2 cents. You can keep global objects in Koin that have questionable internal state, but Koin itself should be very clear (IMHO).

"There is still no multi-threaded coroutines on native"

Some people are waiting for that. The rest of us ship apps without it. MT coroutines won't solve the underlying issues (except that they're driving "relaxed mode"). The threading safety of Koin, or lack thereof, should be clear regardless.

Thoughts?

[arnaudgiuliani]

On JVM Koin makes no threading safety claims at all. I am not sure it should but it is not unreasonable of a request.

the Koin get() function is held by synchronized Kotlin keyword. It help a bit about multithread access.

[kpgalligan]

For JVM, things like the following may be problematic

private val definitions: HashSet<BeanDefinition<*>> = hashSetOf()

//...

private fun removeDefinitions(module: Module) {
        module.definitions.forEach { definition ->
            removeDefinition(definition)
        }
    }

    private fun saveDefinitions(module: Module) {
        module.definitions.forEach { definition ->
            saveDefinition(definition)
        }
    }

It's not super likely, but if somebody was adding/removing definitions from different threads, that could be an issue and extremely difficult to diagnose. I would assume that's not super likely to happen either, but as stated, I'm not really a Koin expert.

That's a broader discussion for the JVM, however. For native, I should be clear. We simply don't have the option of ignoring thread issues. There's a basic question:

Can code access anything related to Koin from multiple threads?

If yes, then the graph needs to be frozen, and you need concurrent structures. The changes in my branch are technically required, and releasing a version without that will be a problem. It's possible to see state from different threads without freezing, although the API makes it somewhat difficult. If you can access non-frozen state from multiple threads, you run the risk of leaks or crashes, because reference counting is not atomic. Your state may be dealloced early, and you'll get a seg fault.

So, for native, one of two options:

1. Koin is accessible from multiple threads, which means frozen

2. It's not, which means adding some protections to make sure it isn't frozen (because your state will become immutable and stop functioning as expected).

I personally feel like the "one thread" option, option 2, should also force the JVM to one thread, presumably the main thread on Android. That's probably not going to happen, but having different models is problematic to what I'd want from "multiplatform".

Relaxed Mode

This situation may change in the future. Native has a strict threading model, which has had some resistance in the community. To facilitate coroutines (presumably) they're experimenting with "relaxed mode", which would essentially work like the JVM and Objc/Swift with regards to concurrent state.

That's all a "maybe", and has an uncertain date. However, option 3 is to do nothing and see how that plays out, but we're shipping code now and need some usable tools, so we'll be trying out our forked version.

[arnaudgiuliani]

We have several things that are "dynamics" concerning the dependency graph:

• adding/removing definitions
• creating object instances (from definitions function)
• scope instances & scoped definitions instances

Just to better understand what you mean by freezing the graph: keep collections "as is", kindof read-only?

Adding/Removing definitions or scopes modify the underlying collections of Definitions & Scopes. Creating instances and resolve needed dependencies do not impact the collections them self, but more the instances values behind definitions.

Tricky case about scoped instances, that is a definition that can have multiple instances.

Forcing to use Koin from one single thread is not a good case. All collections behind are mainly ConcurrentHashMap, that helps synced operations on the JVM. (Don't remember if spring Framework has, for example, a special multithread mode or something like that...)

What do you call frozen Koin in that way?

[arnaudgiuliani]

or perhaps more a kindof pessimistic lock you mean?

[drieks]

Hi,

frozen is a memory mode of kotlin native. when an object is frozen, a mutable operation to this object will throw an exception. A object is actually mutable -> can only be accessed by one thread or immutable (frozen) -> can be accessed by multiple threads at the same time.

• adding/removing definitions can also be done using immutable references (using copy)
• creating object instances is a problem because this is a state transforming operation
  ** but this can be workarounded in kotlin native using AtomicReference

One Example: Two Threads, both calling a koin getter for the same lazy dependency at almost the same time.

• you can use synchronized (on JVM) to prevent a second dependency creation call
• maybe the dependency has other lazy stuff that needs to be injected
• because this other lazy stuff can also be accessed (and therefore created) by other threads, you also have to synchronize this
• to prevent such cases, the best way is to synchronize all read and write operations to koin
  ** read operations to lazy values can be write operations
• when the factory method call will block (maybe because it is invoking a blocking network call), all read and write access to koin is also blocked
• another options is, of course, using a ConcurrentHashMap
  ** there is no ConcurrentHashMap for kotlin native
  ** there is no synchronized operation in kotlin native

There are some solutions:

• we can assume that koin is single threaded, delegating the synchronize-problem to the koin user
  ** most koin users won't care about threading
  ** most koin users will not see any problems
  ** multi threaded access can lead in hard to reproduce bugs
• every thread can hold a thread-local copy of koin
• we can globally synchronize every access to koin
• we can use atomic reference
• jvm only: ConcurrentHashMap

A unit test can be:

• create a module with maybe 1000 named lazy beans
• every bean will be created by the same factory: the first invocation will return 1, the second 2... the last one 1000
• Create 100 threads
• after all threads are started, they should start to access all 1000 named beans in a random order
• every thread should sum up all beans
• will all 100 threads return 500500? [n*(n+1)/2 -> 1000*1001/2 -> 1001000/2]
• will the factory function be exactly 1000 times invoked?

[arnaudgiuliani]

Thanks @drieks it's pretty clear.

I have to check your PR to see completely your proposal, and then what do you use instead of the ConcurrentHashMap.

I see several orientations then:

• rework read/write access to make it synchronous (but, have to take care about performances and all the kind of deadlocks problem we can have with a dependency graph to resolve)
• open the Koin internal design, to allow a "simple JVM use" and provide implementation with controlled access, more kotlin native friendly

The 2nd way would relax the pressure on the global internals (as we would keep the JVM version kindof like this), and let find the perfect fit for Kotlin native needs.

I definitely have to check your PRs 👍

[kpgalligan]

Mixing replies.

"Just to better understand what you mean by freezing the graph: keep collections "as is", kindof read-only?"

Stately has an implementation of a concurrent hash map and list(s) specifically for use in Native and a frozen object graph. That means it's a frozen mutable collection. It will not win any performance races, but enables solutions to the issues we are discussing here.

Freezing is a runtime designation of Native that basically says everything here is really immutable. The "workaround" with AtomicReference allows you to change state in a frozen object. I don't think we should categorize it as a "workaround". You will have a short, painful career as a Kotlin Native developer if you aren't familiar with Atomics*.

(* the native team may introduce "relaxed mode", as previously discussed)

"Forcing to use Koin from one single thread is not a good case" Agree. Just saying if you do, it should be that way for JVM and Native, not different on one or the other.

"adding/removing definitions can also be done using immutable references (using copy)" How, without AtomicReference? The global Koin instance would be frozen. You are saying copy, with atomic reference (again, coded in my branches with FrozenDelegate).

"creating object instances is a problem because this is a state transforming operation" I would imagine this is the same issue as above (adding/removind definitions). Also, this only applies to longer lived objects, not factory

"another options is, of course, using a ConcurrentHashMap
** there is no ConcurrentHashMap for kotlin native
** there is no synchronized operation in kotlin native"

There are with libraries (Stately, as mentioned and coded)

"we can assume that koin is single threaded, delegating the synchronize-problem to the koin user
** most koin users won't care about threading
** most koin users will not see any problems
** multi threaded access can lead in hard to reproduce bugs"

This is not an option for native. Again, it's not just "simple" race conditions. It's crashes. JVM race conditions will produce some hard to repro issues, sure, but native is more dire.

"every thread can hold a thread-local copy of koin"

Yeah, but then "single" isn't. Right? I think this would be bad, and again, should be the same for JVM and Native.

"we can globally synchronize every access to koin"

In you example above about how this locks on a network call, I think there are answers between "globally synchronize" and "do nothing". Also, this won't help with Native without alternate structures (ie Stately-like things). You still need a way to mutate frozen state. You cannot have normal mutable state and simply synchronize it. I mean, technically you can, but you will have problems.

"we can use atomic reference
jvm only: ConcurrentHashMap"

Again, not JVM only.

Sorting out concurrency issues with the JVM is a related topic, but I would say step one for Native is deciding if you're going to "see" koin from multiple threads or not, and if so, you don't have a ton of options other than make frozen mutable structures.

I don't know if what I'm saying is really landing, though. I'd do a call at this point if this was a client project. I feel like we're going to go back and forth repeating a lot of things.

[kpgalligan]

You replied while I was replying. With regards to the JVM, I think "The 2nd way would relax the pressure on the global internals (as we would keep the JVM version kindof like this), and let find the perfect fit for Kotlin native needs." This is kind of what I'm saying. The JVM should get a rethink in case of concurrency issues, but trying to solve that and native at the same time will add complexity.

If native is multithreaded, then you need some concurrent frozen structures. If not, then you don't. That's kind of the bottom line.

[kpgalligan]

I guess to also clarify, there's the 519 PR to start multiplatform, then I have some branches built on that which would be a rebase and PR. I just want to clarify where I think the native direction should go. Not saying the 519 PR should change. Just that you should not create a release from it. Does that make sense?

[drieks]

Hi,

@arnaudgiuliani: this PR will still use ConcurrentHashMap for JVM, native has only single thread support. But native multi thread support and JS support is already added by @kpgalligan.

@kpgalligan: yes, my last reply was mainly targeted for the JVM.

Sure, I also think that single thread or thread local global state is not an option. When using copy, there is still a place required to store the "current" state of the immutable structure. I just wanted to list the options I see.

Given the problem "where to store the global source of truth", a full immutable koin implementation does not solve many problems. Read access to immutable data always works, but because the data may be mutated (lazy initialized) while reading, such an immutable structure may be already outdated because another thread has already created a copy - a new "source of truth".

Maybe ConcurrentHashMap for JVM and Stately for native is the best option. (JavaScript is not affected by this because it is already single threaded)

The JVM should get a rethink in case of concurrency issues, but trying to solve that and native at the same time will add complexity.

If native is multithreaded, then you need some concurrent frozen structures. If not, then you don't. That's kind of the bottom line.

I agree. As @kpgalligan said, PR 519 is just a first step to MP support. We should keep ConcurrentHashMap for JVM and add Stately for native, @kpgalligan has already added this.

[erickok]

Is the KoinMPLock simply an abstraction over the differnet locking (synchronization) mechanisms in Kotlin/JVM versus /Native, or is there more to it?

In all fairness, after reading the code here and @kpgalligan 's fork (with multithreaded branch) the functional changes are much less and simpler than I expected at first.

[kpgalligan]

KoinMPLock is just an abstraction over the different locking mechanisms. The "Kotliny" addition is using the invoke operator, which was a little confusing to me at first tbh. Functionally, very little has changes. For multiple threads and native, in general, it's not so much that everything is frozen/immutable, but minimizing mutable state, and being forced to be precise about it. In the actual code, not much has changed. Where there are var properties, they're backed by frozen delegates. Mutable collections have their thread-safe form.

Performance is a different discussion. It may make sense to abstract a bit more to get JVM back where it was. I think we're synchronizing more aggressively now on JVM, and that may be something to roll back.

There are functional changes that might make sense, but I think at this point, any significant thinking around concurrency and native should wait and see what happens with 1.3.50.

https://github.com/JetBrains/kotlin-native/blob/master/CHANGELOG.md#v1350-aug-2019

"Major memory manager refactoring (GH-3129)"

JetBrains/kotlin-native#3129

That's the stuff I was referring to earlier. It was less clear when that was landing, but now it appears at least to be available in 1.3.50.

I wouldn't necessarily hold off on releasing native while waiting for 1.3.50, as we don't know how "relaxed mode" will ultimately look, and changing the existing code to support relaxed mode is not complex.

[drieks]

@erickok:

Is the KoinMPLock simply an abstraction over the differnet locking (synchronization) mechanisms in Kotlin/JVM versus /Native, or is there more to it?

Yes, it is mainly a wrapper around the java synchronized keyword. In Koin Version < 3.0.0, all synchronize invocations are on this. But because synchronizing on a public lock is an anti-pattern (someone may call synchronize using the koin instance in another thread without leaving the lock), KoinMPLock for JVM has a private val lock = Object() that is used for synchronize calls.
The native Version of KoinMPLock is a no-op implementation.

In all fairness, after reading the code here and @kpgalligan 's fork (with multithreaded branch) the functional changes are much less and simpler than I expected at first.

I tried to change just the parts that are required to compile and run the tests on native and only small other changes.

@kpgalligan: I think you refer to this commit?
ed9dee8

I think we're synchronizing more aggressively now on JVM, and that may be something to roll back.

Synchronizing only on write access is also an anti pattern. Other threads can call into app while close is still running by some thread.

[kpgalligan]

@drieks "Synchronizing only on write access is also an anti pattern"

Not that commit. The changes in general. There's more concurrency checking in the JVM than there was previously, which could impact performance. If we're talking about just making this multiplatform, there's an argument to revert anything impacting the JVM. If the goal is to refactor how Koin works internally that's OK, but that's a bigger discussion, and probably would include other refactoring.

I've seen that there's a performance test out in the wild, but I'm guessing it's not super formal. If performance is a concern, I think there should also be some kind of benchmark, so it'll be easier to understand what impact changes will have.

So, anyway, the point of the comment was the JVM now synchronizes more, and uses concurrent collections in places it previously was not, which is better if using from multiple threads, but we should understand what impact that will have, and there may be a desire to not introduce that impact now.