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

Classes generated by PFL's Wrapper are sometimes incompatible with JDK17 #23628

Closed
dmatej opened this issue Sep 27, 2021 · 2 comments · Fixed by #23770 or #23801
Closed

Classes generated by PFL's Wrapper are sometimes incompatible with JDK17 #23628

dmatej opened this issue Sep 27, 2021 · 2 comments · Fixed by #23770 or #23801
Assignees
Labels
bug Something isn't working techdebt
Milestone

Comments

@dmatej
Copy link
Contributor

dmatej commented Sep 27, 2021

When I implemented #23621 changes, I tried to follow instructions on @deprecated methods of the Wrapper class, but tests then failed.

// WORKS, but is deprecated:
return Wrapper._generate(loader, generator.getAnchorClass().getProtectionDomain(), props);
//  WORKS, but result is not usable for generating proxy by another classloader
return Wrapper._generate(generator.getAnchorClass(), props);

Btw we already have javaassist in dependencies.

This issue relates also to #23627, because on modern JDK's that mess between packages brings serious issues around modularity and classloading rules.

@dmatej dmatej added the component upgrade A component dependency has been upgraded label Sep 27, 2021
@dmatej dmatej changed the title Replace PFL's Wrapper usages with more actual tool Replace PFL's Wrapper usages with more current tool Sep 27, 2021
@dmatej dmatej added this to the 6.2.5 milestone Jan 22, 2022
@dmatej dmatej changed the title Replace PFL's Wrapper usages with more current tool Classes generated by PFL's Wrapper are sometimes incompatible with JDK17 Jan 22, 2022
@dmatej dmatej added bug Something isn't working and removed component upgrade A component dependency has been upgraded labels Jan 22, 2022
@dmatej
Copy link
Contributor Author

dmatej commented Jan 23, 2022

I switched this issue to a bug, because the solution in #23621 doesn't play well with proxies. That was discovered when I resuscitated ejb_all tests under #23507 , so it will go both under single PR, because merged state always have to pass all tests and has to be stable.

The issue is that the generating code by MethodHandles is more strict than the code generated by ClassLoader.defineClass.
Facts:

  • Under JDK11+ doesn't exist Unsafe.defineClass (already resolved in PFL).
  • MH: generated class will have the same package and same classloader as the anchorClass
  • CL: generated class can have any package, will use the provided classloader and protectiondomain
  • getClassLoader methods can return null for bootstrap classloader
  • Proxy.newProxyInstance has strict rules: all referred types must be loadable by the used classloader
  • org.glassfish.pfl.dynamic.codegen.spi.Type.type uses internal cache to remember classes while it generates new ones.
  • -> no way to generate proxies for class referring class generated by MH for anchorClass loaded by embeddedGF's ASURLCL
  • -> EXCEPT ...

@dmatej dmatej self-assigned this Jan 23, 2022
@dmatej
Copy link
Contributor Author

dmatej commented Jan 23, 2022

Relates to eclipse-ee4j/orb-gmbal-pfl#43 and #23616

dmatej added a commit to dmatej/glassfish that referenced this issue Jan 23, 2022
…d deprecated Proxy.getProxyClass

- EJB tests failed on JDK17 because of this
- also more consistent generateAndLoad (to be finished in next commits)
dmatej added a commit to dmatej/glassfish that referenced this issue Jan 23, 2022
dmatej added a commit to dmatej/glassfish that referenced this issue Jan 23, 2022
- not completely as under some circumstances we cannot live without them
- but we can decide when
- if the class was generated in the same package, the usage and the classloader
  are suitable, we can use MethodHandles. Otherwise we have to use the
  ClassLoader.defineClass method (accessed by reflection)
- we had to avoid using Base class from PFL, because it uses just MethodHandles
  XOR ClassLoader XOR Unsafe depending on the JDK.
- this solution was tested by
  - glassfish's ejb_all tests with JDK11 and JDK17
  - TCK/ejb with JDK11
dmatej added a commit to dmatej/glassfish that referenced this issue Jan 23, 2022
dmatej added a commit to dmatej/glassfish that referenced this issue Jan 24, 2022
…ot in the same package

- that means that we cannot use the MethodHandles.Looku.defineClass impl
dmatej added a commit to dmatej/glassfish that referenced this issue Jan 27, 2022
- formatting of the code to make it easier to read
- fixed several log messages (wrong formatting)
- ContainerInfo used to reduce duplications
- replaced several deprecated method calls with simple straight replacements
- deleted 20 years old TODOs
dmatej added a commit to dmatej/glassfish that referenced this issue Jan 27, 2022
@dmatej dmatej linked a pull request Jan 27, 2022 that will close this issue
@dmatej dmatej closed this as completed Jan 28, 2022
dmatej added a commit to dmatej/glassfish that referenced this issue Jan 31, 2022
dmatej added a commit to dmatej/glassfish that referenced this issue Feb 11, 2022
- now ClassGenerator is responsible for everything around this.
dmatej added a commit to dmatej/glassfish that referenced this issue Feb 11, 2022
- now ClassGenerator is responsible for everything around this.
dmatej added a commit that referenced this issue Feb 11, 2022
- now ClassGenerator is responsible for everything around this.
@dmatej dmatej linked a pull request Feb 11, 2022 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working techdebt
Projects
None yet
1 participant