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

Scope is resurrected with its instances #758

Closed
adenisyuk opened this issue Mar 23, 2020 · 6 comments
Closed

Scope is resurrected with its instances #758

adenisyuk opened this issue Mar 23, 2020 · 6 comments
Labels
core important 🔥 status:checking currently in analysis - discussion or need more detailed specs type:issue
Milestone

Comments

@adenisyuk
Copy link

STR:

  1. Create a scope
  2. Declare an instance on that scope
  3. Close the scope
  4. Create a new scope the same way
  5. Declare an instance on that scope (same way as in step 2)

ER: Instance is successfully declared
AR: Created scope already contains the instance

package com.test

import org.junit.Test
import org.koin.core.qualifier.named
import org.koin.dsl.koinApplication
import org.koin.dsl.module

class ScopeTest {

    val baseUrl = "base_url"
    val baseUrlKey = named("BASE_URL_KEY")

    val scopeId = "user_scope"
    val scopeKey = named("KEY")

    val koin = koinApplication {
        modules(
            module {
                scope(scopeKey) {
                }
            }
        )
    }.koin


    @Test
    fun `recreate a scope`() {
        val scope = koin.createScope(scopeId, scopeKey)
        scope.declare(baseUrl, baseUrlKey)
        scope.close()

        val scope2 = koin.createScope(scopeId, scopeKey)
        scope2.declare(baseUrl, baseUrlKey)
    }

}

org.koin.core.error.DefinitionOverrideException: Trying to override existing definition '[Single:'java.lang.String',qualifier:q:'BASE_URL_KEY',scope:q:'KEY']' with new definition typed 'class kotlin.String'

The issue was not observed on v2.0.1 but appeared after upgrading to 2.1.4

@arnaudgiuliani arnaudgiuliani added status:checking currently in analysis - discussion or need more detailed specs type:issue labels Apr 7, 2020
@vitkhudenko
Copy link

Having this issue on v 2.1.5. 😒

@vitkhudenko
Copy link

koin_screenshot

Thanks to @StanislavChumarin for investigation.

@arnaudgiuliani arnaudgiuliani added this to the 2.2.0 milestone Jul 21, 2020
@arnaudgiuliani arnaudgiuliani added core status:accepted accepted to be developed and removed status:checking currently in analysis - discussion or need more detailed specs labels Jul 21, 2020
@arnaudgiuliani
Copy link
Member

Not sure we need to deep copy the ScopeDefinition itself. This ScopeDefinition brings the Scope with all related definitions, no runtime things here.

But need to investigate 👍

@vitkhudenko
Copy link

vitkhudenko commented Jul 21, 2020

Not sure we need to deep copy the ScopeDefinition itself. This ScopeDefinition brings the Scope with all related definitions, no runtime things here.

But need to investigate 👍

Maybe "deep copy" is not the accurate/right term here, but the idea is that a newly created scope should be only seeded with the compile time definitions (versus using the object directly as is). Scopes should not share among themselves the same seeded compile time definitions object, otherwise setting a new definition in runtime on a particular scope effectively sets it to all scopes (even future ones).

@Pwootage
Copy link
Contributor

I'm having this problem as well - downgrading back to 2.0.1 for now fixes it.

This is particularly devastating to my use case, as it leads to leaking information across requests. I create a new Koin scope for each incoming request, and attach the per-request information to that scope, so that it can be dependency injected later on.

 getKoin().createScope(this.uuid, REQUEST_SCOPE).use { scope ->
  // Register external request and session objects
  scope.declare(request)
  scope.declare(session)

  // Get the requested impl, which injects Request and Session
  val impl: TImpl = scope.get(
    clazz = implClass
  )
  // Use the impl
  callback(impl)
} 

@arnaudgiuliani arnaudgiuliani added status:checking currently in analysis - discussion or need more detailed specs and removed status:accepted accepted to be developed labels Oct 28, 2020
@arnaudgiuliani
Copy link
Member

Fixed in last 2.2.0-rc-4. Closing scope was not clearing any new declared extra definition.

@Test
    fun `recreate a scope`() {
        val baseUrl = "base_url"
        val baseUrl2 = "base_url"
        val baseUrlKey = named("BASE_URL_KEY")

        val scopeId = "user_scope"
        val scopeKey = named("KEY")

        val koin = koinApplication {
            modules(
                    module {
                        scope(scopeKey) {
                            scoped { Simple.ComponentA() }
                        }
                    }
            )
        }.koin

        val scope = koin.createScope(scopeId, scopeKey)
        scope.declare(baseUrl, baseUrlKey)
        assertEquals(baseUrl,scope.get<String>(baseUrlKey))

        scope.close()

        val scope2 = koin.createScope(scopeId, scopeKey)
        scope2.declare(baseUrl2, baseUrlKey)

        assertEquals(baseUrl2,scope2.get<String>(baseUrlKey))
    }

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

No branches or pull requests

4 participants