No code generated unless there's at least one singleton

mikehearn commented 3 years ago

I have a piece of code where I do the typical thing and manually create a bunch of objects and wire them together. I'd like to try DI to automate this process.

However, this program is not a server, it's a command line driven tool. Therefore nothing in my app can be made a singleton. Everything that's built either needs or is derived from some manually constructed objects from user provided flags.

Given our previous discussions about scope I figured, no problem, I'll just use a Request scope for everything. However, it seems Avaje Inject doesn't trigger at all unless at least one @Singleton is found. If the only objects you have are @Request, it doesn't work. That looks a bit like an oversight - I can't think of any reason a DI scope would require singletons.

rbygrave commented 3 years ago

it doesn't work. That looks a bit like an oversight

Yeah that should work. I'll look to try it and see why that is the case.

mikehearn commented 3 years ago

I looked at this a bit more and realised that my original assertion wasn't true - it was possible to rejig things and use the IFactory feature of picocli to let Inject build more of the objects including the original "primordial" object. A non-singleton scope sure is useful whilst refactoring though as it lets me work from the outside-in rather than inside-out.

mikehearn commented 3 years ago

Actually that didn't work out. My app initializes stuff like logging after PicoCLI has parsed command line arguments, but Inject (and any @PostConstructs) want to do things, so it was causing ordering problems and I had to back off from that approach.

I've ended with a hybrid in which I'm using @Singleton (path of least resistance at the moment) but with a factory that just reads the non-Avaje-controlled objects through a static field. So the app is two parts:

Do stuff without using Avaje and stash the results in a static field.
Call ApplicationScope.scope() which results in all the other objects being created.

It feels a bit indirect and the factory is boilerplatey, but more in the spirit of how things are meant to work given that my "requests" are actually more like executions of the tool. I can imagine wanting to re-run the tool multiple times in the same JVM in future but there are probably better ways to handle that than making the entire app a "request".

To eliminate the factory I'd need to be able to add beans to the Application scope, and tell the annotation processor to allow certain beans to be missing. But then I guess that would be very similar to a Request scope. The App/Request scope distinction still feels a bit odd, as if the core library shouldn't really have any such distinction and they're just some pre-initialized defaults or something.

rbygrave commented 3 years ago

To eliminate the factory I'd need to be able to add beans to the Application scope,

Well, if we ignore ApplicationScope and just use a programmatically created BeanScope like:

  Pump externallyProvidedPump = ...
  Heater externallyProvidedHeater = ...

  BeanScope scope = BeanScope.newBuilder()
      .withBean(Pump.class, externallyProvidedPump)
      .withBean(Heater.class, externallyProvidedHeater)
      .build();

  Pump pump = scope.get(Pump.class);

  CoffeeMaker coffeeMaker = scope.get(CoffeeMaker.class);
  coffeeMaker.makeIt();

That sounds a bit closer to this use case?

and tell the annotation processor to allow certain beans to be missing.

I haven't updated the docs here but we can do this via: @InjectModule(requires = ...). "requires" in that these are the things [that are supplied externally] that this module requires in order to wire.

Edit: So when compiling these are the types that the annotation processor allows to be missing (at compile time).

https://github.com/avaje/avaje-inject/blob/master/inject/src/main/java/io/avaje/inject/InjectModule.java#L109

example 1

@InjectModule(requires = MyExternallyProvidedThing.class)

example 2

@InjectModule(requires = {Pump.class, Heater.class})

This takes an array of Class so doesn't allow for qualifiers, just types - but I think that is ok.

rbygrave commented 3 years ago

With BeanScope created by application code we just need to remember that we need the application code to close() it in order to fire any PreDestroy lifecycle methods.

rbygrave commented 3 years ago

But then I guess that would be very similar to a Request scope.

When the application code is programmatically creating the BeanScope like the example above then this is pretty similar to request scope.

Differences:

Everything that is a @Singleton scope is created eagerly when the BeanScope is created
Everything that is a @Request scope is created on demand

Hmmm, that is really the only significant difference I can think of at the moment. Plus currently limitations on list() and listByAnnotation() that you have hit.

The App/Request scope distinction still feels a bit odd

So with the application scope the underlying BeanScope is automatically created and we have the question of how to get externally provided dependencies to it. Parking that question for a moment.

Application scope generally holds instances that are all created eagerly and live for the lifetime of the application. With request scope we can create shorter lived scopes. With request scope it will wire on demand @Request scoped beans and have "access" to all the beans held by the application scope (when wiring any request scoped instances). [Request scope makes more sense when it is used in conjunction with application scope - there are beans with both short and long lived scopes]

We can also think of application scope as just a BeanScope that is created automatically (on demand) and shutdown automatically (via shutdown hook). We can instead ignore ApplicationScope altogether and take control of creating the BeanScope and shutting it down in application code.

Rob note: Can we have a way to easily provide beans to application scope. Modify the "bootup" of application scope such that we register a callback to the ApplicationScope.init() method. https://github.com/avaje/avaje-inject/blob/master/inject/src/main/java/io/avaje/inject/ApplicationScope.java#L36

Something like ... ah, just doing a PR for this: https://github.com/avaje/avaje-inject/pull/129

mikehearn commented 3 years ago

Ah ha, yes, all that sounds right! I already realized last night that ApplicationScope is just a default-initialized BeanScope and now that works great, the factory is still there but it's not actually used. The InjectModule(requires) trick seems like the last step needed for things to make sense.

W.R.T. the initialization callback, I'm actually wondering if perhaps rather than adding new features, the ApplicationScope can just be removed? It's currently just a facade around a static BeanScope. It took a little while for me to clock this - the special casing of @Singleton made me feel like there was more to it than there really is. Given that all apps start in a main method somewhere and must at some point call ApplicationScope.get, is it actually any harder to use the library if you just require users to create their own static field with a BeanScope in it? It's then super obvious how to control what gets done and the API is made smaller / more transparent + it feels like the amount of typing saved is actually not much.

W.R.T. request vs global scope eager vs lazy init. Yes, that also confused me a bit - when I started with request scope I internalized the idea that "dependency injectors construct object graphs on demand". Then I switched to @Singleton and was surprised when everything started being created up front. I take it there's no real convention for this established by prior DI implementations?

To me, being created on demand feels more intuitive. It's closer to how hand written code behaves and it seems to have these advantages:

It means that you can put expensive stuff in @PostConstruct (or even a regular constructor?) and be sure that it won't run unless it needs to run. In a server where probably every object is used all the time, maybe it makes no difference. For what I'm using it for where objects represent some notion of work, it will be common to not instantiate every object.
Lower memory usage in cases where you don't need everything.
It makes handling of optional dependency graphs easier.

One thing that I'm currently playing with is (3). Some of my sub-object-graphs depend on configuration or options that may not be present, i.e. I have A -> B -> C where C may not be usable, and in that case neither C nor B should be constructed which is fine because B is optional to A. Factories and the nullable annotation let me have optional beans, but it's not totally clear to me how this works when everything is constructed up front.

rbygrave commented 3 years ago

ApplicationScope can just be removed?

So, I am going to say - yes.

Background story around ApplicationScope [don't read this if it's boring]. So my bias is that I've been largely writing java server side services for ages (~20 years). The servlet spec has application scope, session scope and request scope. Spring DI has ApplicationContext (which is most often tied to the servlet application scope). Back in the day when we building wars one of the dark arts was making the ApplicationContext available everywhere we needed it - largely imo because a lot of bootstrap is defined in xml (spring and web.xml both). That is, there is no main method where we bootstrap stuff using code. To make the Spring ApplicationContext available to a servlet filter it gets put into the servlet application scope etc.

I'm putting the existence of avaje-inject ApplicationScope down to my bias that comes from those days with servlet application scope and Spring ApplicationContext.

These days for server apps we have main method and code for controlling bootstrapping of the service. We don't have gnarly issues around making the BeanScope available to some far flung servlet filter (via xml config) because we are bootstrapping our service using code and using an embedded server (like jetty). Now I haven't written a "war" for at least 6 years and I suspect that a heck of a lot of devs these days don't relate to servlet application scope (it is not something they see or come into contact with).

So, I see removing of avaje-inject ApplicationScope as a really good point and it is somewhat good for me to try and explain why I think we can do that and how it got in here in the first place. I'll ping some more people to sanity check my thinking.

if you just require users to create their own static field with a BeanScope in it?

I actually think people won't even need to do this. Typically we see BeanScope using in the main method / bootstrap and actually it isn't needed anywhere else or is passed directly there.

So yeah, bit of a light bulb moment for me I think.

dependency injectors construct object graphs on demand" ... convention for this established by prior DI implementations?

As I see it there is not a convention. Spring DI has default eager singletons (and this is my defacto bias based on many years of using Spring DI). Micronaut has default lazy singletons but we can change the default globally - I first found this out when I saw confusion with people asking why their PostConstruct methods were not firing. Guice as I see it we choose more explicitly eager or lazy.

Why does avaje-inject have a bias to eager singletons?

Server apps in Kubernetes (K8s readiness probe)
Time to first response
Simplicity when BeanScope is effective immutable (for server apps)
Wiring is not expensive anymore?

1. Server apps in Kubernetes (K8s readiness probe)

Apologies if you know how K8s works. In short services have liveness and readiness probes/endpoints. liveness ~= "app is starting but not ready to take requests yet" readiness ~= "app has started, everything is good, it is ready to take requests" K8s effectively supports declarative blue/green rollout of services.

e.g. We have 3 instances/pods of my-service version 1 running. We ask K8's to rollout version 2 of that service

1 instance of my-service version 2 is started
K8's waits for it to get to readiness ok 200 status, when it does then it brings that instance into the cluster
We now have 3 instances of my-service version 1 and 1 instance/pod of my-service version 2
K8's can now gracefully terminate one of the my-service version 1 instances
rinse and repeat until all the pods/instances are my-service version 2

Now the main point here is that a service should ONLY advertise readiness ok (200 response) when it is truely and honestly ready. If it connects to databases, queues, any interesting resource it should do this before it returns readiness ok. If any of these fail (can't connect to database) the service should not come up, not say readiness ok (it goes into crash loop).

The implication is that any singleton that does expensive stuff (say cache warming) or any risky stuff that might fail (establish tcp connections etc) should be wired eagerly before readiness ok. It is good and proper for an application to fail fast.

2. Time to first response

This is to say that when a service is ready it ideally is ready for production load. It isn't great for a service to come in too cold and for the first response to take significantly longer to execute because it actually was not initialised or it is being initialised in a thread safe manor and we have locking involved so multiple concurrent requests are waiting on locks as initialisation is occurring.

It is not great to be "up" but slightly suck performance wise - being "really cold" is bad.

3. Simplicity when BeanScope is effective immutable (for server apps)

With eager initialisation we have the case that our BeanScope is effectively immutable. So in our server app space we can expect it to take a lot of concurrent requests and that is all good and simple. The only caveat is around shutdown and running preDestroy methods - we want that to occur when the app no longer has requests. K8's makes graceful shutdown nice - we just should be conscience that preDestory lifecycle is typically mutating our BeanScope (and for server apps we want that to happen after the point the service has stopped getting load).

If we don't have this simplicity then BeanScope needs to initialise things in the face of potentially many concurrent requests. It is not sufficient to use a ConcurrentHashMap - lots of things now have race conditions (with likely fast paths and slow paths implementation wise).

4. Wiring is not expensive anymore?

So optional wiring is a big thing / big feature in Spring DI and Micronaut. Only wire X based on various optional things.

Now back in the day a project had a large modular monolith spring app and this app took maybe 2 mins to startup. This type of experience is where this motivation comes from as I see it. Also as I see it people are somewhat going to trend back from everything being a microservice to some more modular monolith service apps (perhaps more conscience decisions on this rather than knee jerk architecture). So I think conditional wiring is going to be more a big thing going forward for these modular monolith apps.

However, wiring 1000 things with Dagger2 DI is roughly / approximately something like invoking 1000 methods plus some overhead (I am guessing here but it is very very close to manual written code that only invokes the injection methods). Now it takes next to no time for the JVM to execute that 1000 methods. avaje-inject does a little more than Dagger2 but not a whole lot more. avaje-inject conditionally checks if something should be wired or if it is already provided plus avaje-inject registers the wired instance into it's map. So wiring 1000 things with avaje-inject might be in the order of executing 5000 methods. For example, all the unit tests in avaje-inject pretty much wire the whole graph for each and every test invocation (and we don't really notice or care). [edited paragraph]

The way Dagger2 and avaje-inject work puts it into the "wiring is not expensive" world - this is very very different to how micronaut and spring actually work in that wiring for them is still a visible/noticeable expense. Micronaut uses annotation processing but largely only generates meta data and some stuff to avoid reflection - this is very different to what dagger and avaje-inject are doing.

Dagger2 and avaje-inject actually do the vast majority of DI work at compile time so that the actually wiring is just running a bunch of methods (that have already been ordered appropriately) and the JVM chomps that up without blinking. DI in a dagger/avaje-inject world is super cheap.

So optional wiring and lazy wiring we are going to hear a lot about going forward but this also is solving a problem I tend to not see because A) In K8s we want to fail fast and wire expensive stuff eagerly B) Wiring with avaje-inject is so cheap - way way cheaper than the folks that want optional and lazy wiring.

rbygrave commented 3 years ago

It makes handling of optional dependency graphs easier.

So I haven't had to deal with this case.

If this is at startup time and we were using Guice we would possibly use an optional module. Only include/wire this module (and wire all its dependencies) when config X = true.

Hmmm.

mikehearn commented 3 years ago

Thanks for the background. I've not used Kubernetes but I've used Borg, which is quite similar, so I totally grok those issues.

The way I'm going at the moment is to implement my own notion of optionality on top of the injector, in effect, and this might end up being more useful anyway because e.g. it makes sense to be able to tell users why a piece of work isn't available, not just that it's not there (which is all Optional<T> gives you). So maybe I don't need this from Avaje after all.

The slowness of handling concurrent build of objects lazily is a point well made.

OK, so, I guess we're in agreement that maybe ApplicationScope isn't carrying much weight these days, and maybe the whole notion of App vs Request scope can just be collapsed down into a single concept (concepts are mentally expensive, so, yay!). It'd be neat to fully generalize this, e.g.


@CreateScope
annotation class MyGlobal

@CreateScope(external = [Foo::class, Bar::class], parent = MyGlobal::class)
annotation class MyRequest

@MyGlobal class A
@MyRequest class B(val a: A)

fun main() {
  val scope = MyGlobalScope.newBuilder().build()
  val requestScope = MyRequestScope.newFrom(scope).build()
}

or words to that effect. So the core library only knows about scopes, and how you arrange them is up to you - most projects would have a global scope, but if you'd like to introduce AI in just a small part of the codebase then you don't need to injectify everything up front.

rbygrave commented 3 years ago

can just be collapsed down into a single concept

Yes, I believe this possible and yes I agree if we can distill it down to just one concept that would be great.

Noting that @Singleton is a built in @Scope ...

@Scope
@Documented
@Retention(RUNTIME)
public @interface Singleton {}

So the concept could be we could define our own scopes similarly like:

@Scope
@Documented
@Retention(RUNTIME)
public @interface MyCustomScope {}

... although the examples you gave above might be better

@CreateScope(external = [Foo::class, Bar::class], parent = MyGlobal::class)
annotation class MyRequest

What the generator currently does is create a "BeanScopeFactory" (which I think should instead be called Module). When we create a BeanScope we could specify the modules to include and perhaps a parent BeanScope.

  val scope = MyGlobalScope.newBuilder().build()

MyGlobalScope.newBuilder()
// could translate to
BeanScope.newBuilder().withModule(new MyGlobalModule())

MyRequestScope.newFrom(scope)
// could translate to
BeanScope.newBuilder().withModule(new MyRequestModule()).withParent(scope)

Ok, I think the next step is to create a branch and hack away and try and release a release candiate / prototype.

rbygrave commented 3 years ago

Putting the experiment into https://github.com/avaje/avaje-inject/pull/134

rbygrave commented 3 years ago

I have tested 6.5-RC0 and we can use custom scopes without any @Singleton so I believe we can treat this as fixed in 6.5-RC0

mikehearn commented 3 years ago

Nice! I just upgraded. It works great and my code makes much more sense now. Only thing I noticed is that I get this warning:

warning: Empty module but meta is not empty? [hydraulic.parts.tool.PartsApplication:parts]

I'm not really sure what that means. @InjectModule doesn't seem to have a meta property.

mikehearn commented 3 years ago

Ah- spoke too soon. It nearly works. The list() method still isn't getting all the interfaces+superclasses. It's now capturing the superclass hierarchy properly, but, not interfaces on both class and superclasses.

rbygrave commented 3 years ago

but, not interfaces on both class and superclasses.

Ah, interfaces of the superclasses ... yes, that will be missing. I'll create a failing test and log an issue for that.

warning: Empty module but meta is not empty? [hydraulic.parts.tool.PartsApplication:parts]

Do you see that warning all the time or has it gone away with a full rebuild? That warning is supposed to be where the 'default' module is empty (only using custom scopes) ... but it somehow ended up in the last round with dependency meta data added to the scope. In theory this suggests a @Singleton or @Factory without a custom scope annotation was compiled which added dependency meta data to the default module (which was expected to be empty).

Meta data in this sense is the dependency meta data that is used to order the construction - ordering of the build methods.

I suspect that PartsApplication is a @Factory that initially didn't have a custom scope annotation or something like that.

mikehearn commented 3 years ago

Ah ha. PartsApplication is a class being generated from an annotation processor which is annotated with @Singleton. I think that's left over from some earlier experimentation and isn't really necessary or helpful with the new beanscopes design. I'll un-annotate it.

rbygrave commented 3 years ago

Ah, interfaces of the superclasses ...

I logged that as https://github.com/avaje/avaje-inject/issues/135 and fixed in 6.5-RC1 which I have just released to maven central. So you should be able to try out 6.5-RC1 shortly @mikehearn

Oh, I think that is good news on that PartsApplication warning - I believe that warning was operating as expected.

Cheers, Rob.

mikehearn commented 3 years ago

Thanks. I wonder if the warning can be rephrased in ways that are easier to understand. I don't have any bright ideas right now tho.

rbygrave commented 3 years ago

When I put in the warning I was hoping/expecting it not to be output but now looking at the code it looks like it should only appear in exactly the case we have here:

There are no @Singleton or @Factory in the first round (or any prior first round).
A @Singleton appears in a subsequent compile round. This can only happen when that @Singleton is generated by an annotation processor. (Could be a @Factory as well of course but that is a avaje-inject annotation so unlikely).

So this warning should really say something like ...

PartsApplication is being ignored by avaje-inject as there is no 'default' module. This is expected when PartsApplication is a @Singleton being generated by an annotation processor but there is no 'default' module for it to be part of for avaje-inject dependency injection (only custom modules are being used).

rbygrave commented 3 years ago

BTW, thanks a lot @mikehearn for the discussions and critical thinking. I realise these changes might be a pain for some folks upgrading etc (I hope they forgive me) but I feel like these changes are a really big improvement / a big distillation of the concepts and the result feels really good. Ideally we have distilled avaje-inject into it's pure final form or something very close to that.

We wouldn't have got there without your thoughts and feedback - huge thanks!!

mikehearn commented 3 years ago

Seems like I've got quite some skill in hitting edge cases 😆

You're welcome - I also hope the changes aren't too disruptive. It seems like at the moment Avaje Inject is still new, and older versions worked fine, the new versions are only relevant to people who want a simpler API or are doing things with DI that's a bit off the beaten track like me.

By the way, one last question - could the docs clarify how Avaje is pronounced? Thanks!

mikehearn commented 3 years ago

I've upgraded to 6.5-RC1 but unfortunately it hasn't fixed the issue, interfaces / super-interfaces are still not being registered. I don't fully understand why because the test case you added looks very similar to what I have here. I wonder if it's because my immediate interface is parametric?

I tried building the library to see if I could debug this but failed. I can't figure out how to stop Maven trying to sign artifacts before installing them. Apparently there's no way to tell it to just not do that, you have to define a profile, but, I also can't find which git repo defined the java8-oss parent POM so I can't fork it to add that either. A bit frustrating.

mikehearn commented 3 years ago

I should note though that this isn't a big deal, because I can just list by the scope annotation and for now that is nearly equivalent - I can do my own type filtering on the list of returned objects.

rbygrave commented 3 years ago

stop Maven trying to sign artifacts before installing them.

We can use -Dgpg.skip, I should add that into the README but maybe I should put the GPG sign into a maven profile as we only need it during release.

I wonder if it's because my immediate interface is parametric?

Yes I suspect that is it. I can probably create a failing test for that case.

could the docs clarify how Avaje is pronounced? Thanks!

Ah yes, I can add that in.

Cheers, Rob.

rbygrave commented 3 years ago

I also can't find which git repo defined the java8-oss parent POM

In the pom scm section the connection tells us the source scm location. In this case:

  <scm>
    <connection>scm:git:git@github.com:avaje-pom/oss-parent.git</connection>
    <developerConnection>scm:git:git@github.com:avaje-pom/oss-parent.git</developerConnection>
    <url>git@github.com:avaje-pom/oss-parent.git</url>
    <tag>java8-oss-3.1</tag>
  </scm>

... that is a bit of an odd git repo name (historic) so probably why you could not find it. I think everything that goes to maven central must have that the scm section and certainly anything that uses the release plugin needs it.

The git repo oss-parent redirects to https://github.com/avaje-pom/java8-oss (as I have java11-oss now as well).

rbygrave commented 3 years ago

FYI @mikehearn just released 6.5-RC2 which has support for generic types. I believe that solves that issue with the parametric type. If not let me know what the types are and I'll have a look. Also this version includes an improved warning message for PartsApplication.

Cheers, Rob.

mikehearn commented 3 years ago

I was unable to upgrade as the generated code no longer compiles:

/Users/mike/Library/Caches/gradle-build/product/hydraulic.parts/generated/source/kapt/main/hydraulic/parts/tasks/mac/NotarizeTask$DI.java:28: error: cannot find symbol
  public static final Type TYPE_BuildTaskWithResultT = new GenericType<BuildTaskWithResult<T>>(){};
                                                                                           ^
  symbol:   class T
  location: class NotarizeTask$DI

It seems like a type variable is not getting properly erased somewhere? If this error isn't enough to spot the issue I can send some .class files or try to debug it if you like.

rbygrave commented 3 years ago

Ok Mike, That issue should be fixed in version 6.5-RC3 via https://github.com/avaje/avaje-inject/issues/139

In this case it will register the type BuildTaskWithResult.class and NOT generate a GenericType type and associated provider method.

mikehearn commented 3 years ago

RC3 is golden! Ship it!

rbygrave commented 3 years ago

Awesome, will do !!

mikehearn commented 3 years ago

Thanks for all your hard work on this. Avaje Inject is now really the first time I've felt comfortable with a DI framework. All the previous ones I've seen seemed very strongly tied to big server frameworks, or destroyed type safety, or were confusing or very slow. I was sure this approach was not for me. 6.5 seems to be hitting a sweet spot though. Simple, debuggable, the APIs are small and make sense.

Given all the progress that's been made, I'll close this issue and open new ones for new ideas.

avaje / avaje-inject