2008/10/6 Robbie Vanbrabant <[EMAIL PROTECTED]>: > On Mon, Oct 6, 2008 at 5:11 PM, James Strachan <[EMAIL PROTECTED]> > wrote: >> >> 2008/10/6 Robbie Vanbrabant <[EMAIL PROTECTED]>: >> > I love your efforts, but I wonder if we can have something that has less >> > impact on core Guice. >> > Would it be possible to create like a Scope decorator that adds >> > lifecycle >> > support to a scope? >> >> I tried to go down this route; but its a tad messy as we'd have to >> stop using the standard Guice scopes. e.g. it doesn't really work for >> the Scopes.SINGLETON field which is a static final value - which is a >> real class loader level immutable singleton. So each Injector stores >> the providers returned from the singleton scope instance - not the >> singleton Scope; we don't create a singleton Scope object for each >> Injector. > > > Singleton scope gives out self-scoping providers, right? I'm not sure if I > see what you're saying here, I should look at your patch first. My rough > idea was to delegate to the real scope, and decorate the provider it returns > or someting.
Thats kinda what I do :) The singleton scope returns CloseableProviders which implement Provider<T> and Closeable >> FWIW the change to the REQUEST/SESSION scopes kinda follow your >> approach; though they reuse the existing collections in the scopes >> (HttpServletRequest / HttpSession) - so there's no overhead at all >> using them; the only new logic is fired if you actually do invoke the >> close() method. >> >> Or to say this another way - I tried my best to support this feature >> with the minimal overhead (when being used or not being used) and to >> minimise the change to the core of guice. > > And I really love the effort. Just trying to see how we could improve it > further. Sure no worries - I love feedback :) >> Whether we use this feature or not on the SINGLETON/REQUEST/SESSION >> scopes, we don't introduce any new collections or processing overhead >> - unless folks do actually attempt to close resources down. The only >> real affect on the core is more internal classes implement the >> Closeable interface really - while that might look big in terms of >> lines of code changes in a patch, its not actually that big a deal >> IMHO. >> >> >> > Perhaps I'm just being stupid, because I haven't looked at the patch >> > code, >> > really. But I do think it would be nice if we can keep it as an opt-in >> > extension, keeping the core API clean. >> >> The patch doesn't change the public API of Guice at all - other than a >> new Injector.close() method. I figured it'd be a more drastic change >> to either remove the Scopes.SINGLETON object (as we'd need to create a >> Singleton scope object for each Injector) which is part of the public >> API and break backwards compatibility - or recommend folks stop using >> the scopes in Guice. > > When you follow best practices and bind to the scope annotation instead of > the instance I don't think you'll have a problem with the existing scopes. Yeah - though it would leave a little ickyness if folks use the Scopes.SINGLETON directly things would go strange. > So you could bind @Singleton to the lifecycle singleton scope, and the rest > of your modules don't care. > > About the close(), It's stuff you need to know, right? It's not that people > often use break; and continue; in Java, but they still need to know about > it. So adding lifecycle support does impact everyone, even the people who > don't use it. People will get confused: "Why is this close() here and when > do I call it?" and "Do I need to put that in a finally block to make sure it > cleans up?" and so on. I get your point, but it'd be valuable to at least > consider making it an extension, instead of dumping it in core Guice without > consideration. Well right now noone calls close() and things are OK. Not calling close() is not the end of the world; it just provides a nice graceful shutdown above and beyond killing the JVM or finalisers which may or may not be invoked - so its mostly a kinda optimisation really. I don't really see whats so confusing about users being able to call Injector.close() if and when they want to. I mean folks can do that with streams in the JDK and folks don't find it confusing (and folks don't always close streams etc) The main use cases for calling Injector.close() is for when folks want to DI beans designed to work with Spring, JSR 250 or EJB3 which all have the concept of close; which users of those beans will be well aware of anyway - so I don't think its gonna be confusing for end users. > Anyway, it's not me who you should convince, it's Bob/Jesse. Agreed :) /me crosses fingers -- James ------- http://macstrac.blogspot.com/ Open Source Integration http://open.iona.com --~--~---------~--~----~------------~-------~--~----~ 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 -~----------~----~----~----~------~----~------~--~---
