kohesive / injekt

Dependency Injection for Kotlin
MIT License
235 stars 19 forks source link

Problems w/ Android 4.x #33

Open brianegan opened 8 years ago

brianegan commented 8 years ago

Hello! Trying to get Injekt going with a test project of mine, and running into trouble. It works great on Lollipop+, but on Android 4.x I'm running into trouble.

The error I'm seeing:

java.lang.RuntimeException: Unable to instantiate application com.brianegan.bansa.counter.Application: uy.kohesive.injekt.api.InjektionException: No registered instance or factory for type com.brianegan.bansa.Store<com.brianegan.bansa.counter.ApplicationState, com.brianegan.bansa.Action>
    at android.app.LoadedApk.makeApplication(LoadedApk.java:526)
    at android.app.ActivityThread.handleBindApplication(ActivityThread.java:4390)
    at android.app.ActivityThread.access$1500(ActivityThread.java:139)
    at android.app.ActivityThread$H.handleMessage(ActivityThread.java:1260)
    at android.os.Handler.dispatchMessage(Handler.java:102)
    at android.os.Looper.loop(Looper.java:136)
    at android.app.ActivityThread.main(ActivityThread.java:5105)
    at java.lang.reflect.Method.invokeNative(Native Method)
    at java.lang.reflect.Method.invoke(Method.java:515)
    at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:792)
    at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:608)
    at dalvik.system.NativeStart.main(Native Method)
Caused by: uy.kohesive.injekt.api.InjektionException: No registered instance or factory for type com.brianegan.bansa.Store<com.brianegan.bansa.counter.ApplicationState, com.brianegan.bansa.Action>
    at uy.kohesive.injekt.registry.default.DefaultRegistrar.getInstance(DefaultRegistrar.kt:99)
    at uy.kohesive.injekt.api.InjektScope.getInstance(Scope.kt)
    at com.brianegan.bansa.counter.Application.<init>(Application.kt:30)
    at java.lang.Class.newInstanceImpl(Native Method)
    at java.lang.Class.newInstance(Class.java:1208)
    at android.app.Instrumentation.newApplication(Instrumentation.java:990)
    at android.app.Instrumentation.newApplication(Instrumentation.java:975)
    at android.app.LoadedApk.makeApplication(LoadedApk.java:521)
    ... 11 more

My code can be found here: https://github.com/brianegan/bansa/blob/master/examples/counter/src/main/kotlin/com/brianegan/bansa/counter/Application.kt

I also tried Injekting a module in onCreate instead of using a companion object, with no luck. Feeling like a bit of a n00b here though, trying to figure out what I'm doing wrong! Any advice would be much appreciated :)

apatrida commented 8 years ago

Hi, first a note on your code:

 val store = Injekt.get(fullType<Store<ApplicationState, Any>>())

should simply be a delegate when used as a property:

 val store: Store<ApplicationState, Any> by injectLazy() // or injectValue()

Or can be assigned:

 val store: Store<ApplicationState, Any> = Injekt.get()

and in the injekt registration:

 addSingleton(
                    fullType<Store<ApplicationState, Any>>(),
                    createStore(ApplicationState(), counterReducer));

can be:

  addSingleton<Store<ApplicationState, Any>>(createStore(ApplicationState(), counterReducer))

Or if the return type of createStore is already Store<ApplicationState, Any> then just:

 addSingleton(createStore(ApplicationState(), counterReducer))

No need to use fullType anywhere. Reified generics will take care of this. If you cannot get the IDE to show the correct extension function, then import:

import uy.kohesive.injekt.api.*
import uy.kohesive.injekt.*

temporarily until you have everything in use and happy.

This shouldn't be your problem though unless the newer version of Android is causing the instance to be created without loading the reference to the companion object. With some magic in their VM they could do this, although evil it is possible.

If you only have one application instance anyway, just change it to a member as well:

val injektMain = object : InjektMain() {
        override fun InjektRegistrar.registerInjectables() {
            // ...
        }
    }

It is a small change and will make sure all registrations are done since members are instantiated and initialized in the order they appear top to bottom. Let me know if this helps.

brianegan commented 8 years ago

Great, thanks for the tips! I tried a couple of similar patterns, but not this exact one. I'll give it a shot tonight :)

apatrida commented 8 years ago

How did this work out?

brianegan commented 8 years ago

Hey there! Sorry about the delay -- busy times from my side. Thanks so much again for the help. I made some time tonight to play around with it. Here my findings:

Good call on the code cleanup -- addSingleton and by injectLazy() are really cool improvements.

With regards to Android 4.x, I tried a few things.

Attempt 1 - InjektMain() becomes a field

I tried your suggestion of assigning the InjektMain object to a field in the Application class. Unfortunately, I ran into the same error as described in my original post. You can see this code here:

https://github.com/brianegan/bansa/blob/injekt-android-4.x-fix-1/examples/counter/src/main/kotlin/com/brianegan/bansa/counter/Application.kt

Attempt 2 - Move val store to onCreate

A slight variation on Attempt #1 -- I've simply moved the val store from a class field into the onCreate function to delay the access even further. This also didn't work. Same error.

https://github.com/brianegan/bansa/blob/injekt-android-4.x-fix-2/examples/counter/src/main/kotlin/com/brianegan/bansa/counter/Application.kt

Attempt 3 - importModule

A combination of #1, #2 and now I've introduced a StoreModule : InjektModule class and tried injecting that. This didn't work either :(

https://github.com/brianegan/bansa/blob/injekt-android-4.x-fix-3/examples/counter/src/main/kotlin/com/brianegan/bansa/counter/Application.kt

Attempt 4 - Don't inject in the application class

This isn't my favorite solution as someone might need to both register and then use a module inside the application init phase, but I wanted to give it a shot. I simply move the injection out of the Application class and into the RootActivity. Unfortunately, this also didn't work :(

https://github.com/brianegan/bansa/blob/injekt-android-4.x-fix-4/examples/counter/src/main/kotlin/com/brianegan/bansa/counter/Application.kt

https://github.com/brianegan/bansa/blob/injekt-android-4.x-fix-4/examples/counter/src/main/kotlin/com/brianegan/bansa/counter/RootActivity.kt

Attempt 5 - No Injekt (Control)

As a sanity check, I removed Injekt and the app worked on Android 4.x.

https://github.com/brianegan/bansa/blob/injekt-android-4.x-fix-5/examples/counter/src/main/kotlin/com/brianegan/bansa/counter/RootActivity.kt

Thanks again for helping me track this down! This is such a cool project, I hope we can get it working on Android 4.x.

Hopefully these branches will provide some helpful code samples. If I get a bit more time, I'll try to dig into the root of the problem.

brianegan commented 8 years ago

Another attempt: Tried injecting a basic Kotlin type String into the root activity and that worked... Not sure what it is about my types that work on Android 5+ vs 4.x :(

apatrida commented 8 years ago

can you add logging to see the order of operations, log inside the injekt registration, log inside your onCreate method. Is OnCreate called from the ancestor constructor? or is it called later?

brianegan commented 8 years ago

Sure, I also stepped through the app, and it runs through in this order (which seemed correct to me):

  1. injektMain field is created, runs registerInjectables
  2. registerInjectables runs importModule(StoreModule())
  3. StoreModule#registerInjectables runs, executing the addSingleton<Store<ApplicationState, Any>>
  4. Application#onCreate is called
  5. RootActivity#onCreate is called, runs Injekt.get<Store<ApplicationState, Any>>()
  6. App Crash with error message above
apatrida commented 8 years ago
  1. the error must be different because you are asking for Any instead of Action as the second generic parameter. Can I see the current error with latest attempt?
apatrida commented 8 years ago

also can you try it as a addSingletonFactory and pass a lambda where you cast it to Any if you are going to do that cast.

 override fun InjektRegistrar.registerInjectables() {
            addSingletonFactory { 
                  createStore(ApplicationState(), counterReducer) as Store<ApplicationState, Any>
            }
        }

The compiler should give you warnings about this cast, or say it isn't needed, or something, I'd like to see what it tells you.

Also if you debug, you can look at the injekt registry and see what type is stored in the lookup hashmap which will also give a clue.

apatrida commented 8 years ago

I can mock these out and see if there is a problem with generics somewhere, but I've never seen one.

brianegan commented 8 years ago

Ah, sorry -- that error message above is slightly out of date as this library I'm working on has moved from Action to Any. The error is essentially the same (this is after moving all injection out of the app and into the activity):

java.lang.RuntimeException: Unable to start activity ComponentInfo{com.brianegan.bansa.counter/com.brianegan.bansa.counter.RootActivity}: uy.kohesive.injekt.api.InjektionException: No registered instance or factory for type com.brianegan.bansa.Store<com.brianegan.bansa.counter.ApplicationState, java.lang.Object>

I'll give it a quick shot as a singletonFactory instead. Thanks again for your help debugging this!

I also think the types are correct overall, as it is working in Android 5+ -- I'm not sure if there's something unique to the 4.x environment?

brianegan commented 8 years ago

"Also if you debug, you can look at the injekt registry and see what type is stored in the lookup hashmap which will also give a clue."

Ah, cool -- I'll try this as well to see if it's a weird type mismatch! Thanks again.

brianegan commented 8 years ago

"The compiler should give you warnings about this cast, or say it isn't needed, or something, I'd like to see what it tells you."

Compiler: "No cast needed"

apatrida commented 8 years ago

I'm writing a test case now. Back in a bit with results, maybe something happens when two generics are present.

brianegan commented 8 years ago

Cool, Thanks!

apatrida commented 8 years ago

This test passes:

package uy.kohesive.injekt

import org.junit.Test
import uy.kohesive.injekt.Injekt
import uy.kohesive.injekt.InjektMain
import uy.kohesive.injekt.api.*
import java.math.BigDecimal
import java.util.concurrent.atomic.AtomicInteger
import kotlin.test.assertEquals
import kotlin.test.fail

class TestGithub33 {
    data class Store<T1, T2>(val state: T1, val obj: T2)
    data class ApplicationState(val id: String)
    data class ObjType1(val id: String)
    data class ObjType2(val id: String)
    data class OtherState(val id: String)

    companion object : InjektMain() {
        override fun InjektRegistrar.registerInjectables() {
             addSingleton(Store(ApplicationState("as1"), ObjType1("ot1")))
             addSingleton(Store(ApplicationState("as2"), ObjType2("ot2")))
             addSingleton(Store(OtherState("os1"), ObjType1("ot1")))
             addSingletonFactory<Store<ApplicationState, Any>> { Store(ApplicationState("as3"), "howdy") }
        }
    }

    @Test
    fun testWithGenerics() {
        val check1: Store<ApplicationState, ObjType1> = Injekt.get()
        assertEquals(Store(ApplicationState("as1"), ObjType1("ot1")), check1)

        val check2: Store<ApplicationState, ObjType2> = Injekt.get()
        assertEquals(Store(ApplicationState("as2"), ObjType2("ot2")), check2)

        val check3: Store<OtherState, ObjType1> = Injekt.get()
        assertEquals(Store(OtherState("os1"), ObjType1("ot1")), check3)

        val check4: Store<ApplicationState, Any> = Injekt.get()
        assertEquals(Store<ApplicationState, Any>(ApplicationState("as3"), "howdy"), check4)
    }

    @Test
    fun testDoesNotExistWithGenericType() {
        try {
            val check: Store<OtherState, Any> = Injekt.get()
            fail("Should not exist, this type")
        }
        catch (ex: Throwable) {
            // pass
        }
    }
}

using inject 1.14.x (actually is 1.15.0 with kotlin 1.0.1 but no other changes and reverting Kotlin doesn't change it)

brianegan commented 8 years ago

Ah, interesting. Here's a screenshot of the registrar. Looked correct to me, does this look fishy at all to you?

screenshot 2016-03-24 00 22 18
apatrida commented 8 years ago

yes, this is really odd. can we see the type value passed into the 2nd layer of the Injekt.get() call, after it creates and internal type key.

apatrida commented 8 years ago

if the key looks the same, and it isn't being found in the hashMap, it is something to do with hashcode() and equals() for the Type objects.

apatrida commented 8 years ago

it is a map of Map<Type, lambdaType> that would be failing to compare in the map.

   private val factories = ConcurrentHashMap<Type, () -> Any>()
    private val keyedFactories = ConcurrentHashMap<Type, (Any) -> Any>()
apatrida commented 8 years ago

this method in the DefaultRegistry.kt is being called:

    @Suppress("UNCHECKED_CAST")
    override fun <R: Any> getInstance(forType: Type): R {
        val factory = factories.getByKey(forType) ?: throw InjektionException("No registered instance or factory for type ${forType}")
        return factory.invoke() as R
    }

so if its type matches the type you see in the Map and it is not found, it'll be ugly issue.

brianegan commented 8 years ago
screenshot 2016-03-24 00 36 25
brianegan commented 8 years ago

I may be wrong, but they look like the same type, no? Sorry if that's a tough one to tackle :(

apatrida commented 8 years ago

They do appear the same. But they are no longer comparing as equal or hashcode is no longer the same for the two Type objects.

apatrida commented 8 years ago

maybe a bug in the new Android, I'm asking someone else to look at this from Reflection team of Kotlin itself. He is in Russia so not likely tonight.

brianegan commented 8 years ago

It actually works fine in the newer android (Android Lollipop+, aka 5+). It is broken on the 4.x range though.

apatrida commented 8 years ago

ah, they maybe had a bug they fixed.

brianegan commented 8 years ago

I'm thinking so... Anyhow, thanks so much for the help! Been a fun github-issue debugging session :)

apatrida commented 8 years ago

There could be a workaround of creating a different object type to hold the values, but that means handling 4-5 possible forms of the generics we could have and the whatever stack of values they might contain.

apatrida commented 8 years ago

Ah, one thing that could break equals and hashCode is if these classes came from different classloaders. One step used one class loader, the other step a different. maybe.

apatrida commented 8 years ago

if for the Type, you drill into it and see somewhere in there is a reference to the classloader.

apatrida commented 8 years ago

another work around for you is to put the injekt main code into OnCreate as well and use it within there, any factories it sets up will be available after to anything. That "instance" doesn't do anything since the registry itself is a singleton Kotlin object ... only if you create your own Injekt instances do you have to keep a reference around. You can just Injekt.addSingleton as well within the OnCreate.

you would surely be on the same classloader then.

brianegan commented 8 years ago

Classloader inspection:

screenshot 2016-03-24 00 48 05
brianegan commented 8 years ago

Also, I tried putting it in onCreate with no luck :(

apatrida commented 8 years ago

no clue at this point. We aren't doing anything weird. Unless Type docs say we shouldn't trust its hashcode/equals (maybe it does, I never checked)

brianegan commented 8 years ago

I wonder if something like a TypeToken might be an alternative? http://docs.guava-libraries.googlecode.com/git/javadoc/com/google/common/reflect/TypeToken.html

apatrida commented 8 years ago

The work around would be custom equals/hashcode for types and come up with something. maybe the string version of the Type would be a valid key. But then would not be correct for multiple classloaders. But not sure that is a very useful case.

TypeToken like Guava is what we do to get the Type, just like they do. It isn't any different.

apatrida commented 8 years ago

The problem is using Type as a hash map key. And it not working for older Android

brianegan commented 8 years ago

Ah, ok... for some reason I thought the Guava TypeToken had a slightly different equals and hashcode impl.

brianegan commented 8 years ago

"The problem is using Type as a hash map key. And it not working for older Android"

Yah for sure, was thinking something like TypeToken (which I mistakenly had a slightly different equals and hashcode than Type) would act in place of the Type being used now for the hash map key. Or, as you said, implementing something custom yourself

brianegan commented 8 years ago

Well, interesting getting to the root of it at least! Need to get to sleep from my end, but once again, I really appreciate the help getting to the bottom of it :) Fun journey!

apatrida commented 8 years ago

TypeToken delegates hashcode and equals to Type. one quick change is to use Type.toString and see if it helps. But, there is no docs for toString for Type to say that it should generate anything in particular. so just as it might work, it might not in other environments. The interface Type and ParameterizedType does not say anything about hashcode and equals.

apatrida commented 8 years ago

I could see if I get a Kotlin KType proxy that is reliable or if that needs reflection jar.

apatrida commented 8 years ago

ok, will see what Alex says tomorrow and check in on this again.

apatrida commented 8 years ago

one other approach is to store everything with two maps, one by Type as key and other as Type.toString() as Key and if either hits, it works. Covers at least ONE of them working correctly.

brianegan commented 8 years ago

Hey Hey :) Just a curious ping: Hear anything from your contact Alex? No biggies either way, just curious about the problem!

apatrida commented 8 years ago

He agreed with what I said before,. Equals and hash code broken for Type class or due to classloader differences. It can be fixed by wrapping Type with something reliable for comparison.

apatrida commented 8 years ago

Is this still an issue?