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

sun.misc.Unsafe define class usage #43

Open
smillidge opened this issue Dec 15, 2021 · 12 comments · May be fixed by #61
Open

sun.misc.Unsafe define class usage #43

smillidge opened this issue Dec 15, 2021 · 12 comments · May be fixed by #61

Comments

@smillidge
Copy link
Contributor

See https://github.com/eclipse-ee4j/orb-gmbal-pfl/blob/master/pfl-basic/src/main/java/org/glassfish/pfl/basic/reflection/BridgeBase.java#L241

This needs replacing with a supported mechanism in later JDKs

@ctabin
Copy link

ctabin commented Dec 21, 2021

Hi @smillidge,
If this can help, here is a reproducer of the problem (based on the one provided in payara/Payara#5500 but with eclipse-ee4j/glassfish v6.2.3 and jakartaee 9.1.0). It fails both with JDK11 and JDK17.

Let me know if some issue should be raised in eclipse-ee4j/glassfish or if any other information is needed.

Thanks & best regards.

@ctabin
Copy link

ctabin commented Dec 22, 2021

For the record, check this implementation gradle/gradle#4976. It seems it uses a somewhat hacky solution to access a private method MethodHandles.privateLookupIn in order to mimic the behavior of Usafe.defineClass.

EDIT: seems the PR is a little bit outdated and the actual implementation is a little bit different.

@smillidge Would you be open for a PR that implements this solution ?

@russgold
Copy link
Contributor

russgold commented Dec 22, 2021

It already supports JDK11 via the multi-release jar mechanism; is that not enough? The basic problem is that the API which replaces the JDK8 approach isn't available before JDK9; the catch is that the using project's classloaders need to be upgraded to support MR jars.

@ctabin
Copy link

ctabin commented Dec 22, 2021

Hi @russgold,
Thank you for jumping in. This is a good catch: since we use the maven-assembly-plugin to package all our classes with the full embedded server, we have to add the Multi-Release entry in the manifest:

<plugin>
  <groupId>org.apache.maven.plugins</groupId>
  <artifactId>maven-assembly-plugin</artifactId>
  <configuration>
    <archive>
      <manifestEntries>
        <Multi-Release>true</Multi-Release>
      </manifestEntries>
    </archive>
    <descriptorRefs>
      <descriptorRef>jar-with-dependencies</descriptorRef>
    </descriptorRefs>
  </configuration>
</plugin>

That solves the problem for the fully package JAR, however this is not sufficient when the tests are directly executed through the maven-surefire-plugin. In order to "force" the correct detection, we had to manually add the dependency to the pfl-basic project:

<dependency>
    <groupId>org.glassfish.pfl</groupId>
    <artifactId>pfl-basic</artifactId>
    <version>4.1.2</version>
</dependency>

With those modifications, we were able to deploy our application both in the embedded JAR and through the Maven tests 👍

However, I wonder if it wouldn't be better to implement some sort of reflection in this case to use the JDK11 or JDK8 API and avoids long hours of debugging when hitting this kind of problem 🤔

@russgold
Copy link
Contributor

Ideally, we would use reflection; the problem is that the old APIs are not available in the new JDKs and the new ones are not available in the old JDKs, so there is no way to get both in the same codebase. That's become a lot more common since JDK9, when the Java team started aggressively removing deprecated APIs, which is why MR jars are needed. It has been a royal pain, as the development tools (both IDEs and build tools) don't provide very good support for them. I have been dealing with it via build profiles (see the POM for pfl-basic).

@dmatej
Copy link
Contributor

dmatej commented Jan 20, 2022

FYI, I didn't push yet to GF, but I found that neither one solution is perfect for GlassFish on JDK17, because JDK enforced restrictions. Finally I had to use both mechanisms, via MethodHandles and via ClassLoader to pass old ejb_all tests with JDK17. JDK11 was good (after a small change), but for JDK17 I had to change much more; now it relies on quite ugly PFL api, luckily (by design mistake) allowing me to workaround it's defineClass mechanism (it should be more open, so user's would be able to customize it on their own - that would resolve future issues).

As I am not committer of this project and it seems it is quite unmaintained, I will not spend more time creating PRs if nobody merges them in few hours/days, but what I see is ..

  • there's no need for multirelease jar, but PFL should go for JDK11 and abandon JDK8 (someone can maintain it in a side branch if he needs it)
  • jenkins should cover JDK11, JDK17 and probably next candidate too as it relies on quite low level of the JDK.
  • PFL5.0.0 should be more open for customizations (static API with threadlocals is really scary concept); but that would need much more work and also more understanding than I have now.

@arjantijms
Copy link

there's no need for multirelease jar, but PFL should go for JDK11 and abandon JDK8 (someone can maintain it in a side branch if he needs it)

Should we create a 6.0.0 version where we set the minimum level to JDK 11 of even JDK 17, and get rid of the MR code (as well as the security manager code)?

@dmatej
Copy link
Contributor

dmatej commented Aug 15, 2024

But then you cannot use it with GF7 which still uses SecurityManager. It would be better to fix this first and be able to run on later JDK releases, and then remove the security manager related code for 6.0.0. That is completely different issue.

there's no need for multirelease jar, but PFL should go for JDK11 and abandon JDK8 (someone can maintain it in a side branch if he needs it)

Should we create a 6.0.0 version where we set the minimum level to JDK 11 of even JDK 17, and get rid of the MR code (as well as the security manager code)?

@arjantijms
Copy link

So 5.1 to set the minimum level to JDK 11, and a 6.0 for JDK 17 + removal of security manager?

@dmatej
Copy link
Contributor

dmatej commented Aug 15, 2024

So 5.1 to set the minimum level to JDK 11, and a 6.0 for JDK 17 + removal of security manager?

5.0 already has minimum JDK11, so there is no need to change it, or I missed something? The problem is maximal version, not minimal :-)
See f88839d

EDIT: Aha, we are talking rather about this one ... https://github.com/eclipse-ee4j/orb ... I will check where we are with that ...

@arjantijms
Copy link

5.0 already has minimum JDK11

There's still this stuff: https://github.com/eclipse-ee4j/orb-gmbal-pfl/blob/master/pom.xml#L418

@dmatej
Copy link
Contributor

dmatej commented Aug 19, 2024

5.0 already has minimum JDK11

There's still this stuff: https://github.com/eclipse-ee4j/orb-gmbal-pfl/blob/master/pom.xml#L418

That is already removed on my machine, just give me time to sync all dependencies and verify that I did not break anything ;-)

@dmatej dmatej linked a pull request Aug 21, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants