adding support for closing singleton objects within an Injector (or REQUEST/SESSION scope) for issue 62
James Strachan
http://code.google.com/p/google-guice/issues/detail?id=78
I've taken a stab at adding support for @PreDestroy and Spring's
DisposableBean lifecycles; building on top of my previous patch.
So far the close hooks only work with Singleton scoped objects; but
I've also added the functionality into the REQUEST and SESSION scoped
Scopes too - though I've not yet added any code to the Servlet package
to invoke the close() methods yet.
I've introduced a Closeable interface which a Provider can implement
(and a Scope can return a custom Provider which implements this); so
if a Scope implements Closeable you can then close it down cleanly;
ensuring that all objects are closed and each exception thrown is
collected (rather like we do with the Errors class when binding
objects).
Also I've added a close() method to Injector so you can close down any
singleton objects.
e.g.
Injector injector = Guice.createInjector(...);
// do stuff
// now lets cleanly close down the singletons in this injector
injector.close();
There are many possible strategies we can use to actually close down
an object held within a Singleton/REQUEST/SESSION scope . e.g.
respecting java.io.Closeable; or using @PreDestroy on a method or
Spring's DisposableBean or some developer-defined lifecycle
API/annotation mechanism etc.
So I've introduced a strategy interface, Closer which is a
strategy/adapter to close objects down. So you can bind as many Closer
implementations into your binder and this patch will invoke them all
on each object in the scope being closed. e.g. so you could install
the JavaIO, JSR250 and Spring closers to respect those lifecycles. If
you look at some of the Closer implementations in this patch it will
become a bit more clear.
Note this patch has no effect to objects created outside of a
singleton (and is currently not invoked for REQUEST/SESSION scopes).
The code InjectorImpl.close() and REQUEST/SESSION scopes is fairly
straightforward. The more complex change was dealing with the
Singleton scope; as the singleton SINGLETON object creates Providers
for each key which internally stores the singleton object. So to be
able to close all the singletons on an injector you need to iterate
through all the Providers in the bindings looking for providers which
are Closeable. Since there's a lot of wrapping of Providers along with
InternalFactory; I made a number of internal wrappers and adapters
also implement Closeable. This should have no runtime effect when
folks don't invoke the Injector.close() method.
As an aside - while implementing this I found myself adding a Set<T>
getInstances(Class<T> baseClass) method to InjectorImpl which seems
quite handy (I know its not hard to code, but its a useful helper
method IMHO). I wonder if we should introduce a helper class,
Injectors where we can add some helper methods like this so folks can
easily look up objects using class/Matcher filters etc?
If you are interested the patch is here...
http://code.google.com/p/google-guice/issues/detail?id=62
There are test cases showing the JSR 250 @PostConstruct and
@PreDestroy working along with Spring tests showing
InitializingBean/DisposableBean working.
Any thoughts/feedback most welcome
--
James
-------
http://macstrac.blogspot.com/
Open Source Integration
http://open.iona.com
Robbie Vanbrabant
Would it be possible to create like a Scope decorator that adds lifecycle support to a scope?
Scope lifecycleSingleton = Lifecycles.lifecycleScope(Scopes.SINGLETON);
bind(My.class).in(lifecycleSingleton);
And then instead of injector.close, maybe have something like Lifecycles.destroy(lifecycleSingleton);
Obviously you could provide a class that has the decorated scopes and holds some utility methods:
class LifecycleScopes {
final Scope LIFECYCLE_SINGLETON = Lifecycles.lifecycleScope(Scopes.SINGLETON);
...
static final List<Scope> LIFECYCLE_SCOPES;
static {
// add all lifecycle scopes to LIFECYCLE_SCOPES
}
void destroy() {
// for scope in LIFECYCLE_SCOPES:
// scope.destroy();
}
}
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.
Robbie
James Strachan
> 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.
The other benefit of the patch is we don't introduce another
collection to maintain the singleton objects (as the Injector already
keeps track of the Providers).
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.
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.
Note that the changes in the SPI package are nothing to do with the
two possible Injector v Scopes implementation approaches; they're to
do with actually closing down the objects within-the-scopes and
collecting all the errors into a single exception (like Guice does for
binding exceptions) (i.e. they'd be used whether your or my approach
were used).
Robbie Vanbrabant
2008/10/6 Robbie Vanbrabant <robbie.v...@gmail.com>:
> I love your efforts, but I wonder if we can have something that has lessI tried to go down this route; but its a tad messy as we'd have to
> impact on core Guice.
> Would it be possible to create like a Scope decorator that adds lifecycle
> support to a scope?
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.
The other benefit of the patch is we don't introduce another
collection to maintain the singleton objects (as the Injector already
keeps track of the Providers).
I don't think that's a real disadvantage. I mean, it's not that we're wasting memory. :-)
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.
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,The patch doesn't change the public API of Guice at all - other than a
> really. But I do think it would be nice if we can keep it as an opt-in
> extension, keeping the core API clean.
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. 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. Anyway, it's not me who you should convince, it's Bob/Jesse. :-)
Cheers
Robbie
James Strachan
> On Mon, Oct 6, 2008 at 5:11 PM, James Strachan <james.s...@gmail.com>
> wrote:
>>
>> 2008/10/6 Robbie Vanbrabant <robbie.v...@gmail.com>:
>> > 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
Tim Peierls
needed simple lifecycle management. There's a mini-framework for
defining context-specific scopes that can be closed with an explicit
method call, with pluggable type-specific handlers that are called for
every object of matching type associated with a given context.
("Context" is the parameterizable part of the framework; for example,
in a request scope, the context is the individual request.)
This mini-framework is really independent of DWR, but in the earliest
release the relevant code was mixed up in the same package as the DWR-
specific stuff, as you can see via this out-of-date link:
http://dev.priorartisans.com/tim/dwr-guice/
The main interface here is ContextScope:
http://dev.priorartisans.com/tim/dwr-guice/xref/org/directwebremoting/guice/ContextScope.html
In practice, I use the java.io.Closeable interface with a Closeable-
specific handler, so that all Closeable objects in the context have
their close() method called when the containing context is closed.
Even when a closeable class doesn't have any relation to I/O, it's
just feels easier and nicer to implement an existing standard
interface.
More recently, I've factored out the non-DWR Guice utilities into a
separate package, but I haven't had a chance to bundle it nicely.
Several people have said that the lack of detailed documentation and
examples for this mini-framework is a showstopper for them.
--tim
On Oct 6, 9:48 am, "James Strachan" <james.strac...@gmail.com> wrote:
> Since adding a patch for issue 78http://code.google.com/p/google-guice/issues/detail?id=78
Robbie Vanbrabant
Well right now noone calls close() and things are OK. Not callingclose() 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.
Gili Tzabari
Alternatively, how about having the ability to return all Singletons
associated with an injector, then iterating through them and closing
them as appropriate. Meaning, the user would define some closable
interface and cast his Singletons to that type before invoking close().
Just a thought.
Gili
James Strachan
> Alternatively, how about having the ability to return all Singletons
> associated with an injector, then iterating through them and closing
> them as appropriate. Meaning, the user would define some closable
> interface and cast his Singletons to that type before invoking close().
> Just a thought.
FWIW my patch kinda does this - albeit under the covers - each
singleton is passed to the Closer strategy.
Whether we go with a 'iterate through singletons' API or close() to
'invoke Closer strategies on singletons' approach, we'll need some
kinda way to decorate the tree of Provider objects so from the
InjectorImpl you can navigate into the providers to find the
singletons. Unless we replace the Scopes.SINGLETON implementation with
a different implementation which maintain a Set of singletons created
(though the former is probably more efficient).
Robbie Vanbrabant
James Strachan
> One Josh Bloch quote to rule them all: "Don't make the user do work that the
> library could do".
> So close() is fine. Its location is not ;-)
> If you go for the MulticastInjector, you could probably even disable Closer
> tracking completely when using regular Injectors.
There is no Closer tracking until you actually invoke the close()
method btw :). So it really has zero overhead if folks don't use it -
other than a new method, close()