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

From what you guys say, all implementors of object lifecycle management

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

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

2012/12/4 Emmanuel Bernard <emmanuel(a)hibernate.org&gt; > 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.