pombreda / google-guice

Automatically exported from code.google.com/p/google-guice
Apache License 2.0
0 stars 1 forks source link

Lifecycle support #62

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago
Support for JSR-250 (javax.annotation) would also be very useful for all those 
people using init() 
methods and the list.

The mapping I think is:

@PostConstruct -> Called right after construction
@Resource -> Analogous to @Inject
@PreDestroy -> No concept of destruction, is there?
@Generated -> Doesn't apply

Original issue reported on code.google.com by h...@formicary.net on 14 Mar 2007 at 1:22

GoogleCodeExporter commented 9 years ago
Hooray! 

Whatever Service interface you decide to go with, please make sure there's some 
kind of ServiceStrategy so we can plug in adapters to it - as there's a zillion 
different mechanisms out there for basically calling a 'start' and a 'stop' on 
a bean (JSR 250, Spring's IniitializingBean / DisposableBean then most 
frameworks add their own API with some kind of start/stop method - the JDK has 
Closable etc).

Being able to stop a scope would rock too. 

Original comment by james.st...@gmail.com on 23 Jul 2010 at 7:59

GoogleCodeExporter commented 9 years ago
Some thoughts, based on previous lifecycle stuff I've written to cooperate with 
Guice...

 * both startAndWait & startAsync (similarly both stopAsync & stopAndWait) is unnecessary.  xAndWait can be done by calling get() on the returned future of the async method.
 * I've found an 'initialize' state to be useful -- that is, a state between & State.RUNNING.  (likely two states: State.INITIALIZING & State.INITIALIZED), with a method Future<State> initialize().  This lets a lot of services that are interdependent make sure that they each have a proper setup before starting.  (The service manager would call initialize on everything before starting anything.)
 * While there should be a granularity on services per service (or key), there should also be a way for a single listener to be notified of all service state changes.
 * There should be an easy way to schedule periodic Runnables/Callables (similarly to ScheduledExecutorService), to ensure that they start when the service starts & stop when the service stops.  Something easier than delegating to a SecheduledExecutorService, capturing the returned ScheduledFuture in start & stopping it in stop.. because that usually either goes wrong (forgetting null checking), or is ignored (no stopping).
 * There should be some concept of service order.  Doesn't need to be exact, but I've found buckets (early, middle, late) to be very helpful.

Original comment by sberlin on 23 Jul 2010 at 12:36

GoogleCodeExporter commented 9 years ago
There's no need for the manager to call everything before anything is returned 
- we have a dependency graph, we can just call startup along the dependency 
chains. 

Anyway, Jesse Wilson and I had a similar conversation about five weeks ago (I 
think) and came to a similar conclusion about an interface.  I was working up a 
proposal, but hadn't seen this bug.  I DO think it's important that we not go 
with the "start everything" approach.  Too much up-front hit, I think.  Wiring 
should happen up-front, but one point of separating out wiring from config from 
startup is to keep the costs of these pieces of work separate (and 
maintainable).  

The other thing that needs to happen with any lifecycle system is a strong set 
of recommendations on where to do what kinds of work, so people have a clue.  
So much work is done in providers and constructors to-date, that having good 
instructions could ease migration to using the lifecycle in a maintainable and 
sane way.

Original comment by christia...@gmail.com on 23 Jul 2010 at 6:33

GoogleCodeExporter commented 9 years ago
And yes, I'm volunteering. :)

Original comment by christia...@gmail.com on 23 Jul 2010 at 6:34

GoogleCodeExporter commented 9 years ago
We have a dependency graph for use/construction dependencies, but not for 
service requirements.  It's fairly often that a service (Foo) needs other 
services (Bar, Baz) setup (either initialized or up & running) before Foo can 
start, despite Foo not having a Guice-visible dependency on Bar/Baz.

Original comment by sberlin on 23 Jul 2010 at 6:48

GoogleCodeExporter commented 9 years ago
Why would Foo not have guice visible dependency on Bar/Baz, if Guice is 
actually creating the wiring graph... I don't mean it has to have a direct 
visible dep on Bar or Baz, but it should have a sort of transitive dependency 
on those that Guice, having wired up the graph first, knows about.

Picocontainer does it this way and it works nicely.  Components which need 
lifecycle implement the start() semantics, and the container starts them in 
dependency order as-needed.  What am I missing about how Guice handles things?  
Or is the lifecycle proposal external to the Injector entirely?

Original comment by christia...@gmail.com on 23 Jul 2010 at 6:55

GoogleCodeExporter commented 9 years ago
We discussed this at length, and were pretty convinced that best solution is 
for the user to manage the dependency order with the help of a CompositeService:

Service.start() // called from main()

CompositeService -> calls Service1.start(); Service2.start(); etc.

Original comment by dha...@gmail.com on 23 Jul 2010 at 7:38

GoogleCodeExporter commented 9 years ago
@dhanji -- So long as multiple .start() calls are ignored (the service isn't 
started twice), and the returned Future from latter calls still works for 
blocking.. that seems like a good plan.  I'm a little scared about having an 
insanely large CompositeService -- kind of like an additional Module, except 
for service ordering instead of object wiring.

@cwg -- It's often the case that a service has no need for a real "wiring" 
dependency on something else, but it still has a service dependency on it.  It 
would be ashame, I think, to force an object dependency on it.  This has been 
what's nice about Guice not including lifecycle as part of a base feature -- it 
separates lifecycle dependencies from object dependencies.

Original comment by sberlin on 23 Jul 2010 at 8:10

GoogleCodeExporter commented 9 years ago
I have an interest in the Service approach being talked about here, but to me 
that seems to be a separate issue to how this started out.  Recently I was 
trying to implement a service architecture on Guice, but ran into various 
issues that made a quick solution impractical. A major one being the difficulty 
in finding which bindings are really services, when the interfaces they are 
bound to do not indicate this. That is, finding implementations that are 
singletons with the required "Service" interface, even when the bean itself 
might be bound to a provider method, returning only a public interface for the 
service it offers.

The easiest way around this was (unfortunately) to switch to Spring, which has 
two separate lifecycle concepts, 1) injection lifecycle (init, destroy, etc,) 
2) context lifecycle (start, stop). These two are well defined, but also pretty 
simplistic, in that you get only one context lifecycle, that will always start 
every bean with the right interface, as long is it is defined at the top level 
in the config.

What I think would be more interesting to experiment with for this use case is 
a general purpose service framework, built on top of Guice. This layer would be 
able to offer features such as promotion of some interfaces from an injector as 
services, while hiding all implemention; more complicated state management 
(initialized, starting, started, going offline...); or even automatic 
proxying/remoting.  It might be possible to do something like:

configure() {
  bind(Someimpl.class)
  bind(InternalThing.class).to(another.class)
  bindAsService(MyService.class).to(myserviceimpl.class)
}

And then have MyService available to inject into other modules, while the 
InternalThing binding would only be visible to those locally defined bindings.

However, I don't really see this as a responsibility of Guice - Guice just 
proviides some mechanisms that could really simplify the work.

Original comment by undecons...@gmail.com on 23 Jul 2010 at 8:20

GoogleCodeExporter commented 9 years ago
<sigh>

I was afraid this was where Guice was going to go...

I wholeheartedly disagree that composite-service wtih a separate listing of 
dependency is the way to go.  For me, a collaborator dependency graph IS the 
order in which B needs A, and usage-order/start-order implies construction 
order (as in, if B needs A to be started before it needs to be started, it 
surely needs A to exist before it needs to exist.  Even if it doesnt' know 
about A, because it's transitive.

So I don't see it as an artificial dependency, but an absolutely natural one, 
implicit in the creation graph.

But even if it was artificial, with a CompositeService (which isn't really a 
"feature" of guice, as I can do this with a Multibinder today), you're 
basically specifying a separate, duplicate wiring which (as I said) overloads 
(to an extent) the creation graph.  That's maintaining metadata twice, which 
(again, to me) is really hard to maintain.

I've long considered the lack of lifecycle a bug, not a feature, and a 
radically bad design choice, and what I've seen in terms of people adding work 
to constructors/providers as a "workaround" the missing feature (to me) 
justifies this view.  

I'll dig back through the bug and conversations and see if I can't convince 
myself otherwise, but I"m afraid I wasn't convinced when I first read the 
arguments about "we're an injector not a container" and I suspect I won't find 
much new to convince me.

And I'm sorry if I'm a little snippy in this bug thread, but I had built up 
hopes and I'm actually really disappointed in this decision, because I really 
really really want to like Guice, and it's clean in so many other ways.

Original comment by christia...@gmail.com on 23 Jul 2010 at 8:24

GoogleCodeExporter commented 9 years ago
My problem with the Service approach is just the same as it is with every other 
proposed solution. The attitude is 'we'll come up with something complex to 
address every single case, then it's up to the community to do the 
simplification that 95% of apps need'.

Sure, the Service interface would do the job. Except I now have to implement an 
interface, and I suddenly have to know about concurrency and futures and states.

If I'm the kind of person who knows al that, chances are I'm also clever enough 
to do all that in my @PostConstruct method.

I accept the religious argument against any external dependencies (or rather, 
am resigned to it). So how about guice specific versions of the standard 
annotations? Hell, give them the exact same name and just put them in a guice 
package. That way the totally awesome google code wouldn't have to be polluted 
with javax filth.

Also sorry for the snippy tone, probably cos I'm angry at myself for forgetting 
to celebrate the 3 year anniversary of this issue!

Original comment by h...@formicary.net on 23 Jul 2010 at 9:14

GoogleCodeExporter commented 9 years ago
@60 What do you mean by a implies b ordering, in practical terms? Are you 
saying you want any class in the injector that impl's Startable to have start() 
called on it? Trying to understand... certainly don't want you to stop liking 
Guice!

@hani We can support the javax.* annotations as an optional module using 
Service. The Service scheme is mainly meant to nail down the conceptual 
programming model. There's no problem with supporting different annotations.

Original comment by dha...@gmail.com on 23 Jul 2010 at 9:53

GoogleCodeExporter commented 9 years ago
Ok, I have a possible proposal alternative, whipped up by some fellow googlers 
and I this afternoon, incorporating some of this discussion's pros and cons, 
and some thoughts from my convo with Jesse.  I'm going to try to put it 
together into something coherent, with at least sample usage code over the next 
few days.

Side notes:
@62 - Yes, before it's supplied as a dependency, if it implements lifecycle 
(either the service interface (or annotation or however it's indicated)

@61 - 3 years... heh.  Too funny.

Original comment by christia...@gmail.com on 23 Jul 2010 at 10:51

GoogleCodeExporter commented 9 years ago
@61: the goal of Guava's service interface is to do the concurrency for you. 
Clients who want to start services just call start/startAndWait(). Those 
methods are idempotent and allow you to shutdown a service asynchronously. 
People defining their own service implement AbstractExecutionThreadService, 
where they implement startup, run loop, and shutdown methods.
http://guava-libraries.googlecode.com/svn/trunk/javadoc/com/google/common/util/c
oncurrent/AbstractExecutionThreadService.html

Original comment by limpbizkit on 24 Jul 2010 at 7:51

GoogleCodeExporter commented 9 years ago
@61 FWIW you can use @PostConstruct with Guice if you add the 
http://code.google.com/p/guiceyfruit/ extension. This uses the same SPI that I 
used to re-implement Plexus as an extension on top of Guice.

On a side-note, to support Plexus lifecycles I took the simple approach and 
used an InjectionListener to track instances that implemented the various 
Plexus lifecycle interfaces - for each instance I ran through the appropriate 
startup sequence. This worked because for all cases (so far) the startup 
sequence matched the injection sequence. When the container stopped I just did 
the calls in reverse.

So basic lifecycle support is already possible using the SPI in trunk - I also 
think it's better for this to be an extension rather than baked into core, as 
there will always be someone who wants a different model.

Original comment by mccu...@gmail.com on 27 Jul 2010 at 8:59

GoogleCodeExporter commented 9 years ago
@mcculls yes - the problem isn't the 'start' part of lifecycle, its easy to do 
with the existing hooks. The big problem is trying to do a 'stop/close' or 
@PreDestroy in a clean way without hacks like forking guice (which guiceyfruit 
had to do) or writing your own custom Scope objects etc. (FWIW your idea only 
works if the stop hooks map exactly to the start hooks, which isn't the case in 
JSR 250).

We should be able to create an Injector, look up some objects then close down 
all the singletons for example without jumping through too many hoops. (Ditto 
Request / Session scope in guice-servlet).

Guice knows which objects have been created in a particular scope - they are 
stored in that scope (thats what a scope is afterall), so its kinda trivial to 
be able to iterate through a scope so you can close the objects - it just 
requires a fairly trivial change to guice to be able to do such a thing... 
http://code.google.com/p/google-guice/issues/detail?id=354

Right now there are only a small subset of real 'stop' lifecycles you can do 
without hacking your own replacement Scopes, forking guice or solving a small 
subset through hacks (basically creating your own mirror scope keeping track of 
all the objects you post process in a start interceptor). 

Original comment by james.st...@gmail.com on 27 Jul 2010 at 9:26

GoogleCodeExporter commented 9 years ago
How about implementing this feature incrementally? Does anyone disagree with 
the fact that the built-in scopes (I'm thinking REQUEST/SESSION scopes here) 
should clean up all Closeables at the end of each request/session? That seems 
pretty non-controversial to me. The beautiful part is that invoking close() 
multiple times is guaranteed to have no-effect so this feature won't conflict 
with @PreDestroy.

Original comment by cowwoc...@gmail.com on 26 Jan 2011 at 5:55

GoogleCodeExporter commented 9 years ago
I don't see why any particular Scope should be special and be able to do proper 
lifecycle starting & stopping of resources while others are not. Whether a 
Scope is application/singleton or page/request/session or some other kind of 
scope (conversation or whatnot) - its a pretty basic requirement to be able to 
cleanly start and stop resources without developers having to patch/hack/fork a 
DI container or jump through silly hoops. 

Pico & Spring have supported this for like 5 years; they make it easy for a 
developer. Its pretty sad there's still a discussion on this 4 year old issue 
which has been fixed in pretty much all DI engines ever - apart from Guice :(. 
I still can't fathom the push back. Maybe Guice committers just really get off 
on saying no? :)

Original comment by james.st...@gmail.com on 26 Jan 2011 at 8:51

GoogleCodeExporter commented 9 years ago
Telling the truth, I don't understand why guice have no good lifecycle support 
yet...
Mycilla guice is good, but it is not so good as it should be.
Errors on container start are not properly handled. i.e. some instances are 
already started when error occurs. in that case all services should be stopped 
which were already started...

Original comment by kvas...@gmail.com on 5 Nov 2011 at 12:48

GoogleCodeExporter commented 9 years ago
You can try Sitebricks's AwareModule as a lightweight lifecycle support for 
Guice.

https://github.com/dhanji/sitebricks/blob/master/sitebricks-web/src/main/java/in
fo/sitebricks/persist/PersistAware.java

And the module looks like:
https://github.com/dhanji/sitebricks/blob/master/sitebricks-web/src/main/java/in
fo/sitebricks/persist/StoreModule.java

Original comment by dha...@gmail.com on 27 Nov 2011 at 10:58

GoogleCodeExporter commented 9 years ago
And what about injector creation errors ? Some services may be already started 
in case of partially created injector and we defenitely need to stop them...

Original comment by kvas...@gmail.com on 27 Nov 2011 at 11:02

GoogleCodeExporter commented 9 years ago
Just in case somone interested.
mycilla-guice works ok for @PostConstruct and @PreDestroy for me, but I had one 
problem: guice+mycilla does not work ok when guice fails to create injector (in 
case some of services failed to start). Theese patches add:
* do not allow to recreate singleton objects after error
* perform destroy upon injector create error

Original comment by kvas...@gmail.com on 24 Dec 2011 at 7:59

Attachments:

GoogleCodeExporter commented 9 years ago
This issue is still open? :-)

I'd recommend against adding a sophisticated service lifecycle management 
framework to Guice. This could be a framework in and of itself. Of course, such 
a framework could integrate with Guice.

5 years ago, I didn't want Guice to depend on @PostConstruct and @PreDestroy 
because this would have created a dependency from Guice on Java EE. This would 
have been unacceptable because core Guice needs to run on platforms like 
Android.

Today, these annotations are part of core Java (since Java 6). Functionality 
that depends on these annotations could fail fast when the annotations aren't 
present (Java 5, Android, etc.). For example, Injector.shutDown() would throw a 
runtime exception.

Simple support for @PostConstruct and @PreDestory might make since.

It wouldn't make sense to support @PostConstruct alone. It exists because 
@Resource supports only fields and methods. Guice supports constructors and 
does not need something like @PostConstruct. However, if we support 
@PreDestroy, we should almost certainly support @PostConstruct, too, to avoid 
confusion.

Do we need @PreDestroy support? If so, how exactly should it behave? What level 
of flexibility do we need with regard to concurrency (asynchronous, parallel), 
exception handling, invocation ordering, etc.? Does it apply to only objects 
instantiated by Guice or all objects? What's the compatibility story?

To answer these questions, *we need concrete, real world use cases (examples) 
for @PreDestroy*. We need lots of examples, and in each case, @PreDestroy 
should be the best way of addressing that use case.

Finally, when it comes to the implementation, we need to take care not to 
introduce performance regressions, even if that means making this an "opt-in" 
feature. Performance is already bad enough on Android.

Original comment by crazybob...@gmail.com on 14 Mar 2012 at 5:06

GoogleCodeExporter commented 9 years ago
Hey Bob,

Glad to see that you're still alive :)

I think that this feature needs to be split into two:

1. A built-in @PreDestroy behavior that invokes close() on all instances of 
Closeable.
2. Support for @PostConstruct, @PreDestroy and other annotations.

Everyone seems to be on-board for #1. There seems to be some controversy 
surrounding #2. Couldn't we resolve them separately?

Original comment by cow...@bbs.darktech.org on 14 Mar 2012 at 5:26

GoogleCodeExporter commented 9 years ago
I wouldn't use Closeable (#1) here. It would be surprising to me if Guice 
called close() on my objects just because they happen to implement Closeable. 
Also, "close" doesn't fit all of the use cases.

If anything, we'd support @PreDestroy, but again, what are the use cases? A 
looked through the first 20 or so pages of results on koders.com and didn't see 
one interesting usage. I can try to imagine some:

  1. Closing I/O resources
  2. Shutting down threads and executors
  3. Proactively freeing native memory
  4. Removing external references (listeners, etc.)
  ...

@PreDestroy doesn't seem like a good solution to any of these. Use try-finally, 
try-with-resources, references, explicit shut down logic, etc., instead, and 
your code will be less error prone and more maintainable. No offense intended 
to Hani and others than were on JSR-250, but my sense is that @PreDestroy is an 
annotation in search of a problem.

Original comment by crazybob...@gmail.com on 14 Mar 2012 at 5:42

GoogleCodeExporter commented 9 years ago
The usecase I have is mostly in getting rid of Providers.

Basically, once everything is wired up (config) I want to then 'start stuff 
up', this includes making external connections based on bindings, and so on. 
It's a very good match for @PostConstruct.

@PreDestroy I haven't used. I tend to prefer explicit demarcation lines to 
denote scope ends, rather than have it done for me.

Original comment by h...@formicary.net on 14 Mar 2012 at 5:45

GoogleCodeExporter commented 9 years ago
Basic use case:

* I want my beans to start once all is configured. Some beans needs to creat 
thread to process data e.t.c. This can be done in constructors...
* Also I want my beans to shutdown properly. It means I want services to flush 
caches, to stop threads e.t.c. Usually it is not easy to stop properly...

Original comment by kvas...@gmail.com on 14 Mar 2012 at 6:28

GoogleCodeExporter commented 9 years ago
I very much believe that lifecycles like "starting" should be handled 
explicitly.  Classes should implement 'Service' and can use multibinder to bind 
into a Set<Service>, and then the Services can be started.  This doesn't feel 
like dependency injection to me.

Original comment by sberlin on 14 Mar 2012 at 6:45

GoogleCodeExporter commented 9 years ago
I can live without @PostConstruct, but without @PreDestroy I have to remember 
which instances to close, which is error prone. If there is a better way, Id 
love to hear about it...

public class Application {

  private Injector injector;

  public void onStart() {
    injector = Guice.createInjector(new AbstractModule() {
      @Override
      protected void configure() {
        bind(Foo.class).in(Singleton.class);
        ...
      }
    });
  }

  public void onStop() {
    injector.getInstance(Foo.class).close();
    ...
  }
}

Original comment by eric.j...@gmail.com on 14 Mar 2012 at 6:53

GoogleCodeExporter commented 9 years ago
Services should start in the proper order - all dependencies should start 
before. Same for stop. So it's really close to dependency injection.

Original comment by kvas...@gmail.com on 14 Mar 2012 at 6:56

GoogleCodeExporter commented 9 years ago
Telling the truth, I am really satisfied with mycilla guice. BUT guice 
defenitely should include bug fixes from #676.
1) Guice does not throw errors on null values in injected fields, but it should.
2) Singletons are recreated many many times even if error occurs.

Original comment by kvas...@gmail.com on 14 Mar 2012 at 9:28

GoogleCodeExporter commented 9 years ago
Yes, I never use guice 'base', always go with mycilla instead.

Original comment by h...@formicary.net on 14 Mar 2012 at 9:32

GoogleCodeExporter commented 9 years ago
Apparently I failed to post this to the group when I replied to this thread on 
the dev list. :(  Anyway, 

I agree that in all cases I've encountered in my own industry experience, the 
dependency-construction wiring order was also the correct order for service 
start-up, and there are efficiencies that can be pulled out in terms of 
starting up services in parallel where they're independent, etc.  However, in 
many cases it wasn't "necessary" it was "one correct ordering" of service 
startup.  

Besides,  in this case, what Guice needs to provide is additional API to walk 
the graph of objects along the dependency wiring, so an extension can do the 
work.  Service support doesn't need to be baked in to the core, and probably 
shouldn't, so those who aren't using such don't pay the performance penalty. 

Put simply - Guice manages the metadata of the dependency graph related to 
construction/injection order.  And it also provides the activities of actually 
constructing the objects and injecting them.  And it provides the intermediate 
steps of registering bindings, building a wiring order, and keeping that 
dependency graph straight.  I think we can solve this with extensions, for 
those around whom the startup order is important.

I'm going to repeat Bob's call for use-cases.  Can we have a comprehensive set 
of use-cases for service startup that needs more than Sam B.'s Set<Service> or 
Map<StartOrdinal, Service> (inet.d style startup)?  My historical examples are 
all from code I can't admit to existing, and in the end we opted for 
alternatives that did have service startup built-in.  But if we're going to put 
engineering resources into it, either showing how you would have written a 
piece of real code that actually needs wiring order, and how you worked around, 
and why that workaround sucks - that would be helpful.  Also helpful would be 
for forks/extensions to Guice that have service facilities, to help us 
understand what they needed from within Guice to implement, and we can talk 
about how to create an API to support that. 

Original comment by christia...@gmail.com on 14 Mar 2012 at 9:45

GoogleCodeExporter commented 9 years ago
Ain't this more about proper shutdown than startup?

Original comment by franci...@gmail.com on 14 Mar 2012 at 9:49

GoogleCodeExporter commented 9 years ago
Startup can be handled via constructors.
This is really more about shutdown.

Original comment by kvas...@gmail.com on 14 Mar 2012 at 10:02

GoogleCodeExporter commented 9 years ago
Ok - sorry - shutdown was the key part of this, but startup handled by 
constructors?  That's really not ideal.  I don't think I'm comfortable with 
THAT being the recommendation on how to do initialization of services with 
Guice.

Original comment by christia...@gmail.com on 14 Mar 2012 at 10:16

GoogleCodeExporter commented 9 years ago
FYI, it's been really easy to make a lifecycle extension now the 
ProvisionListener API has been added:

   http://code.google.com/p/google-guice/source/browse/core/src/com/google/inject/spi/ProvisionListener.java

We use this to add Plexus lifecycle support (the classic 
Initializable/Startable/Disposable interfaces)

   https://github.com/sonatype/sisu/blob/master/sisu-inject/containers/guice-plexus/guice-plexus-lifecycles/src/main/java/org/sonatype/guice/plexus/lifecycles/PlexusLifecycleManager.java#L129

The great thing about ProvisionListener is that you can use it to track 
dependency chains, which helps you decide the starting order of components. 
Note: with Plexus you can't simply just start a component on the constructor 
call because of cycles (which exist in legacy code) and because components use 
a lot of field injection.

Original comment by mccu...@gmail.com on 14 Mar 2012 at 10:35

GoogleCodeExporter commented 9 years ago
[deleted comment]
GoogleCodeExporter commented 9 years ago
Please, take a look on:

http://code.google.com/p/google-guice/issues/detail?id=676

This bug is stop point when working with ProvisionListener for startup/shutdown.

Original comment by kvas...@gmail.com on 14 Mar 2012 at 10:39

GoogleCodeExporter commented 9 years ago
(FYI, ProvisionListener is new in Guice head & not in a released version.  But 
+1 to using it.  It's extremely useful for stuff like this. We also use it to 
trace loading times of objects, etc..)

Original comment by sberlin on 14 Mar 2012 at 10:40

GoogleCodeExporter commented 9 years ago
Re #676 our lifecycle support fails-fast so that hasn't been an issue for us, 
but I can see how it could affect others - can you perhaps add a testcase to 
demonstrate each scenario and verify the proposed fix?

Original comment by mccu...@gmail.com on 14 Mar 2012 at 10:50

GoogleCodeExporter commented 9 years ago
Bob,

I'm not suggesting that Closeable replace @PreDestroy. I'm saying: don't make 
us implement @PreDestroy for Closeable. Guice should provide this reasonable 
default behavior.

What's the harm in auto-closing Closeable instances once their enclosing scope 
is destroyed? There is no harm in doing so because the specification explicitly 
states "If the stream is already closed then invoking this method has no 
effect."

Common use-case: Closing database connections (if necessary) to prevent leaks.

Original comment by cow...@bbs.darktech.org on 14 Mar 2012 at 11:05

GoogleCodeExporter commented 9 years ago
> What's the harm in auto-closing Closeable instances once their enclosing 
scope is destroyed?

Maybe I don't want the object to be closed.

Original comment by crazybob...@gmail.com on 14 Mar 2012 at 11:11

GoogleCodeExporter commented 9 years ago
Bob,

Hehe. Okay then :) Please provide a reasonable use-case where an out-of-scope 
Closeable object shouldn't be closed.

Original comment by cow...@bbs.darktech.org on 14 Mar 2012 at 11:33

GoogleCodeExporter commented 9 years ago
Sorry, cowwoc. Guice is explicit and not surprising. Someone maintaining your 
code and trying to figure out if/how a resource gets closed would no doubt be 
surprised to find out that Guice automagically does it for them.

Original comment by crazybob...@gmail.com on 14 Mar 2012 at 11:42

GoogleCodeExporter commented 9 years ago
I think closing/destroying objects when they go out of scope is a red herring.

It sounds like people really want the ability to start and stop per injector 
services in the right order. While the lifecycle of the service is the same as 
the injector's, the object isn't necessarily in singleton scope–maybe the 
service isn't eligible for injection into other objects (see 
Binder.requestInjection()).

@PostConstruct and @PreDestroy seem like a bad fit here. First, they don't 
speak to starting and stopping services. Second, and less important than the 
naming and semantics, using annotations will be slower and more error prone 
than using a marker interface.

How do we decide which objects are eligible to be started? Strawman: any object 
that's created or injected during injector creation. This includes eager 
singletons, objects passed to requestInjection(), and any transitive 
dependencies. Note: This could get a little weird if you use DEVELOPMENT vs. 
PRODUCTION stages.

One could use an InjectionListener named ServiceManager to collect any object 
that implements Guava's Service interface. After injector creation, you call 
ServiceManager.start(). ServiceManager stops collecting objects (by ignoring 
any further callbacks), and then it iterates over the objects in the order in 
which they were created–so you can use dependencies to control the order 
services are started in–and it starts each service.

When you're ready to stop, call ServiceManager.stop(). It'll stop the services 
in the reverse order.

Once Guice supports concurrent singleton injection (by inspecting the 
dependency graphs), ServiceManager could keep track of which threads the 
callbacks come in on. Then it could make sure that objects instantiated in the 
same thread are also started in the same thread.

Original comment by crazybob...@gmail.com on 15 Mar 2012 at 12:11

GoogleCodeExporter commented 9 years ago
So checking for a @PostConstruct is a performance hit when you're already going 
through all methods and fields checking for @Inject? Why is it any slower?

The flow as I see it is, if guice creates an object, it is responsible, once 
the full object graphs has been constructed, for calling @PostConstruct. I 
don't see it getting weird with things like .injectMembers() and 
.requestInjection(), in those cases @PostConstruct would not apply because 
guice did not construct the object. Admittedly with stages it might get weird, 
but nothing that well specified behaviour cannot address. For a novice/beginner 
dev, there are plenty of non-intuitive and cryptic behaviours and features in 
Guice, but they're pretty well specified so it's not terrible.

Keep in mind also that all this *is* possible. I have all the extra behaviour 
I'd like in Mycilla Guice. There's just some fundamental issue I must be 
misunderstanding as to why it's so unthinkable for Guice to get those features.

The 'prove we need it argument' is not very compelling anymore, 100 comments 
and 5 years on from the initial request. I'd like to reverse the challenge, and 
ask for a non-core Guice developer to argue we absolutely should not have it!

Clearly all arguments in favor of this feature are not ones that have resonated 
with any Guice developers, so just close it and say it'll never happen.

Original comment by h...@formicary.net on 15 Mar 2012 at 12:38

GoogleCodeExporter commented 9 years ago
Iterating over methods and checking for an annotation is an order of complexity 
slower than "instanceof Service," especially if you didn't already need to 
reflect on those methods. This is just to say that it would be nice to not 
force this overhead on Android apps.

Why would we only start instances that Guice instantiated? Seems like an 
arbitrary limitation.

To be fair, I think you're asking for something different from what everyone 
else is asking for. Other people want to start/stop services, orthogonally to 
injector creation. That makes some sense to me, but needs further 
specification. For example, which objects exactly are eligible to be started?

You've said that you don't care about stopping things and you just want 
@PostConstruct. This does not make sense to me because @PostConstruct is barely 
different from a constructor. Also, if you just want @PostConstruct support, 
it's trivial to hook in with an InjectionListener already.

Original comment by crazybob...@gmail.com on 15 Mar 2012 at 12:56

GoogleCodeExporter commented 9 years ago
Quick question: Have Guice gotten intelligent about the order in which it 
instantiates objects?

Earlier, it just did one or the other random order, not caring about each 
instance dependencies, creating those annoying proxies when it got to any 
dependency it didn't already have available.

Then one got the option of specifying that proxies should not be used.

However, the order hadn't gotten any smarter, so one still could arbitrarily 
crash into the ordering-problem, only now it stops with exception, not creating 
the proxy.

This even though a simple reordering of the flow would have fixed it.

First create the leafs; the ones without dependencies. Then work your way up: 
Create instances whose dependencies at this point be fulfilled. If you at some 
point iterate through the entire list of "to be instantiated" w/o instantiating 
any, then you have a dependency-loop.

Loops are only evaluated by constructors. Fields and methods can always be 
injected afterwards.

.. which leads to my point #1 about stuff like @PostConstruct..

Original comment by endre.st...@gmail.com on 15 Mar 2012 at 2:46

GoogleCodeExporter commented 9 years ago
You need @PostConstruct because that would only be invoked after ALL 
dependencies have been injected - both the constructor *and* field/methods.

This because you might need some service of some injected instance to start up. 
And that instance was injected by means of field injection. No good with 
constructor startup then.

Also, according to JCIP IIRC, one shall not leak unfinished objects. In a 
constructor, you could e.g. register yourself with the 
OnIncomingUserEventService, and *then* your object instantiation crashes with a 
NPE for some other reason. Therefore, thou shall not ever do "startup stuff" in 
a constructor. Only construct yourself, do not e.g. attach yourself to other 
object graphs.

Original comment by endre.st...@gmail.com on 15 Mar 2012 at 2:52