Issue 743 in google-guice: Move constructionContext.removeCurrentReference from ConstructorInjector.construct to ConstructorInjector.provision?

[google...@googlecode.com]

Status: New
Owner: ----
Labels: Type-Patch Component-Core

New issue 743 by mccu...@gmail.com: Move
constructionContext.removeCurrentReference from
ConstructorInjector.construct to ConstructorInjector.provision?
http://code.google.com/p/google-guice/issues/detail?id=743

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();
}
}
}
//-------------------------------------------------------------------


--
You received this message because this project is configured to send all
issue notifications to this address.
You may adjust your notification preferences at:
https://code.google.com/hosting/settings

[google...@googlecode.com]

Comment #1 on issue 743 by mccu...@gmail.com: Move

constructionContext.removeCurrentReference from
ConstructorInjector.construct to ConstructorInjector.provision?
http://code.google.com/p/google-guice/issues/detail?id=743

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.

[google...@googlecode.com]

Comment #2 on issue 743 by sberlin: Move

constructionContext.removeCurrentReference from
ConstructorInjector.construct to ConstructorInjector.provision?
http://code.google.com/p/google-guice/issues/detail?id=743

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.

[google...@googlecode.com]

Comment #3 on issue 743 by electrot...@gmail.com: Move

constructionContext.removeCurrentReference from
ConstructorInjector.construct to ConstructorInjector.provision?
http://code.google.com/p/google-guice/issues/detail?id=743

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!

[google...@googlecode.com]

Comment #4 on issue 743 by sberlin: Move

constructionContext.removeCurrentReference from
ConstructorInjector.construct to ConstructorInjector.provision?
http://code.google.com/p/google-guice/issues/detail?id=743

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();

[google...@googlecode.com]

Updates:
Status: Fixed

Comment #5 on issue 743 by cgr...@google.com: Move

constructionContext.removeCurrentReference from
ConstructorInjector.construct to ConstructorInjector.provision?
http://code.google.com/p/google-guice/issues/detail?id=743

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