Issue with AssistedInject and bindListener() in Guice 3.0

[elect...@gmail.com]

First, here's a unit test for the issue : https://gist.github.com/electrotype/5105100

I'm trying to use a listener to call an "init()" method on my classes when they are constructed. I also use AssistedInject factories.

I think the problem is that in com.google.inject.internal.ConstructorInjector#construct(...) , this code is supposed to prevent infinite loops :

    T t = constructionContext.getCurrentReference();
    if (t != null) {
      return t;
    }

To remove the "current reference", removeCurrentReference() is called in the finally clause :

    } finally {
      constructionContext.removeCurrentReference();
    }

The problem is that before this removal method is called, the listeners are called :

      membersInjector.notifyListeners(t, errors);

It seems that this is the root of my issue, because the reference still exists when my "init()" methods are called (via the listeners), so when they try to create an instance using the factories, they get the cached instance instead of a new one!

Does this mean I can't use a listener to call my init() methods? Are there some workarounds?

Thanks in advance!

[Stuart McCulloch]

You might want to look at the ProvisionListener hook that's been added in trunk:

https://groups.google.com/d/msg/google-guice/wRPmutuepmM/5TbMHMm3wJUJ

http://code.google.com/p/google-guice/issues/detail?id=78#c16

It lets you intercept after an instance has been completely provisioned (ie. fully injected, including circular proxies) but before it's returned from the injector to the caller.

--
You received this message because you are subscribed to the Google Groups "google-guice" group.
To unsubscribe from this group and stop receiving emails from it, send an email to google-guice...@googlegroups.com.
To post to this group, send email to google...@googlegroups.com.
Visit this group at http://groups.google.com/group/google-guice?hl=en.
For more options, visit https://groups.google.com/groups/opt_out.
 
 

[Sam Berlin]

I agree that this seems wrong with AssistedInject, but I'm not clear on what you'd otherwise expect. If it didn't cache, then you'd fail with a StackOverflowException (since you'd recurse forever).

sam

[elect...@gmail.com]

You are right, my test is not the best to represent the issue. Of course my real code is more complex than that, but I've updated the test to show that the issue occures even if there is no overflow possible : https://gist.github.com/electrotype/5105100

Thanks for the help!

[elect...@gmail.com]

Excellent, thanks! I think this is exactly what I need.

[elect...@gmail.com]

Hummm.... It doesn't seem to work! Maybe I'm missing something?

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

I use :

        <dependency>
            <groupId>org.sonatype.sisu</groupId>
            <artifactId>sisu-guice</artifactId>
            <version>3.1.3</version>
        </dependency>

        <dependency>
            <groupId>org.sonatype.sisu.inject</groupId>
            <artifactId>guice-assistedinject</artifactId>
            <version>3.1.3</version>
        </dependency>

Thank you.

On Wednesday, March 6, 2013 9:59:28 PM UTC-5, Stuart McCulloch wrote:

[Stuart McCulloch]

On 7 Mar 2013, at 13:39, elect...@gmail.com wrote:

Hummm.... It doesn't seem to work! Maybe I'm missing something?

Unfortunately not - by tracing through your test I can see it still hits the issue with the constructionContext reference since that's only removed after all the provisioning hooks are done. I'm wondering whether it makes sense to move the reference removal into a finally block in the provision method - ie. after custom injection and injection listeners, but before all the provision hooks have completed. This fixes your testcase and all the unit tests still pass, but I'd need to do more thorough investigation into whether this could cause problems elsewhere. (Would appreciate Sam's input since he wrote the ProvisionListener feature.)

diff --git a/core/src/com/google/inject/internal/ConstructorInjector.java b/core/src/com/google/inject/internal/ConstructorInjector.java

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

     }

   }

 }

[Sam Berlin]

While writing, I considered notifying a ProvisionListener to still be "part of the construction".  I remember wondering about this specific part (whether the finally should be inside or outside), and ended up settling on "outside".  But, I can't really give you any concrete reason of why I settled on that.

There's a few different places we notify provision listeners (and I added one more internally, to notify on toInstance/bindConstant bindings too... still waiting on the sync-stuff to be fixed to push that out), so I'd have to analyze all of them to really figure it out.

sam

[elect...@gmail.com]

Since I don't want to mess with Guice code directly, I'll wait for the next release before trying to use those listeners.

My temporary workaround will be to lazily call the "init()" methods the first time my objects are used, which is not a big deal in my case since they only have one main public method. It's not the ideal, but it works...

Thanks you both for your help and for Guice in general!

[Stuart McCulloch]

On 7 Mar 2013, at 15:18, Sam Berlin wrote:

While writing, I considered notifying a ProvisionListener to still be "part of the construction".  I remember wondering about this specific part (whether the finally should be inside or outside), and ended up settling on "outside".  But, I can't really give you any concrete reason of why I settled on that.

There's a few different places we notify provision listeners (and I added one more internally, to notify on toInstance/bindConstant bindings too... still waiting on the sync-stuff to be fixed to push that out), so I'd have to analyze all of them to really figure it out.

Thanks, I've logged this under http://code.google.com/p/google-guice/issues/detail?id=743

I'll also run some of my local ProvisionListener-based projects with a patched build to see what happens.

[elect...@gmail.com]

I'm currently trying Guice 4.0-beta.

I still have a problem with this issue ( http://code.google.com/p/google-guice/issues/detail?id=743 ).
The problem is that, even with the new patch, listeners are still called with the reference being cached.

In com.google.inject.internal.ConstructorInjector#provision(...) :

My object reference of type X is stored (line 115) and the listeners are called (line 118) before the reference is removed (line 128). I have a InjectionListener that calls an init() method on the object. This init() method does a lot of things, which ultimately create more instances of type X. Since the first instance is still cached, all "created" references are this exact same object!

I'll try to write an simplified test case tonight or tomorrow.

[Sam Berlin]

Thanks for following up on this. A simplified test case would he much appreciated.

sam

--
You received this message because you are subscribed to the Google Groups "google-guice" group.
To unsubscribe from this group and stop receiving emails from it, send an email to google-guice...@googlegroups.com.
To post to this group, send email to google...@googlegroups.com.

Visit this group at http://groups.google.com/group/google-guice.

[elect...@gmail.com]

Sorry that I'm still not 100% sure, and I do not have a test case to show, but I'm pretty sure I did find the main cause of this "issue" in my application.

I created an interface :

-------------
public interface IInitableAfterCreation
{
    public void init();
}
-------------

And this is what I check in the Matcher of my InjectionListener :

-------------
@Override
public boolean matches(Binding<?> t)
{
    return IInitableAfterCreation.class.isAssignableFrom(t.getKey().getTypeLiteral().getRawType());
}
-------------

The problem is that I was using this IInitableAfterCreation interface on my implementations objects, not on their interfaces. For example :

-------------
public class MyWidget implements IMyWidget, IInitableAfterCreation
{
    @Override
    public void init()
    {
    }
    // ...
}
-------------

This doesn't work! But if I make the IMyWidget interface extend IInitableAfterCreation , then it works :

-------------
public interface IMyWidget extends IInitableAfterCreation {}

public class MyWidget implements IMyWidget
{
    @Override
    public void init()
    {
    }
    // ...
}
-------------

Is this the way it should work? Some of my IMyWidget implementations that don't need to be initialized at startup will still have to implement the init() method?

Anyway, the issue seems less important than what I though, that's good news! :-)

Let me know if you still want a test case or if this is not a real problem actually (I would appreciate to better understand the way it works though!). Thanks!

[Nate Bauernfeind]

Did you try using Matchers.subclassesOf(InitableAfterCreation.class)?

[Sam Berlin]

Interesting. I haven't fully digested it, but if its causing surprising behavior that you're having trouble explaining, then I think a test case would be useful.  If only so we can examine it more closely and figure out if there is indeed a bug.

[elect...@gmail.com]

I will, thank you.

[elect...@gmail.com]

Let me a couple of days (this weekend) and I will try to simply my original issue to a test case. I still think that maybe something funny is going on under some specific conditions.

[elect...@gmail.com]

*simplifly*
 

[Christian Gruber]

As a side note, if you don't have a required inheritance hierarchy in
your context, you can make an AbstractInitableWidget which has a
default, overridable init() and implements IInitableAfterCreation, just
to cut down on boilerplate. It would be good if Matching on the
subclassesOf bit works though, as the above is sort of janky.

c.

On 21 Aug 2013, at 3:55, elect...@gmail.com wrote:

> Let me a couple of days (this weekend) and I will try to simply my
> original
> issue to a test case. I still think that maybe something funny is
> going on
> under some specific conditions.
>
>
> On Tuesday, August 20, 2013 10:57:47 PM UTC-4, Sam Berlin wrote:
>>
>> Interesting. I haven't fully digested it, but if its causing
>> surprising
>> behavior that you're having trouble explaining, then I think a test
>> case
>> would be useful. If only so we can examine it more closely and
>> figure out
>> if there is indeed a bug.

>> On Aug 20, 2013 9:20 PM, <elect...@gmail.com <javascript:>> wrote:
>>
>>> Sorry that I'm still not 100% sure, and I do not have a test case to
>>> show, but I'm pretty sure I did find the main cause of this "issue"
>>> in my
>>> application.
>>>
>>> I created an interface :
>>>
>>> -------------
>>> public interface IInitableAfterCreation
>>> {
>>> public void init();
>>> }
>>> -------------
>>>

>>> And this is what I check in the *Matcher* of my *InjectionListener*
>>> :

>>>
>>> -------------
>>> @Override
>>> public boolean matches(Binding<?> t)
>>> {
>>> return
>>> IInitableAfterCreation.class.isAssignableFrom(t.getKey().getTypeLiteral().getRawType());
>>> }
>>> -------------
>>>

>>> The problem is that I was using this *IInitableAfterCreation*
>>> interface
>>> on my *implementations* objects, not on their interfaces. For

>>> example :
>>>
>>> -------------
>>> public class MyWidget implements IMyWidget, IInitableAfterCreation
>>> {
>>> @Override
>>> public void init()
>>> {
>>> }
>>> // ...
>>> }
>>> -------------
>>>

>>> This doesn't work! But if I make the* IMyWidge*t *interface* extend
>>> *IInitableAfterCreation
>>> *, then it works :

>>>
>>> -------------
>>> public interface IMyWidget extends IInitableAfterCreation {}
>>>
>>> public class MyWidget implements IMyWidget
>>> {
>>> @Override
>>> public void init()
>>> {
>>> }
>>> // ...
>>> }
>>> -------------
>>>

>>> Is this the way it should work? Some of my *IMyWidget*

>>> implementations
>>> that don't need to be initialized at startup will still have to
>>> implement

>>> the *init()* method?

>>>
>>> Anyway, the issue seems less important than what I though, that's
>>> good
>>> news! :-)
>>>
>>> Let me know if you still want a test case or if this is not a real
>>> problem actually (I would appreciate to better understand the way it
>>> works
>>> though!). Thanks!
>>>
>>>

>>> ********

>>>
>>> --
>>> You received this message because you are subscribed to the Google
>>> Groups
>>> "google-guice" group.
>>> To unsubscribe from this group and stop receiving emails from it,
>>> send an

>>> email to google-guice...@googlegroups.com <javascript:>.

>>> To post to this group, send email to

>>> google...@googlegroups.com<javascript:>
>>> .

>>> Visit this group at http://groups.google.com/group/google-guice.
>>> For more options, visit https://groups.google.com/groups/opt_out.
>>>
>>
>
> --
> You received this message because you are subscribed to the Google
> Groups "google-guice" group.
> To unsubscribe from this group and stop receiving emails from it, send
> an email to google-guice...@googlegroups.com.
> To post to this group, send email to google...@googlegroups.com.
> Visit this group at http://groups.google.com/group/google-guice.
> For more options, visit https://groups.google.com/groups/opt_out.

Christian Gruber :: Google, Inc. :: Java Core Libraries :: Dependency
Injection
email: cgr...@google.com :::: mobile: +1 (646) 807-9839

[elect...@gmail.com]

This is not probably not my last test case for this issue, but it's at least a temp test case that shows one issue (not sure if it's the main issue in my case though) :

https://gist.github.com/electrotype/6296350

A bindListener's Matcher that checks for the IInitableAfterCreation which is used on the concrete class :

- Works if no assisted factory is involved
- Doesn't work if an assisted factory is involved

P.S. I can't spell! S-I-M-P-L-I-F-Y! :-) Sorry for that, english is not my main language.

[elect...@gmail.com]

Indeed, thank for the idea.
The problem is that my project is some kind of framework, and I would like to say to developers that they only have to implements IInitableAfterCreation on their objects for the framework to automatically calls init() on them! Otherwise I would have to explain them that they have to extend it from theirs interfaces.

[Christian Gruber]

Not uncommon. I don't actually like the solution I proposed, but it can
clean up some boilerplate in some cases - possibly not yours, sadly.

c.

On 21 Aug 2013, at 9:15, elect...@gmail.com wrote:

> Indeed, thank for the idea.
> The problem is that my project is some kind of framework, and I would
> like
> to say to developers that they only have to implements

> *IInitableAfterCreation
> *on their objects for the framework to automatically calls init() on

> them!
> Otherwise I would have to explain them that they have to extend it
> from
> theirs interfaces.
>
>
> On Wednesday, August 21, 2013 10:21:01 AM UTC-4, Christian Gruber
> wrote:
>>
>> As a side note, if you don't have a required inheritance hierarchy in
>> your context, you can make an AbstractInitableWidget which has a
>> default, overridable init() and implements IInitableAfterCreation,
>> just
>> to cut down on boilerplate. It would be good if Matching on the
>> subclassesOf bit works though, as the above is sort of janky.
>>
>> c.
>>
>>
>

[elect...@gmail.com]

In com.google.inject.matcher.Matchers$SubclassesOf

--------------
public boolean matches(Class subclass) {
    return superclass.isAssignableFrom(subclass);
}
--------------

It sadly does the same thing than what I explicitly do.

On Tuesday, August 20, 2013 10:44:28 PM UTC-4, Nate Bauernfeind wrote:

[elect...@gmail.com]

Actually, I'm now pretty sure this was the cause of my issue. All my tests now passed if I use IInitableAfterCreation on the interfaces instead of on the concrete classes.

So, except if I find a case where the "bindListener involving assisted factories" doesn't seems to be the core of the problem, I don't think I will have other test cases to show.

Thanks again for your work on Guice.

[Nate Bauernfeind]

I was just about to suggest annotating the init method on the interface with the @Inject annotation.

Turns out, that doesn't work either.

[Stuart McCulloch]

I'm on vacation atm without access to a pc, but afaict you're matching on Binding.getKey which is the "from" part of the binding. Without assistedinject there will be a just-in-time untargetted binding that matches; ie the implementation type, equivalent to bind(MyImpl.class);

With assistedinject the implementation is hidden inside the factory binding, which is why the matcher no longer matches unless you add the init'able interface to the binding interface.

IIRC it is possible get the implementation type from the assistedinject SPI binding type and from there look for the init'able interface. Should just be a matter of extending your matcher to be aware of other more specific binding types.

Will take a closer look when I get back.

--

[elect...@gmail.com]

Stuart, what you say makes perfect sense. But I didn't find how to access the implementation type from the Binding. I think the problem is that com.google.inject.internal.ConstructorBindingImpl is not public so I can't cast the binding to it to access the constructorInjectionPoint. I guess I could use some reflection workarounds here, but I would prefere not. Maybe there is another way to get that implementation type?

This is what I see when breaking into the matches() method, ClassAAA is there, I just don't know how to access it :

http://i.imgur.com/WDVDiPb.png

Thanks in advance if you look into that issue when you're back. And have some good vacations!

[Sam Berlin]

I think what Stuart is saying is that the Binding itself doesn't expose the info because, well, it doesn't have the info.  You need to use the AssistedInject SPI extension to get the info you want.  The getAssistedMethod method should expose the info you want in AssistedMethod.

 sam

[elect...@gmail.com]

Thanks Sam. By reading about those SPI extensions, I found the way to get the implementation type in the matches() method :

https://gist.github.com/electrotype/6299241

So it's now working! All my tests passed even with the IInitableAfterCreation interface set on the implementation classes.

The only thing I'd like to be sure is if I do I have to check for other kind of binders than ConstructorBinding for my bindListener to always work? Or is this the only case where a direct binding.getKey().getTypeLiteral().getRawType() won't be the type that has to be validated agains the IInitableAfterCreation interface? In my application, for now, checking for ConstructorBinding seems to be enough.

[Sam Berlin]

Hrm.  It seems like your particular problem is because ProvisionListener is designed to notify you when a particular binding is provisioned, and the binding it supplies is the one for the exact thing being provisioned. So take this example:

  interface Foo {}

  class FooImpl implements Foo {}

  bind(Foo.class).to(FooImpl.class);

... a ProvisionListener specifically for exact(Foo.class) will find nothing, because Foo is never provisioned.  OTOH, a listener for exact(FooImpl.class) will match, because FooImpl is being provisioned.  Even if someone is injecting Foo and not FooImpl.

In your case, it's as-if the code was more like:

  interface Foo {}

  class FooImpl implements Foo, IInitableAfterCreation {}

  bind(Foo.class).toConstructor(cxtorFor(FooImpl.class));

... in that case, a listener for exact(Foo.class) *will* match, because that's the actual binding being provisioned -- it's not linked to any other binding.  However, binding.getKey() will be Foo.class, which doesn't implement IInitableAfterCreation.  But, the constructor that loads it is for a FooImpl, and FooImpl *does* implement the extra interface.  That's why you need the extra matching.

Off the top of my head, I can't think of any other kind of binding that would lead to the same scenario.  Perhaps Providers, with something like:

  @Provides Foo provideFoo() { return new FooImpl(..); }  

.. with the same class setup as above, you'll be notified of a Foo binding, although Guice in this scenario has no way of knowing what the contents of the method are, so you can't use the same kind of code to detect for IInitableAfterCreation.  (A normal toProvider binding would get the same effect if the contents of the get() method were the same.)

If it's necessary to check for those types, then you could just have the matcher return 'true' always for provider bindings, and your onProvision method will be the same (because it already does the instanceof check).

 sam

[elect...@gmail.com]

If it's necessary to check for those types, then you could just have the matcher return 'true' always for provider bindings, and your onProvision method will be the same (because it already does the instanceof check).

Now I feel very stupid for not having thought of that! :-)

Do you think it could lead of significative performance degradation to always returning true there?

Thanks for that clear explanation!

[Sam Berlin]

There's some performance overhead for having ProvisionListeners installed, but not much.  I think most big servers here at have them installed, and we're getting along well with pretty high QPS on many of them.  So it shouldn't be a big problem (unless, of course, you measure it to be a problem... and then you have some outs).

 sam