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

Allow loading custom Groovy extension modules #34447

Merged

Conversation

essobedo
Copy link
Contributor

@essobedo essobedo commented Jun 30, 2023

fixes #34446

Motivation

The custom Groovy extension modules cannot be detected from the System classloader

Modifications:

  • No more defines Groovy as a parent first artifact
  • Adds GroovyClassValue to disable the usage of ClassValue in Groovy
  • Removes GroovyCacheCleaner as it is no more used

@quarkus-bot

This comment has been minimized.

@essobedo essobedo force-pushed the 34446/allow-groovy-extension-modules branch from 5920dc6 to cf481c5 Compare July 3, 2023 07:18
@geoand
Copy link
Contributor

geoand commented Jul 3, 2023

Interesting approach. Are you sure this avoids memory leaks? How did you test?

@essobedo
Copy link
Contributor Author

essobedo commented Jul 3, 2023

Interesting approach. Are you sure this avoid memory leaks?

Yes I'm sure I have tested it against my small project and the test project defined in #12498 with 30 tests instead 10 initially

@essobedo
Copy link
Contributor Author

essobedo commented Jul 3, 2023

Actually, the problem is related to the ClassValue added to the JDK for languages like Groovy but they are very hard to clean up, the only way I know is the one described in this SO answer but in some use cases it can be too much and can have side effects

@essobedo essobedo force-pushed the 34446/allow-groovy-extension-modules branch from cf481c5 to 786b44e Compare July 3, 2023 07:39
@essobedo essobedo force-pushed the 34446/allow-groovy-extension-modules branch from 786b44e to 752ec72 Compare July 3, 2023 09:39
@geoand geoand added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Jul 3, 2023
@essobedo
Copy link
Contributor Author

essobedo commented Jul 3, 2023

It looks like the CI is unstable because Maven Tests - JDK 11 Windows fails in my repo while JVM Tests - JDK 17 Windows passes as you can see here, and here it is the exact opposite. Moreover, I would be surprised if those changes were really OS-specific.

@essobedo
Copy link
Contributor Author

essobedo commented Jul 3, 2023

Another remark is about the duration of some Jobs that seem to be very long, I don't know if it is common or not?

@quarkus-bot

This comment has been minimized.

@essobedo
Copy link
Contributor Author

essobedo commented Jul 3, 2023

I see more or less the same results (duration + failing tests) in another PR #34417

@geoand
Copy link
Contributor

geoand commented Jul 4, 2023

Yeah, those seem to be flaky tests

@quarkus-bot
Copy link

quarkus-bot bot commented Jul 4, 2023

✔️ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

@geoand geoand merged commit 0fa5d79 into quarkusio:main Jul 4, 2023
@quarkus-bot quarkus-bot bot added kind/enhancement New feature or request and removed triage/waiting-for-ci Ready to merge when CI successfully finishes labels Jul 4, 2023
@quarkus-bot quarkus-bot bot added this to the 3.3 - main milestone Jul 4, 2023
@essobedo essobedo deleted the 34446/allow-groovy-extension-modules branch July 4, 2023 12:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow loading custom Groovy extension modules
2 participants