Add more flexibility to Class Loading

[stuartwdouglas]

This changes from a fixed ClassLoader per context, to a per-context selectable TCCL.

Motivation: When Quarkus is running in dev mode it needs the ability to change the current ClassLoader on hot replacement. This changes the internal ClassLoader fields to Supplier<ClassLoader>, and provides the ability to select a default ClassLoader.

[stuartwdouglas]

cc @Sanne @cescoffier

[stuartwdouglas]

I have no real familiarity with this code, so it really needs someone who knows it to look over this.

[Sanne]

+1 for the approach. I'm also not familiar with the details so I'll stay out of them :)

[zero88]

IMHO, this approach will not be acceptable. Because:

• you add Supplier into DeploymentOptions, it is data object and should be serializable and jsonable
• change VertxInternal API, might affect to developer.
• no test

And actually, I don't get a idea why need to provide supplier instead of concrete value. I'm not yet familiar with hot reload tech-inside, but I can imagine it will have an event to notify and you completely get exact classloader in this time and redeploy again.

If my assumption is wrong, I think the elegant way is create ClassLoaderProvider SPI. So by default,

interface ClassLoaderProvider {
  default ClassLoader get() {
     return Thread.currentThread().getContextClassLoader();
  } 
}

but need to ensure whenever use tccl, whenever your provider

[cescoffier]

An SPI requires that we can access the classloader using static methods, so not great either. Plus, the code you show gets it from the TCCL, which is the TCCL calling the supplier, and it's not what we need.

However, we may be able to work with that if Vert.x calls the provider's get every time the PR calls the supplier.

[cescoffier]

About Data Object - wasn't that limitation lifted? I don't remember it had to be serializable BTW, json yes (using converters most of the time).

[Sanne]

yeah having a static mutable global state would be quite horrible; the goal here being to cleanup some hacks, not to add more :)

[stuartwdouglas]

ClassLoader is not serializable, and it literally the field above what I have added into DeploymentOptions. If this is a problem though I can remove it, as we mainly need the ability to set it per vert.x instance (not per deployment).

The VertxInternal chances are backwards compatible, unless you are actually implementing the interface (i.e. you have your own Vertx impl). The old methods are still there, they have been defaulted to delegate to the new ones.

I don't want to spend time writing tests until I know that the overall approach is acceptable.

[zero88]

Might be I'm not clear myself, but actually my first implementation will be:

interface ClassLoaderProvider {
  ClassLoader get();
}

In VertxImpl

@Override
  public EventLoopContext createEventLoopContext() {
    ClassLoader cl = Optional.ofNullable(ServiceHelper.loadFactoryOrNull(ClassLoaderProvider.class)).map(ClassLoaderProvider::get).orElseGet(()->Thread.currentThread().getContextClassLoader())
    return createEventLoopContext(null, closeFuture, null, cl);
  }

You can extract ServiceHelper.loadFactoryOrNull(ClassLoaderProvider.class) to another class and cache it ás singleton if needed.

As your comment:

as we mainly need the ability to set it per vert.x instance (not per deployment)

per vert.x instance should use VertxOptions per deployment verticle should use DeploymentOptions

And coz you said want to each Vertx context, so instead of using supplier(dont like this coz the supplier is passed via several method and invoked without context, that doesn't make sense), then I think ClassLoaderProvider should be pass something else depends on your hot reload context, such as

interface ClassLoaderProvider {
  ClassLoader get(Vertx vertx);
}

However, I'm not sure detail about how hot reload work in Quarkus? track file changes, and init new Vertx object then kill the old one? might be VertxServiceProvider can help.

I think we should ask @vietj to get more detail.

[vietj]

I believe that we should make ContextImpl overridable (using an SPI like ContextFactory) and then Quarkus can extend this class and provide a specific implementation of the classLoader() method.

[pmlopes]

Just a heads-up. Can you verify that this change will work with the vertx file system implementation? The FS resolves resources from the classpath, but if it changes you will need to invalidate caches as resources will probably also change.

[stuartwdouglas]

@pmlopes good point. I will need to think about how to handle that.

Maybe Supplier<ClassLoader> is not the correct approach and we should have some way of notifying vert.x when the ClassLoader changes.

[stuartwdouglas]

I have updated this, to also allow for registering listeners that are notified on ClassLoader change.