On 8 May 2011 21:04, Sam Berlin <[email protected]> wrote: > FYI, I updated issue 78 (Support Provision Interceptor / Constructor > Interception) <http://code.google.com/p/google-guice/issues/detail?id=78>with > a proof-of-concept (but fully working) patch that should hopefully > address this. If it does, I'll clean it up & firm up the API. >
Hi Sam, I tried out the patch and it provides enough functionality to get everything working again, even when circular providers are involved - so definitely +1 from me. -- Cheers, Stuart > sam > > > On Wed, May 4, 2011 at 7:21 PM, Sam Berlin <[email protected]> wrote: > >> >> >> On Wed, May 4, 2011 at 7:14 PM, Stuart McCulloch <[email protected]>wrote: >> >>> On 4 May 2011 23:59, Sam Berlin <[email protected]> wrote: >>> >>>> 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. >>>> >>> >>> Well, I did say that testcase was contrived :) our actual usecase is a >>> bit more sophisticated and is well-tested (since it drives Maven3) >>> >>> 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? >>>> >>> >>> The main issue is knowing when injection is complete, so we can fire off >>> some lifecycle events - without it (and with the circular provider detection >>> enabled) we're in danger of touching the provider proxies before they've >>> been initialized. With the ProvisionListener we'd know the object is fully >>> injected and can be managed safely. >>> >>> We're using the InjectionListener event at the moment to detect end of >>> injection, but when provider proxies are involved that's not really the end >>> of provisioning because the proxy delegates held by the instance are only >>> set after all the injection listeners have run and the instance has been >>> passed back from the provider. Hence the need for some sort of Provision(ed) >>> event. >>> >> >> Got it, makes sense. I'll play around and see what forms. >> >> sam >> >> >>> >>> 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. >>>> >>> >>> >>> >>> -- >>> 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.
