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

Service exit from all scopes callback? #151

Closed
jamolkhon opened this Issue Mar 5, 2019 · 8 comments

Comments

Projects
None yet
2 participants
@jamolkhon
Copy link

jamolkhon commented Mar 5, 2019

Is it possible to be notified when a service is destroyed completely, that is when it exist all scopes? ScopedServices.Scoped only notifies when services from any single scope. I need it to clear all disposables. Currently disposing inside onExitScope is dangerous because a service could be part of multiple scopes.

@Zhuinden

This comment has been minimized.

Copy link
Owner

Zhuinden commented Mar 5, 2019

I didn't expect the same service to be registered in every scope manually, as the idea was that if a service is shared then it'd be in a shared scope; technically from my side I expected GlobalServices to be used for such a thing (although of course that is the last thing you check during a lookup, as immediate parents are preferred).

Technically it's possible to track this (it'd be odd if not) but it'd be a new type of callback.


However, currently an amazingly tricky thing that you can do is that you actually get what scope tags you were registered with onEnterScope(String scopeTag), so if you collect those into a set, then in onExitScope(String scopeTag) you remove them from the set, and if the set is empty then you're not registered in any scopes.

@jamolkhon

This comment has been minimized.

Copy link
Author

jamolkhon commented Mar 5, 2019

Do you consider disposing in onExitScope ignoring the scope parameter to be safe? I'm only using implicit scopes.

@Zhuinden

This comment has been minimized.

Copy link
Owner

Zhuinden commented Mar 5, 2019

Do you consider disposing in onExitScope ignoring the scope parameter to be safe?

I did, until I got this question 😄

This actually never came up and we've been using implicit scopes for quite a while now.

Technically there is nothing in ScopeManager that would stop it from being able to build/manage a Map<Object, Set<String>> where String is scope tags it was registered with as it is binding services to scope or destroying the scope.

I can envision a ScopedServices.Registered callback similarly to Scoped and Activated.

@Zhuinden Zhuinden added the enhancement label Mar 5, 2019

@Zhuinden

This comment has been minimized.

Copy link
Owner

Zhuinden commented Mar 10, 2019

This is gonna take me more time than I originally thought, but in the meantime it's actually fairly easy on user side to create these callbacks.

abstract class RegisteredService: ScopedServices.Scoped {
    private val scopeSet = mutableSetOf<String>()

    @CallSuper
    override fun onEnterScope(scopeTag: String) {
        if(scopeSet.isEmpty()) {
            onServiceRegistered()
        }
        scopeSet.add(scopeTag)
    }

    @CallSuper
    override fun onExitScope(scopeTag: String) {
        scopeSet.remove(scopeTag)
        if(scopeSet.isEmpty()) {
            onServiceUnregistered()
        }
    }

    abstract fun onServiceRegistered()
    abstract fun onServiceUnregistered()
}

Which is much easier than what I have to do 😄


The trickery with multi-registration is that I call Bundleable callback before calling onEnterScope callback.

I wonder if that should be moved to "before onServiceRegistered when I add it.

@Zhuinden Zhuinden added the CRITICAL label Mar 13, 2019

@Zhuinden

This comment has been minimized.

Copy link
Owner

Zhuinden commented Mar 13, 2019

On second thought, I think we actually currently have a subtle bug in our app directly caused by relying on onEnterScope in a similar manner.

I should definitely add onServiceRegistered, and call Bundleable before that.

Which ends up with the question "what is the use of ScopedServices.Scoped", with that, I'm not sure.


In the meantime, I've just now migrated over to the onServiceRegistered()/onServiceUnregistered() by the exact same schematics as I said above.

    private val registeredScopes = linkedSetOf<String>()

    @CallSuper
    open fun onServiceRegistered() {
        isServiceRegisteredRelay.accept(true)
        lifecycleSubject.onNext(ServiceLifecycleEvents.REGISTERED)
    }

    @CallSuper
    open fun onServiceUnregistered() {
        isServiceRegisteredRelay.accept(false)
        lifecycleSubject.onNext(ServiceLifecycleEvents.UNREGISTERED)
    }

    @CallSuper
    @Deprecated(message = "This method is not safe, use onServiceRegistered() instead", level = DeprecationLevel.ERROR)
    override fun onEnterScope(scope: String) {
        if (registeredScopes.isEmpty()) {
            onServiceRegistered()
        }
        registeredScopes.add(scope)
    }

    @CallSuper
    override fun onScopeActive(scope: String) {
        lifecycleSubject.onNext(ServiceLifecycleEvents.SCOPE_ACTIVE)
        isScopeActiveRelay.accept(true)
    }

    @CallSuper
    override fun onScopeInactive(scope: String) {
        lifecycleSubject.onNext(ServiceLifecycleEvents.SCOPE_INACTIVE)
        isScopeActiveRelay.accept(false)
    }

    @CallSuper
    @Deprecated(message = "This method is not safe, use onServiceUnregistered() instead", level = DeprecationLevel.ERROR)
    override fun onExitScope(scope: String) {
        registeredScopes.remove(scope)
        if (registeredScopes.isEmpty()) {
            onServiceUnregistered()
        }
    }

I guess I can't thank you enough for bringing this mistake to my attention 😞

@jamolkhon

This comment has been minimized.

Copy link
Author

jamolkhon commented Mar 14, 2019

Thank you for actively responding and dealing with issues.

I guess I can't thank you enough for bringing this mistake to my attention

I don't deserve this much appreciation.

Thank you for this awesome lib!

@Zhuinden

This comment has been minimized.

Copy link
Owner

Zhuinden commented Mar 16, 2019

Thank you for this awesome lib!

I'm glad you like it, and that it also helps you simplify navigation and service lifecycle management 🙂

I also have some good news, ScopedServices.Registered is now available in 1.14.0 (see #156 and 86297b8 )- "migration path" is to just ditch .Scoped and use .Registered instead.

@Zhuinden Zhuinden closed this Mar 16, 2019

@jamolkhon

This comment has been minimized.

Copy link
Author

jamolkhon commented Mar 16, 2019

That was quick, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.