2012/12/4 Emmanuel Bernard <emmanuel@hibernate.org>

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()

Yes, I think that's basically the two options we have.

I'm still not sure whether we really do gain much by releaseInstance(). I think the argument for it is that it allows for releasing validators upon memory shortage *before* ValidatorFactory#close() is called.

But when and how would a BV provider make use of this? Right now I can see two possible implementation approaches:

1) A BV provider uses a fixed-size cache of validators and calls CVF#releaseInstance() on an LRU basis for recently unused validators. But how would this cache be sized? I think it could easily be either too small (causing new validators to be created unnecessarily) or too large (causing memory issues).

2) A BV provider uses a Reference based cache, utilizing as much memory as possible. In case of memory shortage the provider would call releaseInstance() for freed instances.

To me, option 1) seems not so good due to the sizing issue, so I'd personally go for option 2). But if a BV provider has to deal with References anyways, wouldn't it be simpler to do the same in CVF implementations (properly disposing validators once they're freed)?

By using Reference based caches in CVF and BV provider, I think there would also be no problem for frameworks which don't offer a shutdown hook, as validators would be released as required. All in all, to me the simplified contract between CVF and BV provider by removing releaseInstance() seems like the better option.

If we go down that route, we still could keep ValidatorFactory#close(). Its postcondition would be that the VF keeps no references to any objects passed into the BV runtime (be it message interpolator, validators etc.). An integration framework could thus safely dispose all such objects after VF#close() was called.

Note that this is not to say that releaseInstance() doesn't work, I just think it is not really required and possibly would not really be leveraged by BV providers. In that case we could keep the API simpler by omitting it.

--Gunnar

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@lists.jboss.org
> https://lists.jboss.org/mailman/listinfo/beanvalidation-dev

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