Looking at the testcase, I'm not sure how listeners on provision help your case, unless you'd re-architect the whole thing in general.
FWIW, if the circular detection on providers is reverted, you can see the testcase ends up calling 'provideA' twice, despite the fact it's a singleton and should have been scoped. The Scopes.SINGLETON check wasn't catching it because it happens to return the exact same instance (and it used instance equality to find the re-entrant providers). Still, I'd say it's a bug regardless of the return value (there was just no way to notice it before), because the user tagged the provider as @Singleton, so clearly isn't expecting to provision more than one time. How do you think you'd write the code if a ProvisionListener existed? sam On Wed, May 4, 2011 at 2:06 PM, Stuart McCulloch <[email protected]> wrote: > > On 4 May 2011, at 15:51, Sam Berlin wrote: > > Would something like Provision Listeners (that were notified before & after > location/injection of all injected objects) help? > > > Yes, some sort of ProvisionListener would actually make our adapter code > much simpler - we really just need something that lets us know _when_ the > provisioning step completes for injected objects so we can fire off some > additional events (but nothing that would change what's being injected or > returned). At the moment we're using InjectionListeners with a guard against > nested injection to try and detect these events, which of course isn't > enough with the change for circular providers. > > Issue http://code.google.com/p/google-guice/issues/detail?id=78 is a > long-standing one.. although I'm hesitant to make full-on "provision > interceptors" because that could make it too easy for people to silently > change what objects are injected or what is returned. I think some form of > provision listener may work though, something like: > > ProvisionListener { > void beforeProvision(Key<T> key); > void afterProvision(Key<T> key, T provided); > } > > or more like interceptors, but without the ability to modify the provision: > > ProvisionListener { > void onProvision(Provision<T> provision); > Provision<T> { > T provision(); > Key<T> getKey(); > } > } > > (Perhaps also providing a Set<InjectionPoint>.) > > In my head, these would apply to _all_ provisioned objects, which includes > Providers & normal bindings. The main use-case I've seen for this is folks > that want to get timing data around how long it takes to inject/provision > their objects, so the can optimize accordingly. But it could also apply to > your use-case of needing to know when provision is all finished with > Providers, and may be a simpler method than InjectionListeners. > > > Sounds good - we're more interested in the final event, but I can see > supplying both before and after events would be useful for profiling > > PS. I've attached that example I mentioned as a patch > to CircularDependencyTest > > http://code.google.com/p/google-guice/issues/detail?id=626#c3 > > it's a bit contrived, but it hopefully shows what the adapter does (ie. > basic bean injection) and why it was working without blowing the stack (ie. > no constructor injection) > > sam > > On Tue, May 3, 2011 at 7:11 PM, Sam Berlin <[email protected]> wrote: > >> >> >> On Tue, May 3, 2011 at 7:05 PM, Stuart McCulloch <[email protected]>wrote: >> >>> On 3 May 2011 23:30, Sam Berlin <[email protected]> wrote: >>> >>>> >>>> On Tue, May 3, 2011 at 6:14 PM, Stuart McCulloch <[email protected]>wrote: >>>> >>>>> Hi Sam, >>>>> >>>>> I think the change introduced in r1543 to fix Issue 626 breaks the >>>>> semantics of InjectionListeners in certain circumstances. >>>>> >>>>> Previously when the InjectionListener was called you could be certain >>>>> all @Inject annotated members had been injected and any circular proxies >>>>> were initialized. Unfortunately the change in r1543 means that the >>>>> instance >>>>> could now still contain uninitialized provider proxies at the time >>>>> the InjectionListener is called. This makes InjectionListeners more >>>>> fragile >>>>> and less useful, especially wrt basic lifecycle operations. >>>>> >>>>> I'm not quite sure how best to solve this - yes detecting cycles >>>>> between providers can be useful, but typically people put providers in to >>>>> try and break cyclic dependencies. So this change might even lead to >>>>> situations where (due to legacy code) there's a cycle that would have been >>>>> solved with providers but now can't be solved. But assuming we can't wind >>>>> back the clock (or make this check optional), is there any chance we could >>>>> add a new SPI listener that gets called at the end of "circularGet"? ie. >>>>> after the call to "finishConstruction" like the listeners >>>>> in ConstructorInjector? That way we'd still have a way to manage the >>>>> instance after all proxies are initialized, but before it gets passed back >>>>> to the caller. >>>>> >>>> >>>> The change should be strictly improving upon situations that would have >>>> previously failed. There were two failure scenarios before: >>>> >>>> 1) Two providers were circular dependent directly on each other. This >>>> scenario would have resulted in a StackOverflowError as each provided is >>>> recursively called to provide for the other. >>>> >>>> 2) A provider is involved in a circular dependency that involves a >>>> non-provider binding. If the Provider was a singleton, this would have >>>> resulted in Scopes.SINGLETON failing with a "re-entrant provider" failure. >>>> If it wasn't a singleton, I'm not 100% sure what would have happened (but >>>> it was likely broken). >>>> >>>> Would you be able to write a failing test case that uses >>>> InjectionListeners? The circular provider checks are intended to fix >>>> actual >>>> problems that we ran into at Google, and should result in all around safer >>>> code. I'm not sure what stands out with the circular dependency detection >>>> on Providers that wouldn't have already caused problems with Constructors, >>>> since the two have basically the same detection/handling model. >>>> >>> >>> Sure, I can write up a failing test - the degenerate case is quite easy >>> to recreate with a Provider cycle and basic InjectionListener, albeit not >>> ideal code. Note the difference between the Constructor and Provider case is >>> that the Constructor fires off its listeners after the proxies are >>> initialized (ie. their delegates set) whereas with the Provider case there >>> are no listeners to fire (and therefore no way to perform some last minute >>> management). >>> >>> I might experiment with adding the new SPI listener in sisu-guice. (For >>> now I've rolled this change back over there as it breaks our Plexus adapter >>> - which I admit does push Guice to the edge wrt. lifecycle management >>> because it has to support legacy code and semantics involving some very >>> tortuous cycles!) >>> >> >> Sounds good, thanks. I'd be curious to look at the test and see the >> Provider cycle, and how you made it work without getting StackOverflowErrors >> or randomly generated duplicate provided copies. >> >> sam >> >> >>> >>> sam >>>> >>> >>> -- >>> Cheers, Stuart >>> >>> -- >>> You received this message because you are subscribed to the Google Groups >>> "google-guice" group. >>> To post to this group, send email to [email protected]. >>> To unsubscribe from this group, send email to >>> [email protected]. >>> For more options, visit this group at >>> http://groups.google.com/group/google-guice?hl=en. >>> >> >> > > -- > You received this message because you are subscribed to the Google Groups > "google-guice" group. > To post to this group, send email to [email protected]. > To unsubscribe from this group, send email to > [email protected]. > For more options, visit this group at > http://groups.google.com/group/google-guice?hl=en. > > > -- > You received this message because you are subscribed to the Google Groups > "google-guice" group. > To post to this group, send email to [email protected]. > To unsubscribe from this group, send email to > [email protected]. > For more options, visit this group at > http://groups.google.com/group/google-guice?hl=en. > -- You received this message because you are subscribed to the Google Groups "google-guice" group. To post to this group, send email to [email protected]. To unsubscribe from this group, send email to [email protected]. For more options, visit this group at http://groups.google.com/group/google-guice?hl=en.
