On Fri, 2 Jun 2023 03:36:35 GMT, Chen Liang <li...@openjdk.org> wrote:

>> Add API to explore Class Hierarchy with a `ClassLoader` or a `Lookup` with 
>> proper privileges, with tests.
>> 
>> This addition is useful in case classes at runtime are not loaded from the 
>> system class loader, such as Proxy. This is also useful to APIs that 
>> generate bytecode with a `Lookup` object, such as a custom 
>> single-abstract-method class implementations from a method handle.
>> 
>> See 
>> https://mail.openjdk.org/pipermail/classfile-api-dev/2023-March/000249.html 
>> as well.
>> 
>> Current questions, which I wish to discuss with @asotona:
>> 1. Should the resolver fail fast on `IllegalAccessException` from the 
>> lookup? This usually indicates the hierarchy resolver is set up improperly, 
>> and proceeding may simply yield verification errors in class loading that 
>> are hard to track. For bytecode-generating APIs, throwing access errors for 
>> the Lookup eagerly is also more preferable than later silent generation 
>> failure.
>> 2. Whether the default resolver should be reading from jrt alone, reflection 
>> alone, or jrt then reflection. I personally believe reflection alone is more 
>> reliable, for classes may redefined with instrumentation or jfr, which may 
>> not be reflected in the system resources.
>> 3. In addition, I don't think chaining system class loader reflection after 
>> system resource retrieval is really meaningful: is there any case where 
>> reflection always works while the system resource retrieval always fails?
>
> Chen Liang has updated the pull request with a new target base due to a merge 
> or a rebase. The pull request now contains 12 commits:
> 
>  - 1. Moved the default resolver to a static method, in anticipation of 
> future changes
>    2. Removed SecurityManager related content
>    3. Changed ClassHierarchyInfo into an interface
>    4. Moved caching method from static to instance method
>  - Merge branch 'master' into hierarchy-resolve
>  - rename to ofClassLoading/ofResourceParsing
>    convert the default class provider to bypass security manager restrictions
>  - Merge branch 'master' into hierarchy-resolve
>  - Merge branch 'master' into hierarchy-resolve
>  - Test both cached and uncached resolvers
>  - Update the class hierarchy resolver api as per mailing list last week
>  - Merge branch 'master' into hierarchy-resolve
>  - Update 
> src/java.base/share/classes/jdk/internal/classfile/impl/ClassHierarchyImpl.java
>    
>    Co-authored-by: Andrey Turbanov <turban...@gmail.com>
>  - Make lookup based resolver throw on illegal access eagerly
>  - ... and 2 more: https://git.openjdk.org/jdk/compare/101bf229...9e9079fb

Globas static cache needs to be synchronised If this PR goes in before #14180 
Or a `defaultResolver()` should create a new cache instance (and document it is 
thread-unsafe) if it goes after #14180

src/java.base/share/classes/jdk/internal/classfile/ClassHierarchyResolver.java 
line 57:

> 55:      */
> 56:     static ClassHierarchyResolver defaultResolver() {
> 57:         return ClassHierarchyImpl.DEFAULT_RESOLVER;

defaultResolver() delegates to a thread-unsafe global static instance of cached 
resolver.

src/java.base/share/classes/jdk/internal/classfile/impl/ClassHierarchyImpl.java 
line 130:

> 128:         @Override
> 129:         public ClassHierarchyInfo getClassInfo(ClassDesc classDesc) {
> 130:             var ret = resolvedCache.computeIfAbsent(classDesc, new 
> Function<>() {

couldn't this function be cached?

test/jdk/jdk/classfile/ClassHierarchyInfoTest.java line 34:

> 32:  *          java.base/jdk.internal.classfile.impl
> 33:  *          java.base/jdk.internal.classfile.impl.verifier
> 34:  *          java.base/jdk.internal.classfile.java.lang.constant

jdk.internal.classfile.java.lang.constant package has been removed

-------------

Changes requested by asotona (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/13082#pullrequestreview-1466930855
PR Review Comment: https://git.openjdk.org/jdk/pull/13082#discussion_r1221124185
PR Review Comment: https://git.openjdk.org/jdk/pull/13082#discussion_r1221102574
PR Review Comment: https://git.openjdk.org/jdk/pull/13082#discussion_r1221108184

Reply via email to