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.