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

[Gunnar Morling]

2012/12/4 Emmanuel Bernard <emmanuel at 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 at lists.jboss.org
> > https://lists.jboss.org/mailman/listinfo/beanvalidation-dev
>
> _______________________________________________
> beanvalidation-dev mailing list
> beanvalidation-dev at lists.jboss.org
> https://lists.jboss.org/mailman/listinfo/beanvalidation-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.jboss.org/pipermail/beanvalidation-dev/attachments/20121210/ba6d57e7/attachment-0001.html