singletions recreated many times on error, memebrs injected values may be null

[gissuebot]

From kvaster on December 25, 2011 05:39:32

1. Singletons should be not recreated many times if there was error whil constructing it. i.e.:
   class T1, class T2 depends on T1 and class T3 depends on T1.
   All of them singletons. Guice will try to create T1 three times: when creating itself, when creating T2 and T3.

2. Injected member may be null in case of dependent bean creation error. i.e.:
   T1 class, T2 class have member of T1.
   T1 failed to create due to exception, but T2 will not fail, rather it will have T1 fieled equal to null.

Attached patch to fix issues.

Attachment: gist
   guice-singleton-recreation.patch

Original issue: http://code.google.com/p/google-guice/issues/detail?id=676

[gissuebot]

From mcculls on March 14, 2012 15:51:18

Hi - a testcase for each of the scenarios would be really helpful to demonstrate the problem and validate the fix.

[gissuebot]

From kvaster on March 24, 2012 03:57:09

class Test1
{
   Test1()
   {
       throw new RuntimeException();
   }
}

class Test2
{
   @Inject
   private Test1 member;
}

class Test3
{
   @Inject
   private Test1 member;
}

Just bind all classes as eager singletons.

1. Guice will fail to create Test1 class, but it will create Test2 normally. It should fail creating Test2 cause 'member' is not nullable.

2. Guice will try to create Test1 class twice. First when creating Test2 class and second one when creating Test3 class. It's really annoying that guice will try to create failing singleton 1000 times...

[gissuebot]

From sberlin on December 05, 2013 15:52:13

Please provide a patch in JUnit form, with an assertion on the right behavior, so that it currently fails.

[gissuebot]

From mikhail.mazursky on April 11, 2014 21:01:54

Here is the testcase using JUnit. It currently fails but with patch applied it does not.

I wish this fix can make it into 4.0 release - few days ago I had a 10+ MB of output in log file from an exception thrown by Guice with 900+ messages in it and that is completely undebuggable.

p.s. there is a minor exception handling problem with the patch but it is probably easy to handle it properly.

Attachment: gist
   FailingSingletonTest.java

[gissuebot]

From mikhail.mazursky on May 04, 2014 20:43:51

Can someone from committers take a look at this issue please? It should be straightforward to address it.

[stevenschlansker]

I have independently discovered / reproduced this bug. Here is my test case:

public class TestGuiceThrowDoubleSingleton {
    @Test
    public void testGuiceCreatesSingletonTwice() throws Exception {
        try {
            Guice.createInjector(Stage.PRODUCTION, new AbstractModule() {
                @Override
                protected void configure() {
                    bind (Boom.class).in(Scopes.SINGLETON);
                    bind (Object.class).to(Boom.class);
                }
            }).getInstance(Boom.class);
        } catch (CreationException expected) { }
        Assert.assertEquals(1, Boom.nCalls);
    }

    public static class Boom {
        static int nCalls = 0;
        public Boom() {
            nCalls++;
            throw new IllegalArgumentException("kaboom!");
        }
    }
}

[stevenschlansker]

This bug has been open since 2011 :( @cgdecker @iflan sorry to ping random contributors personally, but is there any chance this could get some love? It is a serious usability problem if you ever encounter it - it took me hours to track down what was going on!

[tavianator]

@stevenschlansker That behaviour is expected, at least to me. If you want Guice to remember failures in a scope you can use https://github.com/google/guice/wiki/ThrowingProviders.

The second part of this bug report, about injecting null for a failed singleton, could be an actual bug but I haven't seen a test case that shows it.

[iflan]

@stevenschlansker Is the problem really that even though the Singleton injection happens once (and fails), you can then actually retrieve other objects that depend on the failed Singleton without getting an error? If so, that's not what your test shows.

[stevenschlansker]

@tavianator I don't agree. ThrowingProvider is useful when you expect there to potentially be a (usually checked) exception. This behavior happens even when you do not expect an exception, say an accidental NullPointerException. Asking every singleton to be wrapped in a ThrowingProvider to guard against coding errors is IMO the wrong solution (and completely impractical), when we can improve Guice core to handle it more gracefully.

The attached test case from Mikhail above shows the null injection behavior. It's a little hard to find due to the gissuebot importing, but it's there.

[stevenschlansker]

@iflan I'd guessed that these two problems are both related to Guice's poor handling of Singleton creation failure. You're right that they may be two separate issues, but I suspect underneath the hood they are not.

[tavianator]

@stevenschlansker I completely agree that you shouldn't use a ThrowingProvider to guard against NullPointerExceptions. I guess my point is that scopes themselves shouldn't be caching failures because something that failed once may succeed later.

I can see an argument for caching failures that occur during injector creation, since it's going to fail anyway at that point. Might as well not report 100 duplicate errors. But after the injector is created, caching failures is a bad idea.

By the way, I looked at that test case, and I don't see where it shows that null is injected anywhere. In fact the file does not contain the string "null" at all.

[stevenschlansker]

@tavianator You're right, I can't seem to reproduce that behavior. But I swore I hit it before. Maybe it got fixed sometime in the intervening 3 years. But I think the other fix is still valid, and is probably sufficient to close this bug, which will make everyone happy I am sure :)

I have taken Mikhail's code and mine, merged them and rebased to current Guice, and packaged it up as #883. Hopefull you and @iflan like this improvement?