About the special handling of Java 17 inside the `ModuleOpenerImpl` class

[dogourd]

In the Javaagent

public boolean openModuleTo(Instrumentation instrumentation, Class<?> classFromTargetModule, ClassLoader openTo, Collection<String> packagesToOpen) {
        Module targetModule = classFromTargetModule.getModule();
        Module openToModule = openTo.getUnnamedModule();
        Set<Module> openToModuleSet = Collections.singleton(openToModule);
        Map<String, Set<Module>> missingOpens = new HashMap<>();
        for (String packageName : packagesToOpen) {
            if (!targetModule.isOpen(packageName, openToModule)) {
                missingOpens.put(packageName, openToModuleSet);
            }
        }
        if (!missingOpens.isEmpty()) {
            if (instrumentation.isModifiableModule(targetModule)) {
                logger.debug("Opening packages {} from module {} for instrumentation to module {}", missingOpens.keySet(), targetModule, openToModule);
                try {
                    instrumentation.redefineModule(targetModule,
                        Collections.<Module>emptySet(), //reads
                        Collections.<String, Set<Module>>emptyMap(), //exports
                        missingOpens,  //opens
                        Collections.<Class<?>>emptySet(), //uses
                        Collections.<Class<?>, List<Class<?>>>emptyMap() //provides
                    );
                } catch (Exception e) {
                    if (JvmRuntimeInfo.ofCurrentVM().getMajorVersion() >= 17) {
                        logger.error("Failed to open packages {} from module {} for instrumentation due to exception", missingOpens.keySet(), targetModule, e);
                        return false;
                    } else {
                        logger.warn("Failed to open packages {} from module {} for instrumentation due to exception", missingOpens.keySet(), targetModule, e);
                    }
                }
            } else {
                if (JvmRuntimeInfo.ofCurrentVM().getMajorVersion() >= 17) {
                    logger.error("Cannot open packages {} from module {} for instrumentation because module cannot be redefined!", missingOpens.keySet(), targetModule);
                    return false;
                } else {
                    logger.error("annot open packages {} from module {} for instrumentation because module cannot be redefined!", missingOpens.keySet(), targetModule);
                }
            }
        }
        return true;
    }
}

if (JvmRuntimeInfo.ofCurrentVM().getMajorVersion() >= 17) {
     logger.error("Cannot open packages {} from module {} for instrumentation because module cannot be redefined!", missingOpens.keySet(), targetModule);
     return false;
}

The code above is contained in the ModuleOpenerImpl class. I'm a bit puzzled by the special handling for Java 17 and above. The code indicates that Java 17 and later versions will return false within some branches, while versions below Java 17 will not. Does anyone have any documentation or links that can explain what details in Java 17 lead to the need for this kind of behavior here? I tried searching previous PR information but didn't find any convincing explanation.

[Sylvain_Juge]

Hi @dogourd ,

I don't recall the low level technical details here, but this change was required when adding the LDAP instrumentation in https://github.com/elastic/apm-agent-java/pull/2977.

Sorry to not being able to provide you with more details, but the underlying reason of why this change was required and is only applied for Java 17+ JVMs probably belongs to JVM engineers and JPMS implementation details. Maybe @Jonas_Kunz remembers more about this than I do.

On you side, can you elaborate a bit why such low-level technical details matter to you ? Is there something that prevents you from using the agent relating this code ?

[dogourd]

Ah, this piece of code currently doesn't have any impact on me - I'm still in investigative mode. I also wanted to do some operations on JPMS in my own project, and then saw the encapsulation and implementation around this part in the elastic javaagent. When reading through this code, I came across some confusing points, because intuitively it looks quite strange (it's like saying versions below Java 17 can successfully process the module even if an exception occurs inside redefineModule). So what I want to know is whether this was an intentional behavior for Java 17 and above, or a minor mistake at the time. I'm just trying to get some details, because I didn't find any valid explanation about this part in the PR or source code comments.

[Jonas_Kunz]

@dogourd

it's like saying versions below Java 17 can successfully process the module even if an exception occurs inside redefineModule

That's exactly the case: strong module encapsulation was added in Java 9, but illegal accesses wouldn't fail, but cause a warning to be printed instead.
Please read the section here about the --illegal-access setting.

Turns out that illegal accesses were actually already denied in Java 16 and not only Java 17 according to that doc, so technically the code snippet here is not 100% correct.

[dogourd]

Thanks @Jonas_Kunz

I think I roughly understand what Elastic is trying to do here. During testing, I found some classes that could not be accessed via reflection even with --illegal-access=permit enabled. However, those classes are currently out of Elastic's scope of interest.

Just to supplement with some test code in case anyone is interested, it demonstrates that some classes cannot be properly accessed via reflection on Java 17 (or Java 16) and earlier versions.

public class OpenTest {
    static final Instrumentation inst = ByteBuddyAgent.install();

    public static void main(String[] args) throws Throwable {
        String[] classes = {
                "com.sun.java.accessibility.internal.ProviderImpl",      // jdk.accessibility/com.sun.java.accessibility.internal
                ClassLoader.getSystemClassLoader().getClass().getName()  // java.base/jdk.internal.loader
        };
        for (String name : classes) {
            Class<?> klass = Class.forName(name);

            // If the method is not called, an exception will be thrown, even in versions below Java16
            open(klass);

            for (Field field : klass.getDeclaredFields()) {
                field.setAccessible(true);
            }
        }
    }

    static void open(Class<?> klass) {
        inst.redefineModule(
                klass.getModule(),
                new HashSet<>(),
                new HashMap<>(),
                Map.of(klass.getPackageName(), Set.of(OpenTest.class.getModule())),
                new HashSet<>(),
                new HashMap<>()
        );
    }
}

Another issue is users can explicitly configure --illegal-access=deny to deny, which could break things here. Perhaps a saner way would be to recheck the return value of Module.isOpen .