google / dagger

A fast dependency injector for Android and Java.
https://dagger.dev
Apache License 2.0
17.41k stars 2.01k forks source link

Feature request: Scope-private bindings #1438

Open cesar1000 opened 5 years ago

cesar1000 commented 5 years ago

Some bindings are meant to be injected only in the scope in which they are defined, such that injecting them in a subscope is a logical bug. For example, let's say that I add a Closer binding to my application scope to allow objects to register for cleanup. If an object belonging to a strict subscope of the application scope were to use this Closer, it would get leaked until application cleanup. In the Android world, a Lifecycle object in an activity scope would cause objects in strict subscopes to leak until activity destruction if they were to add themselves as observers.

I've seen this issue numerous times with objects that are essentially event dispatchers: release notifiers, lifecycle event dispatchers, save state handlers... We typically deal with them by qualifying them with a @WithMatchingScope(SCOPE_NAME) annotation, and using static analysis to detect injection in the wrong scopes. However, this is just adding extra boilerplate to work around the core problem: the binding should just not be visible at all outside of the scope for which it's intended. It would be ideal if Dagger allowed annotating such bindings with a marker such as @Private and enforced the contract.

One nice side effect of using scope-private bindings is that nested subcomponents would be able to declare their own instances of each binding without the extra qualification. For example, I'd like to be able to add Closers to all my scopes without concern for clashes, since each Closer is meant to be used only within its own scope. And, more importantly, I'd be able to use Closer without the extra qualification in the injected constructor of my unscoped objects, and get the right instance of the Closer based on the instantiation scope. This would be a critical piece of an implementation of a generic component cleanup mechanism in Dagger.

Ethan1983 commented 5 years ago

Isn't this already supported by Inherited subcomponent multibindings (https://google.github.io/dagger/multibindings.html)?

netdpb commented 5 years ago

A Closer object like you describe would maintain a collection of objects to be closed, right? If you intend for them to be closed when they're GCed, could you use a collection of WeakReferences in order to avoid leaking them?

In general, I'm wary of using Dagger scopes to implement arbitrarily sophisticated persistence or memory management schemes.

cesar1000 commented 5 years ago

Using a WeakReference in this case would prevent the leak, but it would cause the cleanup to be unpredictable: the object in the nested scope may or may not get closed, depending on when it's GC'ed. What's worse, that would introduce a new bug: non-scoped objects using the correct Closer may get GC'd before they can be closed.

I used Closer as a simplistic example, but this request is not fundamentally about memory management. You'd see a similar (and arguably worse) issue with the Lifecycle object in Android (or, in general, any event dispatcher). Let's say that my fragment uses a subcomponent of the activity's component, and I create it and destroy it as I create and destroy the fragment's view (like in a ViewPager). An object in my fragment's scope registering with the activity's lifecycle would continue to get activity events for a while after the fragment's view has been destroyed.

In my view, this is a type of binding that's meant for internal bookkeeping and lifecycle management of a scope, and it comes up relatively frequently when using Dagger components to represent scopes that have a lifecycle associated with them (like is frequently the case on Android applications). I feel that the use case is relatively commonplace, but the current need to apply scope-specific qualifiers to mimic the visibility constraint makes it clunky to implement.

netdpb commented 5 years ago

OK. We've had various discussions about features like this for a while. I'm trying to understand in which ways this concept is generally useful for Dagger and in which ways it's really more Android-focused.

An Android-focused way of thinking about this is to have a bound object that indicates the "current" object in Android's Context and view hierarchy for any given binding. Then you could use that to pass to a singleton Closer's registry method, for instance.

A Dagger-general way of thinking about it is to expose the "current" scope (or, equivalently, the "current" component), meaning the component instance that owns the binding. Then you could use that either as a token to pass to some persistence mechanism at the root component (e.g., singletonCloser.register(this, currentComponent)), or maybe to automatically qualify a binding, so that you can inject something like @CurrentComponent Closer.

In both cases, it's ambiguous which component or Android object we want to consider the right one for unscoped bindings. I suppose we could limit the users of this feature to scoped bindings, but that might not be great; what if we want to register an object from an unscoped binding as a listener?

cesar1000 commented 5 years ago

I think that this kind of feature would be particularly useful on Android, since it uses very strict lifecycles in multiple nested scopes, but has general value in any applications that implement a concept of lifecycle or component-scoped event dispatcher.

The alternatives that you suggest are workable, but they require central coordination. Also, they don't prevent registration of an object in a Closer (or other type of event dispatcher) in the wrong scope. Also, it couples component definitions to specific scopes. For example let's say that I define this:

class MyViewController {
  @Inject
  MyViewController(Lifecycle lifecycle) {
    lifecycle.registerObserver(event -> { ... })
  }
}

Ideally, I'd like to be able to inject MyViewController in any scope, and let dagger figure out with Lifecycle is available within that particular scope. Using a qualifier like, say, @FeatureXScope would tie this component to that scope and hinder reuse.

In the case of unscoped bindings, I would think that every unscoped injection can ultimately be traced back to a particular component. You're likely already doing that to determine which scoped bindings are available for injection into an unscoped binding, right?

trevjonez commented 4 years ago

What about an @BindingOverride? Combine that with @FromSuper which would act as a qualifier for a super component and you can build what I would expect to work for most resource clean up needs.

@BindingOverride would only be applicable to an @Provides or @Binds functions? if the qualifier/type combo matches within the same component still throw a duplicate binding error.

@FromSuper would only be applicable within a subcomponent graph and act as a constraint on the dependency resolution source to prevent circular bindings. Maybe only applicable to @Provides params to start with to keep things simple?

@Module
class ConceptModule {
  @Provides
  @SomeCustomScope
  @BindingOverride
  fun scopeDisposable(@FromSuper superScopeDisposable: CompositeDisposable): CompositeDisposable {
    return CompositeDisposable().addTo(superScopeDisposable)
  }
}

given that subcomponents are implemented as inner classes of their super components could an override be possible and have requests resolve to the narrowest scope binding that matches a request?

trevjonez commented 2 years ago

It would seem there is other "high impact" projects leaning heavily on the concept of a scope local or override mechanism.

https://developer.android.com/jetpack/compose/compositionlocal