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.
