[bv-dev] Do we need ConstraintValidatorFactory#releaseInstance()?

[Emmanuel Bernard]

Thanks you for the sum up.

This email is not a rebuttal, more a set of remarks on the subject.

If we manage to remove `releaseInstance()` then we should also remove
`ValidatorFactory.close()`. AFAIR, the only reason for `close()` to
exist is to allow proper implementations of releaseInstance.

The ConstraintValidatorFactory contract is a public contract that can be
implemented by anyone. This is also the way for DI framework to provide
injectable `ConstraintValidator` implementations.

In my mind and in retrospect, `ConstraintValidatorFactory#getInstance()`
should have been named `createInstance()`. Remember, caching is not
allowed in a `CosntraintValidatorFactory` implementation. So you
argument that what creates an object should destroy it applies here:
what requests an object creation should request its destruction and
that's exactly why we have added this method.

>From what you guys say, all implementors of object lifecycle management
libs (let's say DI frameworks) have a @PreDestroy logic that is called
when the CVF bean would be destroyed by its creator. It seems though
that the @PreDestroy method would have to process all objects
returned by `getInstance()`. So `getInstance` would need to keep track
of them in a concurrent structure of some sort.
Is that significantly better than having a `releaseInstance` contract?

Pure JSR-330 implementations have no @PreDestroy concept. Nor does Guice
AFAIK.

Today a Bean Validation implementation can cache ConstraintValidator and
discard them upon memory pressure or if they are never used. If the
@PreDestroy pattern is used, then this optimization is useless as the
object would not be garbage collected until after the CVF is explicitly
destroyed.

It is possible to pass a ConstraintValidatorFactory instance for a
specific validator instance

    Validator validator = validatorFactory
        .usingContext()
            .constraintValidatorFactory(someCVFInstance)
        .getValidator();

In this case and according to the current draft of BV 1.1, the Bean
Validation provider will likely call `getInstance` and `releaseInstance`
for each validate* calls on the `Validator`. It could technically keep
the custom CVF instance around to check if it is reused in a subsequent 
`Validator` creation.

Because of:

- the custom CVF instance per Validator
- the ability to offload cached ConstraintValidator instances
- the ability to implement a dump BV provider that caches nothing

I added what turned out to be a mysterious provision in the spec

> `releaseInstance()` is "typically" invoked when the ValidatorFactory is
> closed

So from what I understand, the choice is between:

- explicit releaseInstance and VF.close() with the benefit of more
  optimizations wrt memory usage - at least not involving custom weak
  reference like implementations
- rely on the @PreDestroy approach - thus limiting CVF implementations with such
  feature - bringing simplicity to the BV provider implementations and
  removing VF.close()

Thoughts?

Emmanuel

On Mon 2012-12-03 12:44, Gunnar Morling wrote:
> Hi all,
> 
> The latest RI version [1] also implements the spec changes regarding the
> integration of BV and CDI.
> 
> During implementation Hardy, Emmanuel and I got into a discussion regarding
> the lifecycle of CDI-managed objects [2] which I'd like to continue.
> 
> The question is whose responsibility is it to release involved objects such
> as message interpolator, traversable resolver or constraint validators.
> Generally we think objects should be released by that party/code that
> created them.
> 
> This means:
> 
> 1) objects created by the user and passed via bootstrap API: to be released
> by user
> 2) objects created by BV implementation (e.g. internal caches): to be
> released by BV implementation
> 3) objects passed by CDI integration layer (e.g. CDI-enabled message
> interpolator etc. based on BootstrapConfiguration): to be released by
> integration layer
> 
> I think 1) is not in our focus, let the user deal with it. For 2) we have
> ValidatorFactory#close(). Interesting is 3). A CDI portable extension could
> make sure that its created managed beans are properly released using CDI's
> pre-destroy hook [3].
> 
> But that way a CDI-enabled ConstraintValidatorFactory could also easily
> release all the constraint validators it created:
> 
> CdiConstraintValidatorFactory implements ConstraintValidatorFactory {
> 
>   public <T extends ConstraintValidator<?, ?>> T getInstance(Class<T> key) {
>     //create validators. keep track of them.
>   }
> 
>   @PreDestroy
>   public cleanUpValidators() {
>     //release created validators
>   }
> }
> 
> That is, we'd again adhere to the principle that whoever creates objects
> has to release them. When following this approach, we actually wouldn't
> need ConstraintValidatorFactory#releaseInstance() anymore. I think this
> would be a good thing as it IMO simplifies the contract between the BV
> provider and code integrating with it (such as a CDI portable extension)
> and clarifies responsibilities.
> 
> In the spec we would describe that a CDI-enabled CVF has to release its
> validators when it itself is going to be destructed and for
> ValidatorFactory#close(), that a BV provider has to release its internal
> state but no objects passed to it.
> 
> One thing not possible with that approach is to release CV instances before
> the application is shut down. The spec currently says that
> releaseInstance() is "typically" invoked when the ValidatorFactory is
> closed, but AFAICS it leaves room for releasing constraint validators also
> in between. OTOH I wouldn't know when/how a BV provider would decide to do
> that.
> 
> Any thoughts?
> 
> --Gunnar
> 
> [1] http://in.relation.to/Bloggers/HibernateValidator500Alpha2And431Final
> [2] https://hibernate.onjira.com/browse/BVAL-338
> [3] http://docs.oracle.com/javaee/6/tutorial/doc/gmgkd.html

> _______________________________________________
> beanvalidation-dev mailing list
> beanvalidation-dev at lists.jboss.org
> https://lists.jboss.org/mailman/listinfo/beanvalidation-dev