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,67 +495,50 @@ 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 ProxyClassContext(Module module, String pkg, int accessFlags) {
Copy link
Member

@mlchung mlchung May 26, 2022

Choose a reason for hiding this comment

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

Suggested change
private record ProxyClassContext(Module module, String pkg, int accessFlags) {
private record ProxyClassContext(Module module, String packageName, int accessFlags) {

private ProxyClassContext {
Copy link
Member

@mlchung mlchung May 26, 2022

Choose a reason for hiding this comment

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

We should validate the accessFlags value as it must be 0 or PUBLIC.

if (module.isNamed()) {
if (pkg.isEmpty()) {
// Per JLS 7.4.2, unnamed package can only exist in unnamed modules.
// This means a package-private superinterface exist in the unnamed
// package of a named module.
throw new InternalError("Unnamed package cannot be added to " + module);
}

/*
* 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");
if (!module.getDescriptor().packages().contains(pkg)) {
throw new InternalError(pkg + " not exist in " + module.getName());
}

if (!module.isOpen(pkg, Proxy.class.getModule())) {
// Required for default method invocation
Copy link
Member

@mlchung mlchung May 26, 2022

Choose a reason for hiding this comment

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

Is this comment true?

The module of the proxy class opens the package to java.base if the proxy interface is non-public in a named module or if all proxy interfaces are public but a proxy interface is not unconditionally exported.

Copy link
Contributor Author

@liach liach May 26, 2022

Choose a reason for hiding this comment

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

The original check and Modules.addOpen calls were added in 8159476, when the invokeDefault support was added.

See: 56b15fb#diff-c15cc774a95bac204c294b9ca8e20332c81904e506e16bb7d5a82d1c30cced17R667, or #313

Copy link
Member

@mlchung mlchung May 26, 2022

Choose a reason for hiding this comment

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

Ah, I see. That assert was added in PR #313 as a clarification to the current proxy implementation but not required by the InvocationHandle::invokeDefault. It could have been added before the default method support.

Copy link
Contributor Author

@liach liach May 26, 2022

Choose a reason for hiding this comment

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

It appears to me that it's required by proxyClassLookup to perform reflective access on the dynamic module or the module where the package-private interface is located.

throw new InternalError(pkg + " not open to " + Proxy.class.getModule());
}
} else {
if (!intf.getModule().isExported(intf.getPackageName())) {
// module-private types
nonExported = true;
if (Modifier.isPublic(accessFlags)) {
// All proxy superinterfaces are public, must be in named dynamic module
throw new InternalError("proxy in unnamed module: " + module);
}
}
}
}

if (proxyPkg == null) {
// all proxy interfaces are public and exported
if (!m.isNamed())
throw new InternalError("unnamed module: " + m);
proxyPkg = nonExported ? PROXY_PACKAGE_PREFIX + "." + m.getName()
: m.getName();
} else if (proxyPkg.isEmpty() && m.isNamed()) {
throw new IllegalArgumentException(
"Unnamed package cannot be added to " + m);
}

if (m.isNamed()) {
if (!m.getDescriptor().packages().contains(proxyPkg)) {
throw new InternalError(proxyPkg + " not exist in " + m.getName());
}
}

private static Class<?> defineProxyClass(ProxyClassContext context, List<Class<?>> interfaces) {
/*
* Choose a name for the proxy class to generate.
*/
long num = nextUniqueNumber.getAndIncrement();
String proxyName = proxyPkg.isEmpty()
String proxyName = context.pkg().isEmpty()
? proxyClassNamePrefix + num
: proxyPkg + "." + proxyClassNamePrefix + num;
: context.pkg() + "." + proxyClassNamePrefix + num;

ClassLoader loader = getLoader(m);
trace(proxyName, m, loader, interfaces);
ClassLoader loader = getLoader(context.module());
trace(proxyName, context.module(), loader, interfaces);

/*
* Generate the specified proxy class.
*/
byte[] proxyClassFile = ProxyGenerator.generateProxyClass(loader, proxyName, interfaces, accessFlags);
byte[] proxyClassFile = ProxyGenerator.generateProxyClass(loader, proxyName, interfaces,
context.accessFlags() | Modifier.FINAL);
try {
Class<?> pc = JLA.defineClass(loader, proxyName, proxyClassFile,
null, "__dynamic_proxy__");
@@ -575,7 +558,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(ProxyClassContext, List)}
*/
static boolean isProxyClass(Class<?> c) {
return Objects.equals(reverseProxyCache.sub(c).get(c.getClassLoader()),
@@ -631,7 +614,7 @@ private static boolean isDebug(String flag) {
// ProxyBuilder instance members start here....

private final List<Class<?>> interfaces;
private final Module module;
private final ProxyClassContext context;
ProxyBuilder(ClassLoader loader, List<Class<?>> interfaces) {
if (!VM.isModuleSystemInited()) {
throw new InternalError("Proxy is not supported until "
@@ -648,8 +631,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 = proxyClassContext(loader, interfaces, refTypes);
assert getLoader(context.module()) == loader;
}

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

final Constructor<?> cons;
try {
@@ -768,10 +750,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 +768,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 ProxyClassContext proxyClassContext(ClassLoader loader,
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,7 +828,7 @@ 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 ProxyClassContext(targetModule, targetPackageName, 0);
}

// All proxy interfaces are public. So maps to a dynamic proxy module
@@ -852,7 +842,10 @@ private static Module mapToModule(ClassLoader loader,
for (Class<?> c : types) {
ensureAccess(targetModule, c);
}
return targetModule;

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

/*