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.
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. >> > > -- 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.
