[hibernate-dev] Centralized access to "bootstrap only" resources

[Steve Ebersole]

On Tue, Aug 30, 2016 at 6:27 AM Sanne Grinovero <sanne at hibernate.org> wrote:

> On 30 August 2016 at 10:09, Emmanuel Bernard <emmanuel at hibernate.org>
> wrote:
> > I am not sure if that is still relevant but in the past, either HSEARCH
> > or HV were keeping the ReflectionManager around to use it at runtime
> > (either because metadata was loaded lazily or because of a reboot of the
> > factories due to a configuration change.
> >
> > So we need to check that losing access to ReflectionManager after SF is
> > created won't be problematic for these projects.
>
> In the "dynamic reconfiguration" case we create our own
> ReflectionManager instance:
>  -
> https://github.com/hibernate/hibernate-search/blob/fd4acb5d8f396201f5dccc89ba3cbc07becea08a/engine/src/main/java/org/hibernate/search/engine/impl/IncrementalSearchConfiguration.java#L26-L35


Interesting that y'all do not specify classloading behavior there (the
ClassLoaderDelegate stuff I added to HCANN)...



> Steve, we had a similar notion of "boot only available components" in
> Search but over time we started to have various "special needs" of
> various other components holding a reference on these.
> When I later tried to re-instate order, it was too late and we got in
> arguments like the API's intent not having been clear enough and too
> much entanglement had happened.
>

Hard to say without specifics. I hate "general rules" :)

So let's look at the specifics in terms of things I have moved to
BootstrapContext...


   1. HCANN ReflectionManager - as you said, y'all create your own for your
   use case.  You'd own the lifecycle of that one you create.  I see no
   conflict there.   Also we know that in 7.0 HCANN use will go away and we
   will move to Jandex.  The Jandex IndexView reference is only valid for a
   limited period of time when WF hands it to us.
   2.  JPA "temp ClassLoader" - I think this one is self-evident.  JPA
   states that this ClassLoader (if one) is available for only a limited time.
   3. ClassmateContext - I centralized this so that we did not have to keep
   "priming" the classmate caches each time we needed to use classmate.  Aside
   from a possible performance hit, there really is nothing special here
   versus creating a new ClassmateContext each time you need it.  For ORM we
   currently never use classmate outside of bootstrap.  Could that change?
   Maybe, and we'd deal with that if/when it does.
   4. scanning components
   (ArchiveDescriptorFactory, ScanOptions, ScanEnvironment, Scanner) - maybe
   going back to your "dynamic reconfiguration" scenario this makes sense.  No
   idea.  But in ORM holding on to these after bootstrap makes no sense.
   5. I've also started making BootstrapContext the holder for bootstrap
   metadata-related collectors.  Here we collect
   SQLFunctions, AuxiliaryDatabaseObjects, AttributeConverterDefinitions,
   and CacheRegionDefinitions.
   6. There are 2 other (new in 6.0) delegates that I keep here too.
   Interestingly, one is fully intended to be held beyond bootstrap.  But I
   think that these intentions just need to be documented.


Overall I'd view a "dynamic reconfiguration" scenario very much like a
limite bootstrap scenario.  Personally I'd expect to have to maker many of
these "boot only resources" available to that process.  Not necessarily the
same ones as used during the primary bootstrap though.  I personally would
prefer to not hold reference to these "just in case" we have a "dynamic
reconfiguration" situation later; I'd just rebuild them.  Granted things
like a WF-handed Jandex IndexView would be difficult to handle in there,
but that is the case regardless of whether we hold reference to it or not;
that has to do with WF eventually invalidating that reference it handed us.


So while I think it's a good idea, and also Search should try this
> again, I think we'd need to design it from day 1 to be defensive
> against future code attempting to hold on these services.
> Not sure what would be the best approach for ORM, but I guess that
> simply invalidating/closing these components after bootstrap and
> having these throw an exception after that would be a good start.
>

That is roughly what I do.  There is a BootstrapContext#release method.  It
in turn releases the delegates it holds.  I can add some defensive checking
for throwing some "unavailable" exceptions in case stuff holds references
to these.  That's a good idea.


However, please allow some flexibility for the case in which someone
> really needs one of the services you're dooming at runtime.
> For example Search might need to re-read configuration properties at
> runtime; we can of course make a copy, but then we'd need a way to be
> able to make such a copy (We currently actually make such a copy of
> the cfg Properties).
> Configuration properties being just an example, maybe we need a
> generic way to be able to declare which services should not be cleaned
> up after bootstrap?
>

We already hold on to configuration properties into the SF.  See
ConfigurationService.



> In practice, the services you've listed should be fine today but the
> need for us to make a copy (or to invoke some API to ask for a life
> extension) might show up in future.
>
> Rough proposal :
>
> interface BootService {
>   void flagForUsageBeyondBootstrap();
> }
>

-1 I think the BootstrapContext is not the right place for this.  It is not
the BootstrapContext itself that needs to remain valid, it is the delegates
it exposes.  That is where the "extension" should be allowed.  If that is
voted as generally worthwhile, I can see 2 options:

   1. Expose #allowExtendedAccess (or somesuch method name) to the actual
   delegates.  This would be an indicator to not release its resources when
   the BootstrapContext#release method tells the delegate to release itself.
   2. Allow OGM, Search, etc to specify specific impls for these
   delegates.  It could handle the delegate's #release method however it
   wanted.

However, realize that if these things are not released by
BootstrapContext#release then ORM washes its hands of cleaning them up (it
would have no "scope" to do that).