add @AutoClose

[Ladicek]

Fixes #876

[Ladicek]

This is a draft because it also includes the @Eager commits (that are also present in #911). Please ignore the first 2 commits and only look at the last.

I'd especially welcome review of the changes to the Portable Extensions API, because I'm not familiar with those.

[Azquelt]

Just to check I have this right:

• A bean can be auto-closable, because it has the @AutoClose annotation, but not implement AutoClosable, in which case nothing happens.
• AutoClosable does not need to be one of the bean types. It's sufficient for the contextual instance to implement AutoClosable.
• The order of operations when destroying a bean is:
  • call PreDestroy lifecycle callback interceptor methods
  • destroy the bean instance
    • call the disposer method (for producer methods)
  • call AutoClosable.close
  • destroy any dependent objects

I'm having trouble working out the order of operations for auto-closable synthetic beans. I originally had this:

• call PreDestroy lifecycle callback interceptor methods
• destroy the bean instance
  • call the disposer method (for producer methods)
  • call Contextual.destroy (for portable extension synthetic beans)
  • call SyntheticBeanDisposer.dispose (for lite extension synthetic beans)
• call AutoClosable.close
• destroy any dependent objects

However, Contextual.destroy says that implementations should call CreationalContext.release to destroy dependent objects, so we'd need to call AutoClosable.close before that?

[Ladicek]

• A bean can be auto-closable, because it has the @AutoClose annotation, but not implement AutoClosable, in which case nothing happens.

Correct.

• AutoClosable does not need to be one of the bean types. It's sufficient for the contextual instance to implement AutoClosable.

Correct.

• The order of operations when destroying a bean is:
  • call PreDestroy lifecycle callback interceptor methods
  • destroy the bean instance
    • call the disposer method (for producer methods)
  • call AutoClosable.close
  • destroy any dependent objects

This is a bit more involved, but essentially correct. Note that "destroying an instance of a bean" is what happens during Contextual.destroy(). It is:

1. Call the @PreDestroy life cycle callbacks in case of managed beans. Call the disposer method in case of producer beans. Call the disposal callback in case of synthethic beans.
2. Call AutoCloseable.close() in case the bean is auto-closeable and the class of contextual instance implements AutoCloseable.
3. Destroy dependent instances.

The following is wrong. I covered synthetic beans above, but I'll cover the issues here as well:

I'm having trouble working out the order of operations for auto-closable synthetic beans. I originally had this:

• call PreDestroy lifecycle callback interceptor methods

No. There are no @PreDestroy callbacks for synthetic beans.

• destroy the bean instance
  • call the disposer method (for producer methods)

No. There are no disposer methods for synthetic beans, synthetic beans are not producer methods. Instead, we call the disposer callback as configured through the Portable Extensions configurator API / through the Build Compatible Extensions API.

If the user provided a custom Bean implementation, they are on their own.

* call `Contextual.destroy` (for portable extension synthetic beans)

Hard NO! Contextual.destroy() does this entire process, it's not called as part of it. As mentioned above, we call the disposer callback as configured.

* call `SyntheticBeanDisposer.dispose` (for lite extension synthetic beans)

Right, as mentioned above.

• call AutoClosable.close

Right.

• destroy any dependent objects

Right.

However, Contextual.destroy says that implementations should call CreationalContext.release to destroy dependent objects, so we'd need to call AutoClosable.close before that?

I think I covered this above. The main misconception comes from the nature of Contextual.destroy(): it is not called as part of bean instance destruction process -- it is the bean instance destruction process.

[Azquelt]

* call `Contextual.destroy` (for portable extension synthetic beans)

Hard NO! Contextual.destroy() does this entire process, it's not called as part of it. As mentioned above, we call the disposer callback as configured.

Aha, this was the bit that I was confused by, having previously had to do a custom Bean implementation.

It seemed odd to have an isAutoClose() method on Bean if the user had to also implement it themselves in destroy, but I guess in most cases they won't, as they should use BeanConfigurator instead in which case the container calls their dispose or destroy callback at the right time and will also call close() for auto-closable beans.

[Ladicek]

Rebased. This still includes the @Eager commits and so remains a draft. Hopefully the @Eager PR will be merged soon. I also need to talk to @manovotn about the Portable Extensions SPI changes.

[manovotn]

Rebased. This still includes the @Eager commits and so remains a draft. Hopefully the @Eager PR will be merged soon. I also need to talk to @manovotn about the Portable Extensions SPI changes.

Just putting this out here for wider audience.
We've had a short exchange over Zulip so far and I think the two main areas of interest are Producer (and InjectionTarget as its subclass) and then custom Bean implementations (custom Contextual implementation to be precise).
Both can be directly registered or overridden via Portable extensions.
Furthermore, users can access and use some of these API via BeanManager#getProducerFactory() or BeanManager#createBean(...)

I think the nature of this PR covers it well. All standard beans ultimately use Producer#dispose so putting the logic there accounts for all the various cases except synthetic beans. Those are a special case and we can only handle them in case user doesn't override the Bean#destroy logic ( so beans added via configurators should work just fine). That's nothing new though; even today users that define their own custom Bean implementations have to bear the responsibility for their creation/destruction.

[Ladicek]

Right. I think it would be fair to override destroy() on Bean to be able to document the auto-close logic.

It also seems fair to add to the other places that the destruction callbacks must not call CC.release().

[Ladicek]

Added a commit with an override of Contextual.destroy() on Bean. Please take a look.

I eventually decided that we should not clutter the javadoc of public APIs with minutiae about CC.release(); it is well defined in the CDI specification. I just slightly modified the Destruction of dependent objects chapter of the spec to also mention disposer methods in addition to @PreDestroy callbacks, because that chapter is referenced from both Lifecycle of managed beans and Lifecycle of producer methods.

[Ladicek]

Also, I'm still not sure if making Producer.dispose() responsible for calling AutoCloseable.close() is the right thing to do. I'll work on TCK tests, but in the mean time, I'd like @manovotn to at least experimentally implement this in Weld to see if integration with other Jakarta EE specs isn't affected.