Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BeanScope PreDestroy #750

Closed
wants to merge 2 commits into from
Closed

BeanScope PreDestroy #750

wants to merge 2 commits into from

Conversation

SentryMan
Copy link
Collaborator

@SentryMan SentryMan commented Jan 4, 2025

Now can do

  @PreDestroy(priority = 100)
  void preDestroy(BeanScope scope) {
  
   // use the beanscope to close tricky beans

  }
  • Adds a new interface for pre-destroy hooks that accept the current BeanScope
  • refactor internals to convert AutoCloseable Interfaces for compatibility
  • refactor postConstruct internals to convert Runnable to Consumer<BeanScope> instead of having two separate lists

Solves #749

@SentryMan SentryMan added this to the 11.1 milestone Jan 4, 2025
@SentryMan SentryMan self-assigned this Jan 4, 2025
@SentryMan SentryMan added the enhancement New feature or request label Jan 4, 2025
@SentryMan SentryMan requested a review from rbygrave January 4, 2025 22:54
@SentryMan SentryMan enabled auto-merge January 4, 2025 22:54
@rbygrave
Copy link
Contributor

rbygrave commented Jan 5, 2025

I don't like this idea of void preDestroy(BeanScope scope) as we are invoking methods on a BeanScope that is in the process of "shutting down" - I feel there will be a lot of problematic cases / race conditions etc.

My preference would be to NOT support this. Currently there is no actual known use cases for PreDestroy methods that need to take the BeanScope right?

@ascopes
Copy link

ascopes commented Jan 5, 2025

@rbygrave another option could be to allow the BeanScopeBuilder to be passed as a parameter to Bean methods. That'd allow registration via the destroy hook method.

@ascopes
Copy link

ascopes commented Jan 5, 2025

Looking at https://docs.oracle.com/javase/8/docs/api/javax/annotation/PreDestroy.html it violates these constraints too.

@rbygrave
Copy link
Contributor

rbygrave commented Jan 5, 2025

another option could be to allow the BeanScopeBuilder

I'm suggesting we try to stick to "First Principals" and first understand clearly the use cases that are not supported nicely/elegantly and the options for handling these cases. Currently, the ONLY use case we have at this stage is the Vertx one with a PreDestroy method that has method chaining .close().blockingAwait() [and we can actually support this by extending the existing mechanism - I have a PR ready to go for that].

We have no other use cases presented to us [yet]. That is, we currently have no use cases of a PreDestroy method that actually needs BeanScope. We currently have no use cases of a PreDestroy method that needs any parameters [and yes, as you point out if we did that would violate the standard there].

@rbygrave rbygrave removed this from the 11.1 milestone Jan 6, 2025
@SentryMan SentryMan closed this Jan 7, 2025
auto-merge was automatically disabled January 7, 2025 01:46

Pull request was closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request refactor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants