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

8284942: Proxy building can just iterate superinterfaces once #8281

Closed
wants to merge 8 commits into from
@@ -495,41 +495,16 @@ private static final class ProxyBuilder {
private static final ClassLoaderValue<Boolean> reverseProxyCache =
new ClassLoaderValue<>();

private static Class<?> defineProxyClass(Module m, List<Class<?>> interfaces) {
String proxyPkg = null; // package to define proxy class in
int accessFlags = Modifier.PUBLIC | Modifier.FINAL;
boolean nonExported = false;
private record ProxyContext(Module module, String pkg, boolean packagePrivate) {}
Copy link
Member

@mlchung mlchung May 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This represents the context for a proxy class. It seems ProxyClassContext would be clearer. I suggest the context to have the accessFlags for a proxy class rather than the boolean whether it's package private. The constructor should check that it must be 0 or Modifier.PUBLIC. FINAL will be added to the access flags when it generates the proxy class.


/*
* Record the package of a non-public proxy interface so that the
* proxy class will be defined in the same package. Verify that
* all non-public proxy interfaces are in the same package.
*/
for (Class<?> intf : interfaces) {
int flags = intf.getModifiers();
if (!Modifier.isPublic(flags)) {
accessFlags = Modifier.FINAL; // non-public, final
String pkg = intf.getPackageName();
if (proxyPkg == null) {
proxyPkg = pkg;
} else if (!pkg.equals(proxyPkg)) {
throw new IllegalArgumentException(
"non-public interfaces from different packages");
}
} else {
if (!intf.getModule().isExported(intf.getPackageName())) {
// module-private types
nonExported = true;
}
}
}
private static Class<?> defineProxyClass(ProxyContext context, List<Class<?>> interfaces) {
String proxyPkg = context.pkg;
Module m = context.module;

if (proxyPkg == null) {
// all proxy interfaces are public and exported
if (!context.packagePrivate) {
// all proxy interfaces are public
if (!m.isNamed())
throw new InternalError("unnamed module: " + m);
proxyPkg = nonExported ? PROXY_PACKAGE_PREFIX + "." + m.getName()
: m.getName();
} else if (proxyPkg.isEmpty() && m.isNamed()) {
Copy link
Member

@mlchung mlchung May 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the refactoring, it may be easier to understand to make this check for both public and non-public access (i.e. drop the else) for clarity - a named module can't have unnamed package.

Copy link
Contributor Author

@liach liach May 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a named module can't have unnamed package

Since this is mandated by the Java Language Specification 7.4.2, I am tempted to change the thrown exception to InternalError, but I cannot find any restriction in the JVM Specification that prevents declaring a package-private superinterface in the unnamed package of a named module. If we can find a reference in the JVM Specification, I'm more than glad to refer to that in a comment and throw InternalError instead.

throw new IllegalArgumentException(
"Unnamed package cannot be added to " + m);
@@ -555,6 +530,7 @@ private static Class<?> defineProxyClass(Module m, List<Class<?>> interfaces) {
/*
* Generate the specified proxy class.
*/
int accessFlags = (context.packagePrivate ? 0 : Modifier.PUBLIC) | Modifier.FINAL;
byte[] proxyClassFile = ProxyGenerator.generateProxyClass(loader, proxyName, interfaces, accessFlags);
try {
Class<?> pc = JLA.defineClass(loader, proxyName, proxyClassFile,
@@ -575,7 +551,7 @@ private static Class<?> defineProxyClass(Module m, List<Class<?>> interfaces) {

/**
* Test if given class is a class defined by
* {@link #defineProxyClass(Module, List)}
* {@link #defineProxyClass(ProxyContext, List)}
*/
static boolean isProxyClass(Class<?> c) {
return Objects.equals(reverseProxyCache.sub(c).get(c.getClassLoader()),
@@ -631,7 +607,7 @@ private static boolean isDebug(String flag) {
// ProxyBuilder instance members start here....

private final List<Class<?>> interfaces;
private final Module module;
private final ProxyContext context;
ProxyBuilder(ClassLoader loader, List<Class<?>> interfaces) {
if (!VM.isModuleSystemInited()) {
throw new InternalError("Proxy is not supported until "
@@ -648,8 +624,8 @@ private static boolean isDebug(String flag) {
validateProxyInterfaces(loader, interfaces, refTypes);

this.interfaces = interfaces;
this.module = mapToModule(loader, interfaces, refTypes);
assert getLoader(module) == loader;
this.context = setupContext(loader, interfaces, refTypes);
assert getLoader(context.module) == loader;
}

ProxyBuilder(ClassLoader loader, Class<?> intf) {
@@ -667,7 +643,8 @@ private static boolean isDebug(String flag) {
*/
@SuppressWarnings("removal")
Constructor<?> build() {
Class<?> proxyClass = defineProxyClass(module, interfaces);
Class<?> proxyClass = defineProxyClass(context, interfaces);
Module module = context.module;
assert !module.isNamed() || module.isOpen(proxyClass.getPackageName(), Proxy.class.getModule());

final Constructor<?> cons;
@@ -768,10 +745,11 @@ private static void addElementType(HashSet<Class<?>> types,
}

/**
* Returns the module that the generated proxy class belongs to.
* Returns the context for the generated proxy class, including the
* module and the package it belongs to and whether it is package-private.
*
* If any of proxy interface is package-private, then the proxy class
* is in the same module of the package-private interface.
* is in the same package and module as the package-private interface.
*
* If all proxy interfaces are public and in exported packages,
* then the proxy class is in a dynamic module in an unconditionally
@@ -785,14 +763,21 @@ private static void addElementType(HashSet<Class<?>> types,
*
* Reads edge and qualified exports are added for dynamic module to access.
*/
private static Module mapToModule(ClassLoader loader,
List<Class<?>> interfaces,
Set<Class<?>> refTypes) {
private static ProxyContext setupContext(ClassLoader loader,
Copy link
Member

@mlchung mlchung May 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps name this method proxyClassContext that returns ProxyClassContext.

List<Class<?>> interfaces,
Set<Class<?>> refTypes) {
Map<Class<?>, Module> packagePrivateTypes = new HashMap<>();
boolean nonExported = false;

for (Class<?> intf : interfaces) {
Module m = intf.getModule();
if (!Modifier.isPublic(intf.getModifiers())) {
packagePrivateTypes.put(intf, m);
} else {
if (!intf.getModule().isExported(intf.getPackageName())) {
// module-private types
nonExported = true;
}
}
}

@@ -838,12 +823,13 @@ private static Module mapToModule(ClassLoader loader,
Modules.addOpens(targetModule, targetPackageName, Proxy.class.getModule());
}
// return the module of the package-private interface
return targetModule;
return new ProxyContext(targetModule, targetPackageName, true);
}

// All proxy interfaces are public. So maps to a dynamic proxy module
// and add reads edge and qualified exports, if necessary
Module targetModule = getDynamicModule(loader);
var context = getDynamicModuleContext(loader, nonExported);
Copy link
Member

@mlchung mlchung May 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest to keep the getDynamicModule method as creating a dynamic module of a given class loader is a clear function. The context can be created in this method body.

            var targetModule = getDynamicModule(loader);
            var pkgName = nonExported ? PROXY_PACKAGE_PREFIX + '.' + targetModule.getName()
                                      : targetModule.getName();
            var context = new ProxyClassContext(targetModule, pkgName, Modifier.PUBLIC);

Module targetModule = context.module;

// set up proxy class access to proxy interfaces and types
// referenced in the method signature
@@ -852,7 +838,7 @@ private static Module mapToModule(ClassLoader loader,
for (Class<?> c : types) {
ensureAccess(targetModule, c);
}
return targetModule;
return context;
}

/*
@@ -904,8 +890,8 @@ private static Class<?> getElementType(Class<?> type) {
*
* Each class loader will have one dynamic module.
*/
private static Module getDynamicModule(ClassLoader loader) {
return dynProxyModules.computeIfAbsent(loader, (ld, clv) -> {
private static ProxyContext getDynamicModuleContext(ClassLoader loader, boolean nonExported) {
var module = dynProxyModules.computeIfAbsent(loader, (ld, clv) -> {
// create a dynamic module and setup module access
String mn = "jdk.proxy" + counter.incrementAndGet();
String pn = PROXY_PACKAGE_PREFIX + "." + mn;
@@ -922,6 +908,9 @@ private static Module getDynamicModule(ClassLoader loader) {
Modules.addOpens(m, mn, Proxy.class.getModule());
return m;
});
return new ProxyContext(module, nonExported
? PROXY_PACKAGE_PREFIX + '.' + module.getName()
: module.getName(), false);
}
}