Feature request: Allow factory beans to have a @PreDestroy method to match/mirror a @Bean method

[ascopes]

Edit: This is an edit to the summary as a TLDR on what this enhancement request has become.

The problem is specific to @Factory components that have methods that create @Bean components, and wanting to have a @PreDestroy method invoked on those beans.

The use case is that using Vertx the desired PreDestroy is actually a chained method invocation of vertx.close().blockingAwait();.

Change 1 - Change @Bean(destroyMethod) to support method chaining

So for example @Bean(destroyMethod="close().blockingAwait()") now works. This change was added via #754

Change 2 - Allow Factories to have a @PreDestroy method that matches a @Bean method

Factories naturally have @Bean methods to create components and it seems natural to allow those factories to have a @PreDestroy method that would match, for example:

@Factory
final class MyFactory {

  @Bean 
  Vertx vertx() {
    var opts = new VertexOptions().setUseDaemonThread(true);
    return Vertx.vertx(opts);
  }
  
  @PreDestroy
  void destroy(Vertx vertx) {
    vertx.close().blockingAwait();
  }
}

This is supported via #756

Also changing the title to reflect that this issue relates specifically to factories only. Normal @Singleton components can and should have their own @PreDestroy methods and shutdown logic (and should not have an extra dependencies required to execute the shutdown logic - aka normal @PreDestroy methods are not allowed to have arguments).

---

First off, thanks for the awesome annotation processor library! This has slotted really nicely into some work I am doing to make some components much easier to manage without the overhead and hassle of a full runtime CDI framework like Spring!

I have a small issue and I am wondering if a feature could be introduced to help address it.

Background

In my application, I am making use of Vert.x RXJava3 components. I have some configuration resembling the following:

import io.avaje.inject.Bean;
import io.avaje.inject.Factory;
import io.vertx.core.VertxOptions;
import io.vertx.rxjava3.core.Vertx;

@Factory
public class MyContext {
  @Bean 
  public Vertx vertx() {
    var opts = new VertexOptions().setUseDaemonThread(true);
    return Vertx.vertx(opts);
  }
}

My issue revolves around the ability to safely destroy the vertx bean so that the threadpool is closed prior to the BeanScope being fully closed. This is useful as it means I do not have to worry about Vertx retaining ports in the background between integration test cases.

Since this is a reactive shim around "regular" Vert.x, methods for object closure are non-blocking, as one may expect.

This means that the following or something similar has to be invoked to shut down my event loop:

vertx.close().blockingAwait();

Unfortunately, I cannot find a "nice" way of handling this with Avaje. In Spring, I'd usually "abuse" the cglib method proxying to allow referencing the bean within the component, like so:

@Configuration
public class MyConfiguration {
  @Bean 
  public Vertx vertx() { ... }
  
  @PreDestroy
  public void close() {
    // calls to the bean are intercepted by spring and injected instead.
    vertx().close().blockingAwait();
  }
}

In Avaje, as expected, this will not work as cglib proxies that modify method call behaviours are not used.

Feature request

What I'd like to be able to say is something like this:

import io.avaje.inject.Bean;
import io.avaje.inject.Factory;
import io.vertx.core.VertxOptions;
import io.vertx.rxjava3.core.Vertx;

@Factory
public class MyContext {
  @Bean 
  public Vertx vertx() {
    var opts = new VertexOptions().setUseDaemonThread(true);
    return Vertx.vertx(opts);
  }
  
  @PreDestroy
  public void destroy(Vertx vertx) {
    vertx.close().blockingAwait();
  }
}

i.e. where dependencies can be injected into the signature of @PreDestroy hooks.

What I have tried

I have tried some workarounds:

Storing Vertx in a field

@Factory
public class MyContext {
  private final Vertx vertx;
  
  public MyContext() {
    var opts = new VertexOptions().setUseDaemonThread(true);
    vertx = Vertx.vertx(opts);
  }

  @Bean 
  public Vertx vertx() {
    return vertx;
  }
  
  @PreDestroy
  public void destroy(Vertx vertx) {
    vertx.close().blockingAwait();
  }
}

This works but feels like it violates the purpose of factory methods.

Registering destroy hooks with the BeanScope

The BeanScope itself does not let me add hooks by the looks of things, so I instead tried this:

@Factory
public class MyContext {
 
  @Bean 
  public Vertx vertx(BeanScopeBuilder beanScopeBuilder) {
    var opts = new VertexOptions().setUseDaemonThread(true);
    var vertx = Vertx.vertx(opts);
    beanScopeBuilder.addPreDestroy(() -> vertx.close().blockingAwait());
    return vertx;
  }
}

...however, the BeanScopeBuilder does not appear to be able to inject itself, so I get a NullPointerException.

Registering a sibling bean

@Factory
public class MyContext {
 
  @Bean 
  public Vertx vertx() {
    var opts = new VertexOptions().setUseDaemonThread(true);
    return Vertx.vertx(opts);
  }
  
  @Bean
  public Object vertxReaper(Vertx vertx) {
    return new Object() {
      @PreDestroy
      public void destroy() {
        vertx.close().blockingAwait();
      }
    };
  }
}

...this never appeared to be invoked.

Registering a singleton

@Factory
public class MyContext {
 
  @Bean 
  public Vertx vertx() {
    var opts = new VertexOptions().setUseDaemonThread(true);
    return Vertx.vertx(opts);
  }
}

@Singleton
public class VertxReaper {
  private final Vertx vertx;
  
  @Inject
  public VertxReaper(Vertx vertx) {
    this.vertx = vertx;
  }
  
  @PreDestroy
  public void destroy() {
    vertx.close().blockingAwait();
  }
}

This works, but feels very clunky and verbose.

Hacking around with destroyMethod hooks on the @Bean annotation

@Factory
public class MyContext {
 
  @Bean(destroyMethod="close().blockingAwait")
  public Vertx vertx() {
    var opts = new VertexOptions().setUseDaemonThread(true);
    return Vertx.vertx(opts);
  }
}

This does not work (and would be a fragile hack regardless).

Closing Vertx outside the bean scope

public class Main {
  public static void main(String[] args) {
    try (var ctx = BeanScope.builder().modules(new Mymodule()).build()) {
      try {
        ctx.get(ThingThatRuns.class).await();
      } finally {
        ctx.get(Vertx.class).close().blockingAwait();
      }
    }
  }
}

This works but means my other classes now have to be called from outside the CDI context, which feels like it is creating fragile cross-cutting concerns.

---

If you any suggestions on this, it would be greatly appreciated!

Thanks

[SentryMan]

I like it, when I get back I'll add it. In the mean time, have you tried registering a sibling bean of type AutoCloseable? (The annotation processor can't read anonymous classes, but autocloseable beans have the close method auto registered as pre destroy hooks)

Could maybe be done like.

@Factory
public class MyContext {
 
  @Bean 
  public Vertx vertx() {
    var opts = new VertexOptions().setUseDaemonThread(true);
    return Vertx.vertx(opts);
  }
  
  @Bean
  public AutoCloseable vertxReaper(Vertx vertx) {
    return ()-> vertx.close().blockingAwait();
  }
}

[ascopes]

I didn't try that but will give it a go. Thanks!

[rbygrave]

@bean(destroyMethod="close().blockingAwait")

This could be made to work easily enough.

and would be a fragile hack regardless).

Specifying a destroyMethod as close().blockingAwait() isn't really any more "fragile" than close(). With avaje-inject the destroy methods results in generated source code and that generated source also needs to compile - so a typo, incorrect method, or change of method would actually result in a compilation error per say unlike the use of reflection or the generation of bytecode. So I don't think this is "more fragile" than the current feature.

In this way we can allow chained method invocations like @bean(destroyMethod="close().blockingAwait()") ... and really that isn't conceptually different to the current direct methods supported. It's just a chained method invocation instead.

What I'd like to be able to say is something like this

The issue I see with this enhancement is this is that as soon as we can go @Bean(destroyMethod="close().blockingAwait()") then the motivation somewhat disappears? What other use cases do we need the enhancement for?

The enhancement might be a good idea but it isn't clear to me yet.

[ascopes]

While it could work, what if you need to pass parameters to the closure method, such as a scheduler to a subscribe call?

It would also be fragile if you treated it as pure executable code rather than a method name, since you could have cases like this:

class Foo {
  int bar;
  public int bar() { return bar; }
}

@Factory
class Baz {
  @Bean(destroyMethod = "bar")
  Foo foo() { return new Foo(); }
}

...you'd now have ambiguity between the field and the method without further semantic analysis.

Another case I thought of is that you technically leak encapsulation by doing this too.

@Bean(destroyMethod = "this.somethingElse()")
public Xxx yyy() { ... }

where this will be the BeanScopeBuilder. This feels like it could result in encouraging very messy and hacky code with other consequences.

I think being able to pass params to close hooks would align with other frameworks, and in a less hacky way. That's be my ideal solution here.

Although it could be possible to do both, I guess?

[rbygrave]

I think being able to pass params to close hooks would align with other frameworks in a less hacky way.

There is effectively a race condition being created wrt the params [the state of the params when the destroy method is invoked]. So as I see it passing params in this fashion is actually the more implicit and "hacky" and prone to error in that people are not actually taking control the dependencies required in order to shutdown something. Being explicit for these cases is what we need to do today and I think that is good.

Perhaps you could be more explicit with the use case. Give us a practical example?

what if you need to pass parameters to the closure method, such as a scheduler to a subscribe call?

You can't do that - it is a limitation [and I'm suggesting that is a good limitation], instead developers with shutdown logic that have dependencies need to own that explicitly [just as they need to today].

Note that the VertxReaper does not have to be public and imo it should NOT be public, it can be an inner class of the factory to keep it local etc.

technically leak encapsulation by doing this too.

Via @Bean(destroyMethod = "this.somethingElse()")? I don't follow you here. This would actually not compile per se ... would generate () -> $bean.this.somethingElse() as the closure and that wouldn't compile.

where this will be the BeanScopeBuilder.

That doesn't make sense to me? You don't and should not have access to the BeanScopeBuilder in a PreDestroy method. I don't follow this part.

[rbygrave]

you'd now have ambiguity between the field and the method

No ambiguity. PreDestory methods are method invocations only.

[ascopes]

Would this not require more complicated validation logic in this case then? Otherwise it results in garbage being passed into the code generator... that is the point I was trying to make.

Passing parameters - You can't do that

So suppose you have a type like so, from a third party library outside your control.

class MessageConsumer {
  ...

  void close(boolean awaitThreadTermination) { ... }
}

How would you handle this case?

[rbygrave]

Would this not require more complicated validation logic in this case then? Otherwise it results in garbage being passed into the code generator.

People can put garbage into the String of the @bean(destroyMethod = "... garbage ...") ... and the source code generated will contain () -> $bean.... garbage ... and won't compile. I see no extra validation logic required ... along the lines that you'll need to put in method chaining that will actually compile [and garbage won't compile just like typo's or spelling mistakes won't compile ].

MessageConsumer

2 special cases here in this example in that A) it has a parameter and B) it is a "Message Consumers" and should be in a specific place in overall

A) Write the code that determines the value of boolean awaitThreadTermination ... and WHEN that is evaluated and then you probably know how to deal with it. That is, the determination of awaitThreadTermination might be at startup via some configuration or determined at runtime at the actual time of shutdown. Write that code / the MessageConsumerReaper - the more logic and dependencies it has the more likely its an inner class of a @Factory.

B) Overall graceful shutdown ordering should be:

• Stop http server accepting new requests, K8s Readiness Off
• Wait for any in flight http requests to complete
• *Stop any "Message Consumer" threads [typically wait for them to complete]
• *Run Dependency Injection PreDestroy methods in order
• Stop any background thread pools
• Shutdown any connection pools

* When "Message Consumer" shutdown is controlled by DI then I'll suggest that they are almost always the first DI lifecycle components that should be shutdown.

[ascopes]

Stop any "Message Consumer" threads [typically wait for them to complete]

So in the case that the MessageConsumer threads are in an ExecutorService, you'd call .shutdown() and .awaitTermination(long, unit); as opposed to .shutdownNow(). How would that look, given the awaitTermination is the part that covers the "wait for them to complete" part? Could you show an example?

[SentryMan]

@bean(destroyMethod = "this.somethingElse()")

I always thought this was the hacky way of doing things, as you're basically coding without an ide.

The problem I find with generating un-compilable code is that it can cause problems for any apt that reads it in subsequent apt rounds. Annotation processor problems tend to cascade, which can hide the real issue that caused compilation to fail.

Trying to diagnose processor problems due to improperly generated code has been really annoying for me personally, so I'm not exactly a fan of it anymore.

[rbygrave]

basically coding without an ide.

Hacky in a sense yes but it has the advantage of no ambigiuty in what its trying to do.

@bean(destroyMethod = "...")

This has the advantage that it has a 1 to 1 relationship with the bean being created so there is no ambiguity in the concept of what it does / what it is trying to achieve / why it is there [and Spring and Micronaut do this exact same thing so its expected]. The only way this messes up is the stringy conversion to source code creating a compile error ... but imo this is the "best sort of failure" in that it simply will not work.

When we say, add a feature to say allow a method on a factory to be marked as @PreDestroy and allow that to take an argument we have a bunch of ambiguity that we need to deal with (via documentation or validation in the annotation processor and compiler errors etc) because that method is not strictly 1 to 1 related to the @Bean method. What arguments are allowed, how many arguments are allowed, what if there are qualifiers, can we do this only on factory classes or can we also do this on normal components, can it be a static method etc.

[SentryMan]

When we say, add a feature to say allow a method on a factory to be marked as @PreDestroy

I understood the request to have a feature of pre-destroy in general, not specific to factories.

Spring

at least for spring you seem to be able to do stuff like this:

  @PreDestroy
  public void close() {
    vertx().close().blockingAwait();
  }

Which to me is even less intuitive than what @ascopes seems to be proposing

What arguments are allowed

Any kind, it'll work like how method injection happens today. (or see that other PR I made for postConstruct)

how many arguments are allowed,

As many as one needs

what if there are qualifiers,

It'll generate in a similar way as in method injection.

can we do this only on factory classes or can we also do this on normal components,

I never really envisioned this as a factory exclusive feature. In fact it can potentially be useful for InjectPlugins, since it allows for more specific actions on close.

can it be a static method.

what?

[rbygrave]

So going back to the original enhancement request ...

What I'd like to be able to say is something like this:

import io.avaje.inject.Bean;
import io.avaje.inject.Factory;
import io.vertx.core.VertxOptions;
import io.vertx.rxjava3.core.Vertx;

@Factory
final class MyContext {
  @Bean 
  Vertx vertx() {
    var opts = new VertexOptions().setUseDaemonThread(true);
    return Vertx.vertx(opts);
  }
  
  @PreDestroy
  void destroy(Vertx vertx) {
    vertx.close().blockingAwait();
  }
}

This makes sense to me as long as we remove the ambiguous cases. Along the lines:

• When a factory has a PreDestroy method with ZERO args its the PreDestroy for the factory itself [this is current behaviour]
• When a factory has a PreDestroy method with ONE arg only, that argument type MUST match a type of @Bean created by the factory. This then relates that method as a PreDestroy method for any @Bean of that type in that factory regardless of qualifier [it is matched on type only].
• A PreDestroy method with more that ONE arg is invalid, ideally detected and gives compilation error
• A PreDestroy method that is static is invalid, detected and gives a compilation error

I understood the request to have a feature of pre-destroy in general, not specific to factories.

This is not as I understand it @SentryMan. To me, what you are suggesting seems too far away from the spec and concept of @PreDestory.

Edit: I'm going to suggest that this feature must ONLY relate to factories because any other components can and should have their own @PreDestory method. That is, this only relates to factories because it only relates to components created via @Bean methods.

[SentryMan]

To me, what you are suggesting seems too far away from the spec and concept of @PreDestory.

I've always used @PreDestory as a "thing that runs before a bean is to be destroyed", so I suppose I'm rather flexible.

For me, the main reason to allow a beanscope in @PreDestroy methods is to have symmetry with the @PostConstruct beanscope feature we already have. (It'll also allow InjectPlugins to register useful preDestroy hooks for beans as well)

I'm going to suggest that this feature must ONLY relate to factories

With this constraint, I find the feature much less interesting.

For factories, I believe the same can essentially be accomplished with even more potency using:

  @Bean
  public AutoCloseable vertxReaper(Vertx vertx) {
    return () -> vertx.close().blockingAwait();
  }

[rbygrave]

reason to allow a beanscope in @PreDestroy methods is to have symmetry with the @PostConstruct

For me, I don't think of them as symmetrical because PreDestroy methods are destructive. The ideal is for components to know how to shutdown/close themselves and then just invoke those in the correct order. The ideal is for shutdown to be as simple as possible and to keep it as simple as possible. We don't really want to see shutdown logic that depends on other components that themselves have shutdown logic etc.

@PostConstruct needed the flexibility to support generic components.

For factories, I believe the same can essentially be accomplished with even more potency using:

I initially thought that too but when I looked at it there is a relatively subtle issue that without qualifiers it breaks once we want more than 1 AutoClosable and to me I think it will break in a confusing way. Only 1 AutoClosable will be wired and only 1 AutoClosable will actually run (unless we use qualifiers).

There might be another way to look at it but currently I think for that to be an option an @Bean AutoCloseable might need special treatment.

[ascopes]

We don't really want to see shutdown logic that depends on other components that themselves have shutdown logic etc.

Is it appropriate for Avaje to dictate this where the shutdown logic is user-provided, or is it down to the user to ensure their shutdown logic is sensibly ordered and maintainable? I appreciate that special cases should not be special enough to break the rules, but at the same time I am not sure if the concern of application logic outside dependency injection is something that Avaje should be attempting to control. It makes assumptions about the codebases and libraries outside Avaje that are being used being implemented in an "ideal" way, which can result in the user being penalized for something that is not in their control either, arguably (if no alternatives exist, anyway).

What do other CDI frameworks do for this? I have experience with Spring and a small amount of experience with Guice (but only for Maven Plugins).

[rbygrave]

Is it appropriate for Avaje to dictate this where the shutdown logic is user-provided

Assuming I understand the question correctly - No, we want user logic to control interesting stuff like that. I gave that example just to point out why we don't want @PreDestory methods to be "clever" or have dependencies / arbitrary args / access to BeanScope. edit: If developers need to create something like the VertxReaper in order to control interesting shutdown logic then that is what we want them to do - we don't want avaje-inject to be clever here.

avaje-inject allows via @PreDestroy [which is from JSR-250 lifecycle annotations] and @Bean(destroyMethod=...) [which is from Spring] to specify which shutdown methods to invoke when the DI BeanScope is shutdown. I'm suggesting this is pretty standard for DI libraries that support lifecycle methods.

avaje-inject uses the [non-standard] priority attributes on those to control the ordering of those method invocations.

That's it, the rest is up to the developer.

What do other DI frameworks do for this?

@PreDestroy [from JSR-250 lifecycle annotations] is standard and commonly supported. Spring, Micronaut, Avaje-Inject additionally all follow the Spring feature of factory/configuration classes with @Bean(destroyMethod=...) methods.

Avaje-Inject has the non-standard priority attribute which we can use to control the ordering of the shutdown method invocation. Spring does not have this and instead you are expected to use @DependsOn to control shutdown ordering. Guice didn't support the standard lifecycle annotations [and I think that is still the case].

Not sure if that answered all of your question?

[SentryMan]

we want more than 1 AutoClosable

For what purpose? It can all be handled via the one.

@Factory
public class SomeFactory {

  @Bean
  public TrickyClose c1() {
    //return something;
  }
  @Bean
  public TrickyClose2 c2() {
    //return something;
  }
  
  @Bean
  public AutoCloseable reaper(TrickyClose c1, TrickyClose2 c2) {
    return () -> {
      // close them both
    };
  } 
}

[rbygrave]

For what purpose? It can all be handled via the one.

One across ALL factories though [as in 1 AutoClosable in the BeanScope]. I'm not sure it's ideal for the case where they have different priorities per se? Hmmm.

[rbygrave]

To clarify, I don't see the AutoCloseable closure approach working due to the subtle issue around "1 AutoClosable in the BeanScope / needs qualifiers etc".

I'm comfortable that we can merge/include #756 and that it is intuitive enough that the ambiguities around possible multiple PreDestroy can be managed.