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