Fallback isolated ClassLoader to platform classes

[paul-hristea]

Solves #338

[mgaffigan]

Can you post repro steps for testing purposes?

[github-actions]

Test Results

128 files  +1  128 suites  +1   4m 2s ⏱️ + 1m 6s
729 tests +2  729 ✅ +2  0 💤 ±0  0 ❌ ±0 
819 runs  +2  813 ✅ +2  6 💤 ±0  0 ❌ ±0 

Results for commit dab99a6. ± Comparison against base commit a4a4183.

♻️ This comment has been updated with latest results.

[paul-hristea]

Sure thing @mgaffigan . I've edited it into the issue description (#338) for better visibility.

[jonbartels]

Would this affect cases where a user has classes in a resource directory that includes libraries that do conflict with the parent classloader?

I reviewed this issue by reading the docs for how the URLClassloader works for both old and new versions of Java:

• https://docs.oracle.com/javase/8/docs/api/java/net/URLClassLoader.html
• https://docs.oracle.com/en/java/javase/26/docs/api/java.base/java/net/URLClassLoader.html

Since the behaviour hasn't changed in 18 major versions I think we can safely assume that it won't break anything in the future :D

I read the references in the PR description.

This change is logical and correct. If a driver is loaded, that driver will have dependencies in the system classpath. Therefore including the system classpath is appropriate. HOWEVER see my question above. What happens if a user needs the classloader specifically to override something on the system classpath? Is this common enough to worry about?

[paul-hristea]

Hi @jonbartels, thanks for taking time to review!

I believe that this is why the "Load Parent-First" option was introduced - it allows us to choose whether each Resource is meant to take priority during class loading. The cases you ask to consider should be solvable by making use of this checkbox - although they should be uncommon since the move from Java 8 began quite recently.

[tonygermano]

I believe this is the correct change to make so that it behaves similarly to how it ran under java 8. The change itself would have been difficult to make prior to bumping the minimum version to java 17 because the method did not exist in java 8; it would have needed to test if it existed and call it by reflection.

I tested using the driver from https://h2database.com/html/main.html

• added the driver as a resource (did not check load parent first)
• added a database reader channel
• added the driver resource to the channel
• specified the driver as org.h2.Driver
• specified the jdbc uri as jdbc:h2:./appdata/h2.db
• clicked the Generate: Select button above the SQL section
• clicked the Get Tables button

Under the main branch, this threw a ClassNotFound Exception for java.sql.Driver.

With the fix from this branch

• no error is thrown
• no tables are returned (I never created any)
• a non-empty database file is created at the location specified in the appdata folder

[tonygermano]

What happens if a user needs the classloader specifically to override something on the system classpath? Is this common enough to worry about?

@jonbartels The platformClassLoader contains classes provided by the JRE which were just always included under java 8, but are no longer considered "core" classes since the move to the module system.

The systemClassLoader is the one that includes the classpath given when the JVM starts.

The platformClassLoader is an ancestor of the systemClassLoader.

[jonbartels]

My quesitons were addressed. LGTM!

[tonygermano]

@jonbartels I did some additional testing. This is still not working 100% as I would expect, but it is better than it was before, and I don't believe the unexpected behavior is related to this PR, so I am not retracting my approval.

I made a channel with a database/js writer containing:

var dbConn;

try {
	dbConn = DatabaseConnectionFactory.createDatabaseConnection('org.postgresql.Driver','jdbc:postgresql://localhost:5432/postgres','postgres','test');
	return dbConn.connection.metaData.driverVersion

} finally {
	if (dbConn) { 
		dbConn.close();
	}
}

• The response was 42.7.12, which is the correct driver version in the main branch.
• I added the driver for 42.7.13 as a resource and assigned it to the channel, and future messages contained the updated driver version.
• Removing the resource reverted back to driver 42.7.12

So far, this is all as it should be, but the part that was unexpected was that the 42.7.13 driver was still active when "load parent first" was chosen on the resource. I suspect it may have something to do with how DriverManager works.

[pacmano1]

Two asks before merge:

1. userutil/ContextFactory.getIsolatedClassLoader() javadoc still says "no parent classloader", twice. Worth stating the real rule while in there: parent-first over the platform loader, and Load Parent-First never applies to this loader.

2. A pinning test. The live repros can't tell the platform parent from a broader one; a loader parented on the application classloader would still pass them while handing the isolated loader the whole server classpath. Scaffolding exists in DatabaseReceiverInvalidColumnNameTests:

isolated.loadClass("java.sql.Driver"); // CNFE without this fix
assertThrows(ClassNotFoundException.class,
    () -> isolated.loadClass("com.mirth.connect.server.util.javascript.MirthContextFactory"));

Pre-existing issues found while reviewing (unclosed loaders on resource reload, NoClassDefFoundError escaping the fallback catches, ChildFirstURLClassLoader not parallel-capable) will be filed separately.

Method note, because I'd like to see more AI-assisted review here: this ran through Claude Code on Fable 5 with ultracode on, using my irritable-developer-check skill, a grumpy Java/Mirth review checklist authored, in every way that matters, by all of you. Plus a ponytail pass for over-engineering and a security review, with every finding adversarially re-verified against the code before posting. Two findings didn't survive that verification and were dropped, which is rather the point. Happy to share the setup with anyone interested.

[paul-hristea]

In response to @pacmano1:

1. userutil/ContextFactory.getIsolatedClassLoader() javadoc still says "no parent classloader", twice. Worth stating the real rule while in there: parent-first over the platform loader, and Load Parent-First never applies to this loader.

Updated the Javadoc. Please review.

2. A pinning test. The live repros can't tell the platform parent from a broader one; a loader parented on the application classloader would still pass them while handing the isolated loader the whole server classpath. Scaffolding exists in DatabaseReceiverInvalidColumnNameTests:

In progress...

@tonygermano so it appears that Load Parent-First actually does not apply to the isolated classloader which is used when Resources are attached to the channel. This would explain why you are seeing 42.7.13 regardless of that option.
I would suggest repeating the test while also having an additional Resource attached to the channel. This Resource may contain any arbitrary library, and is added simply to force instantiation of the isolated ClassLoader, as opposed to the channel defaulting to Driver behavior when no Resources are selected.

[pacmano1]

Reviewing 7e24227 as requested.

Javadoc: one real error, System.getPlatformClassLoader() doesn't exist. The method is on ClassLoader. While fixing that, suggested wording that explains the behavior instead of just naming the parent, since this package is the published User API and this method is exactly where the confusion in this thread lived:

/**
 * Returns a classloader containing only the libraries in the custom resources assigned to the
 * current context. Use it to load classes from those libraries in isolation, for example a
 * specific JDBC driver version, without interference from the versions the server itself ships.
 * <p>
 * Core Java classes (for example {@code java.sql} or {@code javax.xml}) are visible through this
 * classloader, but classes from the server or its plugins are not (its parent is
 * {@link ClassLoader#getPlatformClassLoader()}). A class that exists in both a custom resource
 * and the JDK resolves to the JDK's copy. The "Load Parent-First" option on a resource does not
 * affect this classloader; it applies only to the classloader returned by {@link #getClassLoader()}.
 *
 * @return A classloader containing only the custom resource libraries, or null if the current
 *         context has no custom resources.
 */

@paul-hristea for the in-progress test, here's one you're welcome to take as-is: server/src/test/java/com/mirth/connect/server/util/javascript/MirthContextFactoryTest.java. Before suggesting it I ran it against every parent someone might plausibly put on that line, on this branch:

MirthContextFactory.java:117 parent	./gradlew :server:test --tests '*MirthContextFactoryTest'
ClassLoader.getPlatformClassLoader() (this PR)	passes, 2/2
null (the #338 regression)	fails, ClassNotFoundException on java.sql.Driver
Thread.currentThread().getContextClassLoader()	fails, assertThrows catches the leaked server class
ClassLoader.getSystemClassLoader()	fails, same

So it accepts exactly the parent this PR introduces and rejects the three wrong ones, including the two that the H2/Postgres live tests can't distinguish from correct. Reproduce by swapping the parent on line 117 and rerunning.

// SPDX-License-Identifier: MPL-2.0
// SPDX-FileCopyrightText: Open Integration Engine

package com.mirth.connect.server.util.javascript;

import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertNull;
import static org.junit.Assert.assertThrows;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;

import java.io.File;
import java.net.URL;
import java.util.HashSet;

import org.junit.BeforeClass;
import org.junit.Test;

import com.google.inject.AbstractModule;
import com.google.inject.Guice;
import com.google.inject.Injector;
import com.mirth.connect.server.builders.JavaScriptBuilder;
import com.mirth.connect.server.controllers.CodeTemplateController;
import com.mirth.connect.server.controllers.ConfigurationController;
import com.mirth.connect.server.controllers.ControllerFactory;
import com.mirth.connect.server.controllers.EventController;
import com.mirth.connect.server.controllers.ExtensionController;

public class MirthContextFactoryTest {

    @BeforeClass
    public static void setUpBeforeClass() {
        // Same mocked ControllerFactory pattern as JavaScriptUtilTest, so this class is
        // self-sufficient regardless of which test classes ran (and injected) before it.
        ControllerFactory controllerFactory = mock(ControllerFactory.class);

        EventController eventController = mock(EventController.class);
        when(controllerFactory.createEventController()).thenReturn(eventController);

        ConfigurationController configurationController = mock(ConfigurationController.class);
        when(controllerFactory.createConfigurationController()).thenReturn(configurationController);

        ExtensionController extensionController = mock(ExtensionController.class);
        when(controllerFactory.createExtensionController()).thenReturn(extensionController);

        CodeTemplateController codeTemplateController = mock(CodeTemplateController.class);
        when(controllerFactory.createCodeTemplateController()).thenReturn(codeTemplateController);

        Injector injector = Guice.createInjector(new AbstractModule() {
            @Override
            protected void configure() {
                requestStaticInjection(ControllerFactory.class);
                bind(ControllerFactory.class).toInstance(controllerFactory);
            }
        });
        injector.getInstance(ControllerFactory.class);

        JavaScriptBuilder.setControllersForTesting(extensionController, codeTemplateController);
    }

    /*
     * Regression test for #338: with a null parent, the isolated classloader cannot see java.sql
     * on Java 9+, so custom driver resources failed to deploy. The parent must be the platform
     * classloader: JRE classes visible, server classpath not.
     */
    @Test
    public void isolatedClassLoaderResolvesPlatformButNotServerClasses() throws Exception {
        URL dummyJar = new File("build/tmp/mirth-context-factory-test-dummy.jar").toURI().toURL();
        MirthContextFactory contextFactory = new MirthContextFactory(new URL[] { dummyJar }, new HashSet<>(), false);

        ClassLoader isolated = contextFactory.getIsolatedClassLoader();
        assertNotNull(isolated);

        // Fails with ClassNotFoundException if the parent ever goes back to null
        isolated.loadClass("java.sql.Driver");

        // Fails if the parent is ever widened to a loader that can see the server classpath
        assertThrows(ClassNotFoundException.class, () -> isolated.loadClass(MirthContextFactory.class.getName()));
    }

    @Test
    public void isolatedClassLoaderIsNullWithoutResources() {
        MirthContextFactory contextFactory = new MirthContextFactory(new URL[0], new HashSet<>(), false);
        assertNull(contextFactory.getIsolatedClassLoader());
    }
}

The dummy jar URL never gets read; it only exists because getIsolatedClassLoader() returns null on an empty URL array. Same AI-assisted setup as my earlier comment: the test was run against all four parents above, and a clean full build with this class added is green (665 tests, 0 failures).

[paul-hristea]

Apologies for the oversight.

I have applied the changes. I can confirm the test results.

Thank you @pacmano1 for your guidance.

[mgaffigan]

@paul-hristea, you do not need to keep this branch up to date with main unless there is a conflict. I think reviews are waiting on settlement of @tonygermano's issue. Everyone else has approved once already.

[tonygermano]

@paul-hristea, you do not need to keep this branch up to date with main unless there is a conflict. I think reviews are waiting on settlement of @tonygermano's issue. Everyone else has approved once already.

Issue is settled, and I've approved. Thanks for working on this Paul!

[NicoPiel]

non-blocking:

1. ContextFactory.java: four of the new Javadoc lines have trailing whitespace ("packages) ", "as its ", "platform's copy. ", "classloader; "). The @return line also looks longer than the file's ~100-column wrap.
2. Test, dummyJar: the relative path build/tmp/... depends on the working directory. It's never read, so it does no harm, but new URL("file:///nonexistent.jar") or a TemporaryFolder would make that clearer.
3. Test: the URLClassLoader is never closed. This is trivial.
4. Test setup: it rebinds ControllerFactory statically, which is global state. It copies the existing JavaScriptUtilTest pattern, so that's acceptable, but it's worth knowing about if tests start running in parallel.

[pacmano1]

Following up on the three pre-existing issues I said I'd file separately. I tested each on main (2085b4c) first, and none turns out to be a user-facing bug, so I'm not filing them:

• Unclosed classloaders on resource reload: 30 reloads of a resource in use by a channel. Each reload makes new loaders, the old ones get collected (one live ChildFirstURLClassLoader after GC), and open handles on the jar stay at 1. No leak.
• NoClassDefFoundError escaping the CustomDriver fallbacks: tried with a resource holding derbyclient 10.15.2.0 without derbyshared. The Database Reader fails to deploy with an error naming the missing class, which is the useful outcome; rethrowing as ClassNotFoundException would hide it. A script's try/catch around createDatabaseConnection can't catch it, but that alone isn't worth an issue.
• ChildFirstURLClassLoader not parallel-capable: concurrent loadClass on one instance does fail (a harness got "duplicate class definition" on 4,500 of 4,800 loads, none with a plain URLClassLoader), but nothing in the engine calls it that way. Rhino's package lookup is synchronized and Class.forName holds the loader lock. 16 concurrent scripts over 10 fresh loaders gave no errors.