google / guice

Guice (pronounced 'juice') is a lightweight dependency injection framework for Java 11 and above, brought to you by Google.
https://github.com/google/guice
Apache License 2.0
12.51k stars 1.67k forks source link

Singleton scope on .toInstance() binding should be enabled by default #928

Closed powerbroker closed 9 years ago

powerbroker commented 9 years ago

The code this.bind(SomeType.class).toInstance(new SomeType()); in AbstractModule subclass causes default "inject-and-forget-instance" behavior. The singleton behavior should be instead since the instance itself(but not a type/constructor/etc.) is provided and there is no easy way to enforce Singleton behavior over .toInstance() binding.

Guice version(MANIFEST.MF) Bundle-Version: 4.0.0 Export-packages: com.google.inject.name;version="1.4",com.google.inject.binder;version="1.4",com.google.inject.spi;version="1.4",com.google.inject.matcher;version="1.4",com.google.inject.util;version="1.4",com.google.inject;version="1.4"

sameb commented 9 years ago

Do you have a test showing it behaves this way? It shouldn't (and AFAIK doesn't).

powerbroker commented 9 years ago

Here is the simple project demonstrating this. Required libs: guava-osgi-11.0.1.jar guice-4.0-no_aop.jar javax.inject-1.jar

Console output:

Injection of not annotated type: Failure: Separate instances! Injection of annotated with @Singleton type: As expected: Single instance.

// ------------------------------------------------------------------- package bugsample.kernel; import bugsample.kernel.ui.NavBackAction; import bugsample.kernel.ui.StatePanel; import com.google.inject.Guice; import com.google.inject.Injector;

public class Controller {

public static void main(String[] args) {
    Injector objRegistry;

    objRegistry = Guice.createInjector(new KernelModule());

    StatePanel cgc = new StatePanel();
    objRegistry.injectMembers(cgc);

    NavBackAction a = new NavBackAction();
    objRegistry.injectMembers(a);

    System.out.println("Injection of not annotated type:");
    if(cgc.getMachine() != a.getMachine()){
        System.out.println("    Failure: Separate instances!");
    } else {
        System.out.println("    As expected: Single instance.");
    }

    System.out.println("Injection of annotated with @Singleton type:");
    if(cgc.getSMachine() != a.getSMachine()){
        System.out.println("    Failure: Separate instances!");
    } else {
        System.out.println("    As expected: Single instance.");
    }
}

}

// ----------------------------------------------------------------------------------------------------------------- package bugsample.kernel; import com.google.inject.AbstractModule; public class KernelModule extends AbstractModule { @Override protected void configure() { this.bind(Machine.class).toInstance(new Machine()); this.bind(SingletonMachine.class).toInstance(new SingletonMachine()); } }

// ----------------------------------------------------------------------------------------------------------------- package bugsample.kernel;

public class Machine{ }

// ----------------------------------------------------------------------------------------------------------------- package bugsample.kernel;

import javax.inject.Singleton;

@Singleton public class SingletonMachine { }

// ----------------------------------------------------------------------------------------------------------------- package bugsample.kernel.ui;

import javax.inject.Inject;

import bugsample.kernel.Machine; import bugsample.kernel.SingletonMachine;

public class NavBackAction {

@Inject
protected Machine<String> machine;
@Inject
protected SingletonMachine<String> sMachine;

public Machine<String> getMachine() {
    return this.machine;
}

public SingletonMachine<String> getSMachine() {
    return this.sMachine;
}

}

// ----------------------------------------------------------------------------------------------------------------- package bugsample.kernel.ui;

import javax.inject.Inject;

import bugsample.kernel.Machine; import bugsample.kernel.SingletonMachine;

public class StatePanel{

@Inject
protected Machine<String> machine;
@Inject
protected SingletonMachine<String> sMachine;

public Machine<String> getMachine() {
    return this.machine;
}
public SingletonMachine<String> getSMachine() {
    return this.sMachine;
}

}

sameb commented 9 years ago

This is a bit confusing, but is WAI. You can see the issue more clearly if you add a call to binder().requireExplicitBindings().

The problem is you're binding Foo.class but then injecting Foo<Type>. Foo and Foo<Type> are two different things -- Guice doesn't know anything's bound to Foo<Type> and tries to create a just-in-time binding for it (resolving to the class & seeing no scoping annotation on it, so creating a new one each time).

powerbroker commented 9 years ago

Maybe you have an idea how to bind Foo<Type>?

sameb commented 9 years ago

bind(new TypeLiteral<Foo<Type>>() {}).toInstance(new MyFoo()) -- see the Binder javadoc @ http://google.github.io/guice/api-docs/latest/javadoc/index.html?com/google/inject/Binder.html, specifically the part that mentions TypeLiteral.

The weird syntax is unfortunately required because otherwise the generics data is erased.

sameb commented 9 years ago

Also, @Provides methods make this a little simpler, just have a method:

  @Provides Foo<Type> provideFoo() { return new MyFoo(); }
powerbroker commented 9 years ago

Thx. It seems @Provides method must be annotated with @Singleton or designed to reuse cached instance to enforce singleton behavior.