Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ClassLoaders not controlled by update4j #75

Closed
persal opened this issue Dec 11, 2019 · 28 comments
Closed

ClassLoaders not controlled by update4j #75

persal opened this issue Dec 11, 2019 · 28 comments

Comments

@persal
Copy link

@persal persal commented Dec 11, 2019

We have update4j up and running and everything works nicely beside some classloader issues.
Currently these 3pps does not work without some kind of workaround:

  • Spring
  • XStream
  • Koloboke

Before loading the spring application context, which basically instantiate classes, we have to set the current classloader on the thread like this to workaround NoClassDefs:

Thread.currentThread().setContextClassLoader(tmpClassLoader);

the tmpClassLoader in the above case is taken from an already loaded class.

For XStream we have to do something similar, take the classloader from an already loaded class like this:

XStream xStream = new XStream();
xStream.setClassLoader(XStreamFactory.class.getClassLoader());

For Koloboke we haven't figured out how to handle it.

If we look inside the koloboke code we see this:

private static class DefaultFactoryHolder {
	private static final HashIntObjMapFactory defaultFactory =
			ServiceLoader.load(HashIntObjMapFactory.class).iterator().next();
}

and when we look at JDK 11s ServiceLoader.load method, we see that the classloader is fetched like this:

@CallerSensitive
public static <S> ServiceLoader<S> load(Class<S> service) {
	ClassLoader cl = Thread.currentThread().getContextClassLoader();
	return new ServiceLoader(Reflection.getCallerClass(), service, cl);
}

Digger deeper, if the current threads context classloader is null the ServiceLoader will use the system classloader:

if (cl == null) {
	cl = ClassLoader.getSystemClassLoader();
}

All issues essentially have the same problem, getting the context classloader is either null or returns a classloader that is not controlled by update4j.

There must be a more generic way to do this. Can we configure update4j to make sure all returned classloaders are "valid" in the sense that they are controlled by update4j?
Can we force each new thread to have update4js classloader when invoking:

Thread.currentThread().getContextClassLoader()

We are using update4j 1.44, AdoptOpenJDK 11.0.5
On the boot classpath we have 2 jar files: update4j and our custom boot lib, then we get the rest using the config.

If we run without update4j (using a common classpath) everythings works.

@mordechaim

This comment has been minimized.

Copy link
Contributor

@mordechaim mordechaim commented Dec 11, 2019

I'm out of town for the next few days, I'll investigate on Monday.

@persal

This comment has been minimized.

Copy link
Author

@persal persal commented Dec 11, 2019

Appreciate you coming back so quickly.

We have our own implementations of Launcher, Delegate and UpdateHandler and from the UpdateHandler we will show a progress dialog (awt/swing) when there are changes to download.
In some cases we show a dialog (awt/swing) from the Delegate implementation as well.

I found that when not displaying any awt/swing components before getting to the Launcher we seem to get the right classloader. Tried withouth workaround for Spring and Koloboke and it seem to start as expected.

So I looked at the java.awt.EventQueue implementation and found that it actually initiates a final classLoader variable from current thread which is then kept throughout the applications lifetime. I assume the Delegate or UpdateHandler is running with the boot classloader and therefore the EventQueue will have a reference to the boot class loader instead of the "application classloader" when the Launcher starts the application.

Possible something like this before calling the Delegate or UpdateHandler solves the issue or atleast some of the scenarios.

//somewhere in update4j before loading Delegate or UpdateHandler
ClassLoader contextClassLoader = Thread.currentThread().getContextClassLoader();
try {
	Thread.currentThread().setContextClassLoader(getUpdate4jClassloader());
	//get the event queue to initialize the classLoader variable
	Toolkit.getDefaultToolkit().getSystemEventQueue();
}finally {
	Thread.currentThread().setContextClassLoader(contextClassLoader);
}

Possibly skipping the finally block and always have the context classloader set to application context, or at least making it configurable.

We start everything with mainClass: org.update4j.Bootstrap. Is it currently possible to inject the code above somewhere in the current version, without modifying the core code?
Or is it possible to access the application classloader reference somehow in the Delegate or UpdateHandler?

@mordechaim

This comment has been minimized.

Copy link
Contributor

@mordechaim mordechaim commented Dec 12, 2019

Or is it possible to access the application classloader reference somehow in the Delegate or UpdateHandler?

Yes, with quite a hack.

Firstly, the classloader only gets created when you call config.launch(), you can fish out the classloader into the bootstrap and run your code just before finally doing the launch by adding a @PostInject method in the bootstrap with the first parameter accepting the launcher type (perhaps some common interface that the launcher implements returning the classloader from LaunchContext with the interface defined in the bootstrap).

You can do now anything in the bootstrap, effectively halting the launch until the post inject method returns. Since you now have the launcher reference, you might as well terminate the launch by marking something that the launcher knows to process accordingly.

@persal

This comment has been minimized.

Copy link
Author

@persal persal commented Dec 13, 2019

I think the best solution would be for update4j to provide a classloader from the start that is somewhat context aware where it has 2 delegates.
The first delegate would be the current boot classloader and the other would be the "update4j application" classloader that is created at a later stage.
This way we can be sure that any reference early caused by things done in Delegate or UpdateHandler can be controlled by update4j.

Once update4j creates its own classloader it should then be made available to the "context aware update4j" classloader so that the reference stays the same, but internally it will then use update4j classloader when it has become available.

@persal

This comment has been minimized.

Copy link
Author

@persal persal commented Dec 15, 2019

I got it working with below code. Let me know if you think there are cases where this won't work.
The important part is the setDelegate where it is switched out in the launcher, before starting the application.
Don't really like having to rely on reflection here, but don't see any other solution to it.

public class ClientDelegate implements Delegate {
    static {
        DelegatingClassLoaderHolder.initDelegate();
    }
    ...
}
public class ClientUpdateHandler implements UpdateHandler {
    static {
        DelegatingClassLoaderHolder.initDelegate();
    }

    ...
}
public class ClientLauncher implements Launcher {

    ....
    
    @Override
    public void run(LaunchContext context) {
        setDelegatingClassLoader(context.getClassLoader());
        ....
    }

    private void setDelegatingClassLoader(ClassLoader classLoader) {
        try {
            // change delegate to update4j controlled classloader
            Class<?> delegatingClassLoaderholderClazz = Class.forName(CLASSLOADER_HOLDER_CLASS);
            Method delegatingMethod = delegatingClassLoaderholderClazz.getMethod(CLASSLOADER_HOLDER_METHOD, ClassLoader.class);
            delegatingMethod.invoke(delegatingClassLoaderholderClazz, classLoader);
        } catch (Exception e) {
            bootlogger.log(Level.SEVERE, e, () -> "Unable to set delegating classloader: " + e.getMessage());
        }
    }
    
    ....
}
//class available from boot classpath
public class DelegatingClassLoader extends ClassLoader {

    private Logger logger = BootLogger.getLogger(DelegatingClassLoader.class.getName());
    private volatile ClassLoader delegate;

    public void setDelegate(ClassLoader delegate) {
        logger.info(() -> "Delegate classloader " + delegate);
        this.delegate = delegate;
    }

    private ClassLoader resolveClassLoader() {
        return delegate;
    }

    @Override
    public String getName() {
        if (delegate == null) {
            return getClass().getName() + this; // must always return a name to build identity
        }
        return resolveClassLoader().getName();
    }   
    
    @Override
    public Class<?> loadClass(String name) throws ClassNotFoundException {
        return resolveClassLoader().loadClass(name);
    }
	....
    //override all other public methods
}
//class available from boot classpath
public class DelegatingClassLoaderHolder {

    private static final Logger logger = BootLogger.getLogger(DelegatingClassLoaderHolder.class.getName());
    private static volatile DelegatingClassLoader delegatingClassLoader;

    public static synchronized void initDelegate() {
        if (delegatingClassLoader == null) {
            final ClassLoader currentContextClassLoader = Thread.currentThread().getContextClassLoader();
            try {
                delegatingClassLoader = new DelegatingClassLoader();
                delegatingClassLoader.setDelegate(currentContextClassLoader);
                Thread.currentThread().setContextClassLoader(delegatingClassLoader);
                initContextClassLoaderReferences();
            } finally {
                Thread.currentThread().setContextClassLoader(currentContextClassLoader);
            }
        }
    }

    /**
     * Init references to contextclassloader that is kept throughout application lifetime
     */
    private static void initContextClassLoaderReferences() {
        try {
            Toolkit.getDefaultToolkit().getSystemEventQueue();
        } catch (Exception e) {
            logger.log(Level.SEVERE, e, () -> "Unable to init EventQueue " + e.getMessage());
        }
    }

    public static void setDelegate(ClassLoader delegate) {
        if (delegatingClassLoader != null) {
            delegatingClassLoader.setDelegate(delegate);
        }
    }
}
@mordechaim

This comment has been minimized.

Copy link
Contributor

@mordechaim mordechaim commented Dec 16, 2019

As I said, I'll try to get to this today; I'm quite busy but will try my best.

As a side note, if it fits your needs you might make the delegate just start a new JVM instance (perhaps by running a launcher script, which is itself updated by update4j) instead of extending the running instance. Don't call config.launch(), instead, place your classpath files in a location defined in the launcher script to be discovered by the JVM.

The tradeoff is more manual work on managing which files get loaded, e.g. for OS-specific files, no support for dependency injection, harder report-back from business to bootstrap when the app shuts down and a few more things.

@mordechaim

This comment has been minimized.

Copy link
Contributor

@mordechaim mordechaim commented Dec 16, 2019

There's so much going on in your last comment.

How do you get the launcher classloader in the delegate, isn't the launcher only created when you call launch(), not before?

Also, how do those multiple delegates interact? Don't you usually just use one delegate at any given time (with newer for newer versions)? Are you extending update4j to something I didn't even anticipate?

@persal

This comment has been minimized.

Copy link
Author

@persal persal commented Dec 16, 2019

I don't think starting a new JVM is an option really, not for us atleast.

I'll try to explain what is going on in my last comment. Just realized my bad naming here. Update4j has the Delegate interface which is not to confuse with the classloader delegates that I am referring to.

So when the Delegate (or UpdateHandler) is instantiated by update4j, we start by creating a custom classloader to make sure the EventQueue reference points to that classloader instead of the boot classloader, this way we can "redirect it" using the setDelegate method to use another classloader at a later stage. The custom classloader basically act as a proxy.
We only have 1 implementation of the Delegate interface and 1 implementation of the UpdateHandler interface.

DelegatingClassLoaderHolder creates the custom classloader and sets the current contextclassloader as its delegate. This way our custom classloader will be the reference that the EventQueue always references. However since we have a delegate method, we can change the delegate once update4j instantiates the Launcher and we have access to the classloader that we want to use for the aplication. Once the Launcher is instantiated, we have a reference to update4js classloader from the context.getClassLoader(). So we use context.getClassLoader() and replace the delegate classloader in our custom classloader. This way the EventQueue is "directed" to use update4js classloader going forward.

So, the swap of delegate classloader takes place after config.launch() is called. Since the Launcher is launched afterwards and it is from the Launcher we (with some not so nice stuff) find the reference from our custom classloader throught the DelegatingClassLoaderHolder class with its static reference.

Regarding the call to config.update(getPublicKey()); that we do from our Delegate implementation, is that not how it should be done, or did you mean in the case of using a separate JVM instance? We do never keep a local copy of the config file, but on every startup loads over http/https.

our last few lines in our implementation of the Delegate interface looks like this:

        if (config.requiresUpdate()) {
            config.update(getPublicKey());
        }

        config.launch();
@mordechaim

This comment has been minimized.

Copy link
Contributor

@mordechaim mordechaim commented Dec 16, 2019

Ah! that's an ingenious solution. So while it may work for your case I still want a uniform solution to this issue which you happen not to be the first to complain about. Let me read up on the new classloading restrictions since JDK 9 and try to find a solution.

Regarding the call to config.update(getPublicKey());

Dang, that was a typo, I fixed it already.

@mordechaim

This comment has been minimized.

Copy link
Contributor

@mordechaim mordechaim commented Dec 16, 2019

Do you also have issues if you business app is running in the module-path instead, by setting modulepath=true in config?

@persal

This comment has been minimized.

Copy link
Author

@persal persal commented Dec 16, 2019

Sure, can definitely understand you want a generic solution that cover all cases including modules. We are good for now with above solution, but is looking forward to a future release, solving the above :)

Regarding modulepath=true, not sure I can try that tomorrow.

On our boot classpath we have 2 jar files, and after updating the client we have about 180 jars in the update4j classpath.
Our config looks like this:

<?xml version="1.1" encoding="UTF-8" standalone="yes"?>

<!-- Generated by update4j. Licensed under Apache Software License 2.0 -->
<configuration timestamp="2019-12-16T14:24:45.476327Z" signature="TE7fex9xO2Offqj........mN2GE4zLyYpZ5P2LoZrH+HQ==">
    <base uri="http://localhost:8081/" path="${user.dir}"/>
    <properties>
        <property key="default.launcher.main.class" value="....."/>
        .....
    </properties>
    <files>
        <file uri="lib/client/...-1.2.16.jar" 
			  path="runtime/lib/cache/...-1.2.16.jar" 
			  size="317195" 
			  checksum="2022fa13" 
			  classpath="true" 
			  ignoreBootConflict="true" 
			  signature="poJN90kQJbnJA...........H8enVAObA=="/>

    .....

</configuration>
@mordechaim

This comment has been minimized.

Copy link
Contributor

@mordechaim mordechaim commented Dec 16, 2019

Thanks, keep me posted please about module-path, I think I have a lead.

@mordechaim

This comment has been minimized.

Copy link
Contributor

@mordechaim mordechaim commented Dec 17, 2019

If you find it hard to do, just never mind.

@persal

This comment has been minimized.

Copy link
Author

@persal persal commented Dec 17, 2019

busy day haven't had time to look at it yet, but will look at it later this week

@mordechaim

This comment has been minimized.

Copy link
Contributor

@mordechaim mordechaim commented Dec 17, 2019

A little discovery: New threads always automatically captures its context classloader from the thread that started them. If you look at :

t.setContextClassLoader(contextClassLoader);

You'll see the dynamic classloader is set for the launch thread, therefore if you start— say—JavaFX solely in the business app they should pick up the dynamic classes flawlessly. The problem arises if we want to show an update screen, the thread gets started in the main thread and will not pick up those classes. That's why starting AWT (which starts the event queue) before launch causes issues, but not if is started afterward.

I think of creating a special classloader using your idea, called UnifiedClassLoader which is attached to a configuration object and automatically picks up the new classes for every launch. This makes it much easier to access dynamic classes in the bootstrap. You might assign this loader as the context classloader right at the beginning of your application and will incrementally gain access to all classes.

Implementation details:

  • All classes are resolved by the last launch first, so newer versions of the same class in subsequent launches should first show up.

  • Classloaders should be kept as weak references, so unless the business app explicitly references a bootstrap class the classes should be destroyed and garbage collected when the business app launch() returns (if the class references a bootstrap class, the classloader would always keep the boot class in its cache, preventing garbage collection). This enables a very powerful hot class-reloading mechanism, useful for server development.

Before this can be deployed this needs heavy integration testing, which in itself might hinder the release, as it requires much effort to set up and I'm busy myself. Best would be if you can try it out with your setup in place and removing your solutions for Spring, XStream, and Koloboke.

@persal

This comment has been minimized.

Copy link
Author

@persal persal commented Dec 17, 2019

Let me describe some more details in how we use update4j.
As described before we have a update splash when the UpdateHandler implementation is invoked, which is a simple AWT/swing dialog.

When we get to the Delegate implementation, we show kind of a selection dialog (AWT/swing) where the user can choose between several environments to launch. This is why we can't have this type of dialog in the Launcher implementation, since we do not know which launcher to start until the user have chosen connection point (and therefore chosen which config to load). This provide us with quite a powerful yet simple way of having multiple environments with sometimes different jar files and properties, from the same installation.

Your idea is to have the UnifiedClassloader to be the "current" classloader/contextclassloader already when initiating the UpdateHandler and Delegate I assume, therefore taking care of the issue with EventQueue for example?

Having worked with many app servers before, reloading classes can be useful but what I have found, very tricky sometimes. Definitely interesting stuff.

I can absolutely help out with testing your ideas with our setup.

@mordechaim

This comment has been minimized.

Copy link
Contributor

@mordechaim mordechaim commented Dec 17, 2019

Your idea is to have the UnifiedClassloader to be the "current" classloader/contextclassloader already when initiating the UpdateHandler and Delegate I assume, therefore taking care of the issue with EventQueue for example?

Precisely, set the unified classloader as the context cl in the first line of code, then it will extend itself on each launch. Just like you did but more automatic.

@persal

This comment has been minimized.

Copy link
Author

@persal persal commented Dec 18, 2019

Nice! sounds like a good approach

@mordechaim

This comment has been minimized.

Copy link
Contributor

@mordechaim mordechaim commented Dec 19, 2019

So here's a little problem with your solution, I'm still pondering how to circumvent this.

Given that you set the delegate classloader as the context loader prior to launching, the new dynamic classloader created by update4j would pick up the delegate loader as its parent loader (all classloaders except the native boot classloader have a parent loader in case the class is not present in itself).

Let's assume we try to load a class (using ctx.getClassLoader().loadClass() or a dynamically loaded class uses that class in natural Java) and the class is only present in the bootstrap or not at all. The update4j loader would not find it and forward it to the parent. The parent receives the call to load the class, but immediately delegates it back to the update4j loader which in turn calls its parent, StackOverflowError.

I'm still thinking a way out, I'm open for ideas.

@persal

This comment has been minimized.

Copy link
Author

@persal persal commented Dec 19, 2019

The DelegatingClassLoader I create does not have a real parent in the sense that I specify it in the constructor. It does however have a parent in the sense that we delegate to the ClassLoader currently set by setDelegate(ClassLoader cl)

From the Delegate implementation the first classloader delegate is set as seen here:

INFO Delegate classloader jdk.internal.loader.ClassLoaders$AppClassLoader@77556fd

Later when the Launcher implementation starts we first of all change the delegate in DelegatingClassloader (to update4js URLClassLoader).

INFO Delegate classloader java.net.URLClassLoader@4df5bcb4

We then log each of the different classloader hierarchies, and from what I can see it looks correct.

# Hierarchy of getClass().getClassLoader()
INFO    Class         cl hierarchy: [classpath: java.net.URLClassLoader@4df5bcb4] [app: jdk.internal.loader.ClassLoaders$AppClassLoader@77556fd] [platform: jdk.internal.loader.ClassLoaders$PlatformClassLoader@3b6e121e]  

# Hierarchy of context.getClassLoader()
INFO    LaunchContext cl hierarchy: [classpath: java.net.URLClassLoader@4df5bcb4] [app: jdk.internal.loader.ClassLoaders$AppClassLoader@77556fd] [platform: jdk.internal.loader.ClassLoaders$PlatformClassLoader@3b6e121e]  

# Hierarchy of Thread.currentThread().getContextClassLoader():
INFO    Context       cl hierarchy: [classpath: java.net.URLClassLoader@4df5bcb4] [app: jdk.internal.loader.ClassLoaders$AppClassLoader@77556fd] [platform: jdk.internal.loader.ClassLoaders$PlatformClassLoader@3b6e121e]  

# EventQueue internal reference:
INFO    EventQueue    cl hierarchy: [classpath: delegating classloader java.net.URLClassLoader@4df5bcb4] [app: jdk.internal.loader.ClassLoaders$AppClassLoader@77556fd] [platform: jdk.internal.loader.ClassLoaders$PlatformClassLoader@3b6e121e]

Above the hierarchy is represented using getParent until null.

Looking at the EventQueue reference it references the DelegatingClassLoader which in turn references the update4j classloader which seem to have correct parent.

I found that specifying a parent using the constructor in DelegatingClassLoader only gave me trouble and StackOverFlows as well since the reference became part of the class loader hierarchy in more than 1 place.

Also, creating new instances from classes only available on the boot classpath from inside our application (started by Launcher) works. So I think this would indicate that classloading "back to" boot classpath works.

@mordechaim

This comment has been minimized.

Copy link
Contributor

@mordechaim mordechaim commented Dec 19, 2019

My issue is with update4j classloader parent. It looks like you were careful enough to reset back the context classloader after the event queue call. If the delegating cl is the context cl at launch-time, that's where the issue arises.

@mordechaim

This comment has been minimized.

Copy link
Contributor

@mordechaim mordechaim commented Dec 31, 2019

I ran into some technical difficulties. I'll get back to this issue when I have a chance.

@mordechaim

This comment has been minimized.

Copy link
Contributor

@mordechaim mordechaim commented Jan 12, 2020

A beautiful discovery, I can define a custom system class loader by passing a class name to the system property java.system.class.loader as explained here. This means I could potentially solve this whole issue in a much better way.

Let me do some more research and hope to get this done soon.

@mordechaim

This comment has been minimized.

@mordechaim mordechaim closed this Jan 13, 2020
@mordechaim mordechaim reopened this Jan 13, 2020
@persal

This comment has been minimized.

Copy link
Author

@persal persal commented Jan 13, 2020

Sounds good, looking forward to the new model.

@mordechaim

This comment has been minimized.

Copy link
Contributor

@mordechaim mordechaim commented Jan 14, 2020

I'm working on the other issues before releasing this. If you want, build from source yourself and either call:

Thread.currentThread().setContextClassLoader(new DynamicClassLoader());

in the first line of code in the bootstrap, or start the JVM with this flag:

java -Djava.system.class.loader=org.update4j.DynamicClassLoader

There is much more to this feature, which I still have to document in the wiki, but this should be a quick-fix without the need to understand much of the classloading internals.

@theotherp

This comment has been minimized.

Copy link

@theotherp theotherp commented Jan 15, 2020

Hey. I just found this issue by mistake. I stumbled over the same errors when I tried to implement update4j with a wrapper for a Spring Boot application. That ways two years ago. I dedicated nearly a week to it and then gave up.

I really appreciate your work here (both @mordechaim's on the framework and @persal's on this issue) and look forward to trying it out!

@garzy

This comment has been minimized.

Copy link

@garzy garzy commented Jan 19, 2020

It works like a charm!!. You save my life because I'm having a lot of classloading issues related to this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.