Move constructionContext.removeCurrentReference from ConstructorInjector.construct to ConstructorInjector.provision?

[GoogleCodeExporter]

See https://groups.google.com/d/topic/google-guice/OeEEg2ELQ0M/discussion

TestCase from https://gist.github.com/electrotype/5108107:

//-------------------------------------------------------------------
import static org.junit.Assert.*;

import java.util.UUID;

import org.junit.Test;

import com.google.inject.AbstractModule;
import com.google.inject.Binding;
import com.google.inject.Guice;
import com.google.inject.Inject;
import com.google.inject.Injector;
import com.google.inject.assistedinject.Assisted;
import com.google.inject.assistedinject.FactoryModuleBuilder;
import com.google.inject.matcher.AbstractMatcher;
import com.google.inject.spi.ProvisionListener;

public class ExampleTest
{
    public static class ClassAAA
    {
        private final String id;
        private final boolean myParam;
        private final IClassBBBFactory classBBBFactory;
        private ClassBBB clasBBB;

        @Inject
        public ClassAAA(@Assisted boolean myParam,
                        IClassBBBFactory classBBBFactory)
        {
            this.myParam = myParam;
            this.id = UUID.randomUUID().toString();
            this.classBBBFactory = classBBBFactory;
        }

        public void init()
        {
            if(this.myParam)
            {
                this.clasBBB = this.classBBBFactory.create(UUID.randomUUID().toString());
            }
        }

        public String getId()
        {
            return this.id;
        }

        public ClassBBB getClassBBB()
        {
            return this.clasBBB;
        }  
    }

    public static class ClassBBB
    {
        private final IClassAAAFactory classAAAFactory;
        private ClassAAA clasAAACreatedByClassBBB;

        @Inject
        public ClassBBB(@Assisted String myParam,
                        IClassAAAFactory classAAAFactory)
        {
            this.classAAAFactory = classAAAFactory;
        }

        public void init()
        {
            // The problem is here. Even if the factory should return a new instance
            // of ClassAAA, it returns the one that is creating ClassBBB!
            this.clasAAACreatedByClassBBB = this.classAAAFactory.create(false);  
        } 

        public ClassAAA getClasAAACreatedByClassBBB()
        {
            return this.clasAAACreatedByClassBBB;
        }  
    }

    public static interface IClassAAAFactory
    {
        public ClassAAA create(boolean someParam);
    }

    public static interface IClassBBBFactory
    {
        public ClassBBB create(String someParam);
    }

    @Test
    public void bugGuiceConstructorInjector()
    {
        Injector injector = Guice.createInjector(new AbstractModule()
        {
            @Override
            protected void configure()
            {
                bindListener(new AbstractMatcher<Binding<?>>() 
                             {
                                @Override
                                public boolean matches(Binding<?> t)
                                {
                                    return  ClassAAA.class.isAssignableFrom(t.getKey().getTypeLiteral().getRawType()) ||
                                            ClassBBB.class.isAssignableFrom(t.getKey().getTypeLiteral().getRawType());
                                }
                             }, 

                             new ProvisionListener()
                            {
                                @Override
                                public <T> void onProvision(ProvisionInvocation<T> provision)
                                {
                                    Object obj = provision.provision();
                                    if(obj instanceof ClassAAA)
                                    {
                                        ((ClassAAA)obj).init();
                                    }
                                    if(obj instanceof ClassBBB)
                                    {
                                        ((ClassBBB)obj).init();
                                    }
                                }
                            });

                install(new FactoryModuleBuilder().build(IClassAAAFactory.class));
                install(new FactoryModuleBuilder().build(IClassBBBFactory.class));
            }
        }); 

        IClassAAAFactory classAAAFactory = injector.getInstance(IClassAAAFactory.class);
        ClassAAA classAAA = classAAAFactory.create(true);

        // Shouldn't be the same id!
        assertNotEquals(classAAA.getClassBBB().getClasAAACreatedByClassBBB().getId(), classAAA.getId());  

    }
}
//-------------------------------------------------------------------

Proposed patch:

//-------------------------------------------------------------------
diff --git a/core/src/com/google/inject/internal/ConstructorInjector.java 
b/core/src/com/google/inject/internal/Construc
index e71a25a..8f86db5 100644
--- a/core/src/com/google/inject/internal/ConstructorInjector.java
+++ b/core/src/com/google/inject/internal/ConstructorInjector.java
@@ -94,7 +94,6 @@ final class ConstructorInjector<T> {
         });
       }
     } finally {
-      constructionContext.removeCurrentReference();
       constructionContext.finishConstruction();
     }
   }
@@ -125,6 +124,8 @@ final class ConstructorInjector<T> {
           : userException;
       throw errors.withSource(constructionProxy.getInjectionPoint())
           .errorInjectingConstructor(cause).toException();
+    } finally {
+      constructionContext.removeCurrentReference();
     }
   }
 }
//-------------------------------------------------------------------

Original issue reported on code.google.com by mccu...@gmail.com on 7 Mar 2013 at 3:54

[GoogleCodeExporter]

Update: I've not seen any issues testing this locally, so I might commit this 
fix to the sisu-guice branch and let it bake some more.

Original comment by mccu...@gmail.com on 21 Mar 2013 at 3:51

[GoogleCodeExporter]

Thanks, Stuart -- I completely forgot about this, so haven't had a chance to 
test it internally.  I'll give it a go this weekend & see how things fare.

Original comment by sberlin on 21 Mar 2013 at 3:55

[GoogleCodeExporter]

Is this fix going to be included in the 3.1.0 release? I have to say I begin to 
have a lot of lazy init() calls to workaround the issue. Thanks!

Original comment by electrot...@gmail.com on 30 Mar 2013 at 2:51

[GoogleCodeExporter]

FYI, things are looking pretty good, I don't see any breakages from the change 
so far, so I'm inclined to commit it.  FWIW, I rewrote the test a bit so it 
doesn't require assistedinject.  See below

--

  public void testProvisionIsNotifiedAfterContextsClear() {
    Injector injector = Guice.createInjector(new AbstractModule() {
      @Override
      protected void configure() {
        bindListener(Matchers.any(), new ProvisionListener() {
          @Override
          public <T> void onProvision(ProvisionInvocation<T> provision) {
            Object provisioned = provision.provision();
            if (provisioned instanceof X) {
              ((X)provisioned).init();
            } else if (provisioned instanceof Y) {
              X.createY = false;
              ((Y)provisioned).init();
            }
          }
        });
      }
    });

    X.createY = true;
    X x = injector.getInstance(X.class);
    assertNotSame(x, x.y.x);
    assertFalse("x.ID: " + x.ID + ", x.y.x.iD: " + x.y.x.ID, x.ID == x.y.x.ID);
  }

  private static class X {
    final static Random RND = new Random();
    static boolean createY;

    final int ID = RND.nextInt();
    final Provider<Y> yProvider;
    Y y;

    @Inject X(Provider<Y> yProvider) {
      this.yProvider = yProvider;
    }

    void init() {
      if (createY) {
        this.y = yProvider.get();
      }
    }
  }

  private static class Y {
    final Provider<X> xProvider;
    X x;

    @Inject Y(Provider<X> xProvider) {
      this.xProvider = xProvider;
    }

    void init() {
      this.x = xProvider.get();
    }
  }

Original comment by sberlin on 30 Mar 2013 at 6:18

[GoogleCodeExporter]

Fixed in: 
https://code.google.com/p/google-guice/source/detail?r=2cc8ce904aff3d46a55cb6b88
6e975516a923524

Original comment by cgruber@google.com on 16 May 2013 at 6:45

• Changed state: Fixed