Guice 3.0 Status

[Sam Berlin]

Here's another status update (in an effort to get 3.0 out soon) --

Open issues:

 * 366 - @Provides doesn't respect @Nullable
   Status: Patch attached [from me], needs review (it adds APIs).

 * 436 - Provide access to Guice's own internal TypeConverters
   Status: Patch attached [from Stuart], needs review (it adds APIs, makes some classes public, and changes a bit of the internals).

 * 508 - AOP visibility checks should include the method return type.
   Status: Patch attached [from Stuart], just waiting on some tests before applying.

 * 509 - Cache result of Class.getAnnotations()
   Status: Perf improvement request, no patch attached.

 * 510 -  Remove InjectorBuilder
   Status: Rationale for keeping InjectorBuilder listed in the comments along with other possible scenarios, awaiting replies.

 * 513 - Why do we need Injector.getAllBindings?
   Status: Rationale for why it's useful in the comments, awaiting replies.

 * 514 - Do we need Injector.getExistingBinding()?
   Status: Rationale for why it's useful (but not strictly necessary) in the comments, awaiting replies.

 * 515 - What are the use cases for @Toolable?
   Status: Use cases listed in the comments, awaiting replies.

 * 516 - Investigate duplicate binding filtering
   Status: Performance implications & rationale explained in comments, awaiting replies.

 * 517 - Fix classloader leak
   Status: More questions asked in the comments, awaiting answers.

 * 518 -  Hide Key.ofType()
   Status: Question opened, no replies.  Awaiting comments.

 * 519 - toProvider() should use Guice's Provider, not JSR-330
   Status: Lots of discussion in comments, no resolution.

 * Cleanup new guice-persist extension.
   Status: Some code review changes applied... is it stable?

 * maven2 POM?
   Status: I don't know?

 * Include guice-grapher extension in release
   Status: Open question, awaiting comments.

Closed issues:
 * Undeprecate Guice.createInjector.  [DONE, r1180]
 * Fix servlet extension to allow multiple modules/filters/injectors in one JVM. [DONE, r1187]
 * Add "@since 3.0" to new APIs.  [DONE,  r1183]
 * Remove ScopeChecker [DONE, r1179]
 * Refactor internal package (move utility class to .internal.util) [DONE, r1185]
 * Fix overridden @Inject method behavior [DONE, r1177]
 * JIT providers / autobinders [SHELVED]

sam

[Dhanji R. Prasanna]

Hey thanks for this list...

Persist extension is wanting, It'll be another week at least before I can give it the right attention. For servlet, we need to build some kind of introspection API that's similar to injector's, but that can wait I suppose.

If we can turn injector builder's options into some kind of module config that would rule...

Dhanji.

--
You received this message because you are subscribed to the Google Groups "google-guice-dev" group.
To post to this group, send email to google-g...@googlegroups.com.
To unsubscribe from this group, send email to google-guice-d...@googlegroups.com.
For more options, visit this group at http://groups.google.com/group/google-guice-dev?hl=en.

[Sam Berlin]

Oh, right!  I forgot about that extension introspection stuff (after saying I'd look at it)....  Oops.  Added a new issue @ http://code.google.com/p/google-guice/issues/detail?id=524 .

Re: injector builder -> module config... see the comments @ http://code.google.com/p/google-guice/issues/detail?id=510 .  I'm not convinced that setting the options in a Module makes sense for these kinds of settings.

sam

[Robbie Vanbrabant]

Just out of curiosity, what do you mean with module config?

Cheers
Robbie

On Jul 18, 2:07 am, "Dhanji R. Prasanna" <dha...@gmail.com> wrote:
> Hey thanks for this list...
>
> Persist extension is wanting, It'll be another week at least before I can
> give it the right attention. For servlet, we need to build some kind of
> introspection API that's similar to injector's, but that can wait I suppose.
>
> If we can turn injector builder's options into some kind of module config
> that would rule...
>
> Dhanji.
>

> On Sun, Jul 18, 2010 at 9:56 AM, Sam Berlin <sber...@gmail.com> wrote:
> > Here's another status update (in an effort to get 3.0 out soon) --
>
> > Open issues:
>

> >  * 366 <http://code.google.com/p/google-guice/issues/detail?id=366> -

> > @Provides doesn't respect @Nullable
> >    Status: Patch attached [from me], needs review (it adds APIs).
>

> >  * 436 <http://code.google.com/p/google-guice/issues/detail?id=436> -

> > Provide access to Guice's own internal TypeConverters
> >    Status: Patch attached [from Stuart], needs review (it adds APIs, makes
> > some classes public, and changes a bit of the internals).
>

> >  * 508 <http://code.google.com/p/google-guice/issues/detail?id=508> - AOP

> > visibility checks should include the method return type.
> >    Status: Patch attached [from Stuart], just waiting on some tests before
> > applying.
>

> >  * 509 <http://code.google.com/p/google-guice/issues/detail?id=509> -

> > Cache result of Class.getAnnotations()
> >    Status: Perf improvement request, no patch attached.
>

> >  * 510 <http://code.google.com/p/google-guice/issues/detail?id=510> -

> > Remove InjectorBuilder
> >    Status: Rationale for keeping InjectorBuilder listed in the comments
> > along with other possible scenarios, awaiting replies.
>

> >  * 513 <http://code.google.com/p/google-guice/issues/detail?id=513> - Why

> > do we need Injector.getAllBindings?
> >    Status: Rationale for why it's useful in the comments, awaiting replies.
>

> >  * 514 <http://code.google.com/p/google-guice/issues/detail?id=514> - Do

> > we need Injector.getExistingBinding()?
> >    Status: Rationale for why it's useful (but not strictly necessary) in
> > the comments, awaiting replies.
>

> >  * 515 <http://code.google.com/p/google-guice/issues/detail?id=515> - What

> > are the use cases for @Toolable?
> >    Status: Use cases listed in the comments, awaiting replies.
>

> >  * 516 <http://code.google.com/p/google-guice/issues/detail?id=516> -

> > Investigate duplicate binding filtering
> >    Status: Performance implications & rationale explained in comments,
> > awaiting replies.
>

> >  * 517 <http://code.google.com/p/google-guice/issues/detail?id=517> - Fix

> > classloader leak
> >    Status: More questions asked in the comments, awaiting answers.
>

> >  * 518 <http://code.google.com/p/google-guice/issues/detail?id=518> -

> > Hide Key.ofType()
> >    Status: Question opened, no replies.  Awaiting comments.
>

> >  * 519 <http://code.google.com/p/google-guice/issues/detail?id=519> -

> > toProvider() should use Guice's Provider, not JSR-330
> >    Status: Lots of discussion in comments, no resolution.
>
> >  * Cleanup new guice-persist extension.
> >    Status: Some code review changes applied... is it stable?
>
> >  * maven2 POM?
> >    Status: I don't know?
>
> >  * Include guice-grapher extension in release
> >    Status: Open question, awaiting comments.
>
> > Closed issues:

> >  * Undeprecate Guice.createInjector.  [DONE, r1180<http://code.google.com/p/google-guice/source/detail?r=1180>
> > ]

> >  * Fix servlet extension to allow multiple modules/filters/injectors in one

> > JVM. [DONE, r1187<http://code.google.com/p/google-guice/source/detail?r=1187>
> > ]
> >  * Add "@since 3.0" to new APIs.  [DONE,  r1183<http://code.google.com/p/google-guice/source/detail?r=1183>
> > ]
> >  * Remove ScopeChecker [DONE, r1179<http://code.google.com/p/google-guice/source/detail?r=1179>
> > ]

> >  * Refactor internal package (move utility class to .internal.util) [DONE,

> > r1185 <http://code.google.com/p/google-guice/source/detail?r=1185>]
> >  * Fix overridden @Inject method behavior [DONE, r1177<http://code.google.com/p/google-guice/source/detail?r=1177>
> > ]

> >  * JIT providers / autobinders [SHELVED]
>
> > sam
>
> > --
> > You received this message because you are subscribed to the Google Groups
> > "google-guice-dev" group.
> > To post to this group, send email to google-g...@googlegroups.com.
> > To unsubscribe from this group, send email to

> > google-guice-d...@googlegroups.com<google-guice-dev%2Bunsu...@googlegroups.com>
> > .

[Max Bowsher]

On 18/07/10 00:56, Sam Berlin wrote:
> * maven2 POM?
> Status: I don't know?

Awaiting a Guice committer to take action on my mails to the list on
2010-07-03, entitled "Merge Request: 2.0-maven" and "...and then update
the POMs for 3.0".

Max.

[Sam Berlin]

Would you be able to open an issue with the patch & any more information about applying/testing it, Max?  (Easier to keep track and all that..)

sam

[Sam Berlin]

Take a look at the comments in http://code.google.com/p/google-guice/issues/detail?id=510 , comment #1.  I think it's option 1) and/or 5) listed there.

sam

To unsubscribe from this group, send email to google-guice-d...@googlegroups.com.

[Max Bowsher]

On 19/07/10 14:21, Sam Berlin wrote:
> Would you be able to open an issue with the patch & any more information
> about applying/testing it, Max? (Easier to keep track and all that..)

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

[Endre Stølsvik]

Please evaluate the proxies - an option to turn that idea off is my personal top priority for Guice. The construction of objects should be done such that they aren't needed, or, if cycles exists, any setter/field injection on any of the classes in question should resolve it.

Endre.

--

[Sam Berlin]

I'm not sure what you mean by "please evaluate the proxies".  Do you mean circular proxies, and you want to disable them?  See InjectorBuilder.disableCircularProxies.

sam

2010/7/23 Endre Stølsvik <stol...@gmail.com>

[Sam Berlin]

Yes, it should disable it. I *think* the only time guice created a proxy was when it detected a circular dependency.  If you build the injector with this option, then where guice would have created a proxy, it instead throws an exeption saying circular proxies are disabled. 

Sam

On Jul 24, 2010, at 5:00 AM, Endre Stølsvik <stol...@gmail.com> wrote:

Oh, I didn't realize that it had been implemented!

If this method totally disables Guice's use of proxies (so that it never proxies an object as it did before, you will get the actual objects injected, always), then that is what I've been wanting since day 0! Thanks!

Kind regards,

Endre.

[Endre Stølsvik]

Btw, there have been talk of a dependency graph algorithm thing to go with this feature. Apparently Guice of before just instantiated the objects as it went along, proxying here and there if it didn't have the objects, not distinguishing between constructor and setter/field injecting - thus implementing a setter to "resolve" the fact that you were given a proxy didn't necessarily work out. Guice should rather instantiate after some logic; I guess building some kind of tree of the dependencies, instantiating the leaf nodes (those w/o dependencies) first, then going upwards. Any circle in the constructor graph should resolve correctly by having any of the classes in the circle implement a setter or a field injection taking the object(s) it depends on which is a part of the circle.

Is this how it was implemented?

Thanks again,

Endre.

[Sam Berlin]

2010/7/26 Endre Stølsvik <stol...@gmail.com>

Btw, there have been talk of a dependency graph algorithm thing to go with this feature. Apparently Guice of before just instantiated the objects as it went along, proxying here and there if it didn't have the objects, not distinguishing between constructor and setter/field injecting - thus implementing a setter to "resolve" the fact that you were given a proxy didn't necessarily work out. Guice should rather instantiate after some logic; I guess building some kind of tree of the dependencies, instantiating the leaf nodes (those w/o dependencies) first, then going upwards. Any circle in the constructor graph should resolve correctly by having any of the classes in the circle implement a setter or a field injection taking the object(s) it depends on which is a part of the circle.

Is this how it was implemented?

The detection of when to use a proxy hasn't changed.  If Guice tried to do it before, it will still try to do it.  All that InjectorBuilder.disableCircularProxies does is tell Guice to throw an exception instead of creating the proxy.  However, a quick test shows Guice already is able to resolve circular setter dependencies without a proxy.  (The below would fail if it required proxies, since it's using classes and not interfaces.)
-----
  public void testSetterCircular() {
    Injector injector = Guice.createInjector(new AbstractModule() {
      @Override
      protected void configure() {
       bind(X.class);
       bind(Y.class);
      }
    });
    injector.getInstance(X.class);
  }
 
  public static class X {
    @Inject void setY(Y y) { System.out.println("constructing X(" + this + ") with: " + y); }
  }
 
  public static class Y {
    @Inject void setX(X x) { System.out.println("constructing Y(" + this + ") with: " + x); }
  }
-----
The above prints out:
constructing Y(com.google.inject.CircularDependencyTest$Y@1ce2dd4) with: com.google.inject.CircularDependencyTest$X@122cdb6
constructing X(com.google.inject.CircularDependencyTest$X@122cdb6) with: com.google.inject.CircularDependencyTest$Y@1ce2dd4

But, if I change X to:
    @Inject X(Y y) { System.out.println("constructing X(" + this + ") with: " + y); }
... which uses a constructor dependency in X (despite still having a setter dependency in Y) then things fail.

The necessary bits are in ConstructorInjector.construct, if you're curious to take a look. 

sam
 

[Dhanji R. Prasanna]

No, we have always scanned all the bindings first and resolved them, their order, before instantiating (I mean, you can't really inject until a dep is injected first anyway, so this ordering is natural). The only caveat is that it is not deterministic, I believe. So you cannot predict the order in which two disjoint singletons will be instantiated.

Dhanji.

2010/7/26 Endre Stølsvik <stol...@gmail.com>

[Dhanji R. Prasanna]

Although this may have changed coz I remember Jesse putting in a LinkedHashMap for the binding set at some point...

Dhanji.

[Endre Stølsvik]

On Tue, Jul 27, 2010 at 05:06, Dhanji R. Prasanna <dha...@gmail.com> wrote:

(I mean, you can't really inject until a dep is injected first anyway, so this ordering is natural).

I agree that this order is natural - but that is where I have understood that Guice didn't bother: It just injected down the road, supplying proxies wherever an object wasn't yet instantiated. When these objects became instantiated, all the proxies had it set, so that the calls went through. All very transparent, until some constructor (or even setter) tries to invoke something on the injected proxy before the underlying object existed - and you get that "This is a proxy!" exception..

Because of this graph ignorance, you could end up with lots of proxies in your graph, precisely because no such attempt at ordering them in the best way was done.

This understanding comes from reading mailing list throughout the years regarding the proxies. But of course, I might have misunderstood it all! However, if I have understood it anywhere correctly, then the disableCircularProxies method should make injection crash with this new exception much of the time in anything but dead simple graphs.

If Bob is around, he should be able to set this straight in a few sentences, because I believe it is from him I have this understanding, wrong or right..!

Kind regards,

Endre.

[Sam Berlin]

Yes, this is the way Guice creates objects.  It makes no attempt to derive dependencies and instead constructs objects as requested.  Any attempt to derive an order would have an impact on the speed of Injector creation.

sam

2010/7/27 Endre Stølsvik <stol...@gmail.com>

[Dhanji R. Prasanna]

The greater point about avoiding in-construction exceptions is not to use the constructor for init logic. You would have to wait for something like our lifecycle extension, which would solve that problem nicely.

Dhanji.

2010/7/28 Endre Stølsvik <stol...@gmail.com>

[Endre Stølsvik]

Well, it also happens for setters, since Guice just constructs, sets fields and run setters in "one go" - sticking proxies wherever needed, also into setters.

Up until the SPI-listener stuff, the way I have found to have any semblance of lifecycle, is to give each object some interface to register to, and then run the lifecycle on those registered. If that poor register-object became proxied (which you have NO control over), you were screwed - and just had to try different things until it worked.

Endre.

[Endre Stølsvik]

So the "don't proxy" method is basically worthless, then? It will pretty much always "crash" with "won't proxy" exception for anything but the basicest object graphs, and even sticking in a setter won't resolve it?

How much more time are we talking about here? When the Injector are made, you ONCE set up a tree/graph for how to construct each object, and that would take such a huge time? I can't seem to understand how that would influence Guice that radically to the negative.

Endre.

[Endre Stølsvik]

Also, you could do it so that such graphs were only deduced when the "don't proxy" flag is set. Then there would be no regression at all for the "as now" case, and the don't proxy idea would have meaning.

Endre.

2010/7/28 Endre Stølsvik <stol...@gmail.com>

[Stuart McCulloch]

2010/7/28 Endre Stølsvik <stol...@gmail.com>

So the "don't proxy" method is basically worthless, then?

well the method does exactly what it says it will do - proxies won't be used to satisfy circular dependencies, so if there's a circular dependency then of course it will fail - but at least you'll get early knowledge of this in development and can then fix it accordingly
 

It will pretty much always "crash" with "won't proxy" exception for anything but the basicest object graphs, and even sticking in a setter won't resolve it?

have you tried it in practice? if you find a situation where a graph could be satisfied without using a proxy and it still fails then I'd log an issue with a testcase - then at least we have something concrete to discuss

I'd also suggest creating a new thread specific for this discussion, as this is going a bit off-topic

How much more time are we talking about here? When the Injector are made, you ONCE set up a tree/graph for how to construct each object, and that would take such a huge time? I can't seem to understand how that would influence Guice that radically to the negative.

but imho that's something best left until after 3.0 is cut, otherwise the release will be even more delayed...
 

--
Cheers, Stuart

[Endre Stølsvik]

On Wed, Jul 28, 2010 at 11:48, Stuart McCulloch <mcc...@gmail.com> wrote:

2010/7/28 Endre Stølsvik <stol...@gmail.com>

So the "don't proxy" method is basically worthless, then?

well the method does exactly what it says it will do - proxies won't be used to satisfy circular dependencies, so if there's a circular dependency then of course it will fail - but at least you'll get early knowledge of this in development and can then fix it accordingly

The point is that adding a setter method somewhere in the circular dependency cycle should fix the problem, right? Because then you'd start with that object S, which now obviously should be possible to construct (since it doesn't anymore require an object in the constructor which is part of a cycle). Then you'd construct up the other objects, and finally you'd end up with the object which must be set on object S using your newly added setter. This construction order must furthermore be the result whenever any object belonging to that circular dependency graph is requested.

 

 

It will pretty much always "crash" with "won't proxy" exception for anything but the basicest object graphs, and even sticking in a setter won't resolve it?

have you tried it in practice? if you find a situation where a graph could be satisfied without using a proxy and it still fails then I'd log an issue with a testcase - then at least we have something concrete to discuss

I have not tried it yet, so this is purely from a theoretic standpoint: If the construction of objects still are made as it was, where Guice constructs objects as requested without any graph logic where setters are not treated as a possible way to break cycles, then this method won't achieve anything.

Furthermore, since the construction order is indeterminate, you might get "explosions" by doing something seemingly unrelated in some module, or in some class - that is, even if it still should be possible to construct all objects without using proxies (by using the setters (or fields, obviously)).

I will try to find time to test the current code.

 

How much more time are we talking about here? When the Injector are made, you ONCE set up a tree/graph for how to construct each object, and that would take such a huge time? I can't seem to understand how that would influence Guice that radically to the negative.

but imho that's something best left until after 3.0 is cut, otherwise the release will be even more delayed...

Well, I'm just sayin': If there is no attempt at doing "cycle resolution", then the new don't-proxy method is worthless.

(But since Dhanji says that there is resolution in place, then maybe it already works?)

Kind regards,

[Dhanji R. Prasanna]

2010/7/30 Endre Stølsvik <stol...@gmail.com>

On Wed, Jul 28, 2010 at 11:48, Stuart McCulloch <mcc...@gmail.com> wrote:

2010/7/28 Endre Stølsvik <stol...@gmail.com>

So the "don't proxy" method is basically worthless, then?

well the method does exactly what it says it will do - proxies won't be used to satisfy circular dependencies, so if there's a circular dependency then of course it will fail - but at least you'll get early knowledge of this in development and can then fix it accordingly

The point is that adding a setter method somewhere in the circular dependency cycle should fix the problem, right? Because then you'd start with that object S, which now obviously should be possible to construct (since it doesn't anymore require an object in the constructor which is part of a cycle). Then you'd construct up the other objects, and finally you'd end up with the object which must be set on object S using your newly added setter. This construction order must furthermore be the result whenever any object belonging to that circular dependency graph is requested.

 

 

It will pretty much always "crash" with "won't proxy" exception for anything but the basicest object graphs, and even sticking in a setter won't resolve it?

have you tried it in practice? if you find a situation where a graph could be satisfied without using a proxy and it still fails then I'd log an issue with a testcase - then at least we have something concrete to discuss

I have not tried it yet, so this is purely from a theoretic standpoint: If the construction of objects still are made as it was, where Guice constructs objects as requested without any graph logic where setters are not treated as a possible way to break cycles, then this method won't achieve anything.

Furthermore, since the construction order is indeterminate, you might get "explosions" by doing something seemingly unrelated in some module, or in some class - that is, even if it still should be possible to construct all objects without using proxies (by using the setters (or fields, obviously)).

I will try to find time to test the current code.

 

How much more time are we talking about here? When the Injector are made, you ONCE set up a tree/graph for how to construct each object, and that would take such a huge time? I can't seem to understand how that would influence Guice that radically to the negative.

but imho that's something best left until after 3.0 is cut, otherwise the release will be even more delayed...

Well, I'm just sayin': If there is no attempt at doing "cycle resolution", then the new don't-proxy method is worthless.

(But since Dhanji says that there is resolution in place, then maybe it already works?)

I did not say that, perhaps it was not clear--what I meant was we resolve that the bindings can be satisfied before attempting to inject them. In other words, all configure() methods are called and missing bindings are identified, prior to instantiation.

Dhanji.