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

OOM in tests since Quarkus 3.13.0 (Part 2) #42355

Closed
mschorsch opened this issue Aug 7, 2024 · 19 comments · Fixed by #42388
Closed

OOM in tests since Quarkus 3.13.0 (Part 2) #42355

mschorsch opened this issue Aug 7, 2024 · 19 comments · Fixed by #42388
Labels
kind/bug Something isn't working
Milestone

Comments

@mschorsch
Copy link
Contributor

mschorsch commented Aug 7, 2024

Describe the bug

As described in #42303 we get an OOM in our tests.

Because #42303 doesn't fix the problem completly i've opened a new issue with a new reproducer.

Compared to the reproducer in #42303 i have added more dependencies.

Expected behavior

No response

Actual behavior

No response

How to Reproduce?

oom-reproducer2.zip

./gradlew test --console=plain

⚠️ The reproducer uses 999-SNAPSHOT instead of 3.13.1 because 3.13.1 is not available in Maven Central yet.

Output of uname -a or ver

No response

Output of java -version

Java 21

Quarkus version or git rev

Quarkus 3.13.1

Build tool (ie. output of mvnw --version or gradlew --version)

Gradle 8.8

Additional information

No response

@geoand
Copy link
Contributor

geoand commented Aug 7, 2024

I'll take a look

@geoand
Copy link
Contributor

geoand commented Aug 7, 2024

Although I am yet to determine the actual cause of the problem, I have however been able to narrow it down to the quarkus-micrometer extension

@geoand
Copy link
Contributor

geoand commented Aug 7, 2024

I think I know what the problem is, but it will take a look while before I am able to test my theory

@geoand
Copy link
Contributor

geoand commented Aug 7, 2024

Seems that there are more than one issue... I'll likely continue tomorrow

geoand added a commit to geoand/quarkus that referenced this issue Aug 7, 2024
The way things were being done previously less to
us leaking the Deployment ClassLoader

Additionally, make changes so the extensions follows
some of the practices that others use

Fixes: quarkusio#42355
@geoand
Copy link
Contributor

geoand commented Aug 7, 2024

Would you be able to test #42369?

For whatever reason I could not get Gradle to use my changes...

@mschorsch
Copy link
Contributor Author

I've tested #42369 against our real application and still get an OOM.

Tomorrow i can try the reproducer.

@geoand
Copy link
Contributor

geoand commented Aug 7, 2024

Thanks for checking.

Looking forward to hearing if the OOM still happens with the reproducer

@geoand
Copy link
Contributor

geoand commented Aug 7, 2024

I was able to reproduce the problem in the reproducer even with my fix - so obviously there is even more to it.

@geoand
Copy link
Contributor

geoand commented Aug 7, 2024

I closed #42369 because it does not fix the issue.

I was however able to figure out to pinpoint the problem to the JVM and System related binders. Those use JMX which does not play nicely at all with multiple classloaders.
One option here would be to disable these by default and have users explicitly opt-in to them.
@ebullient WDYT?

cc @gsmet who was also interested in the outcome of this.

@geoand
Copy link
Contributor

geoand commented Aug 7, 2024

@mschorsch my comment above means that you can get the reproducer working by setting:

quarkus.micrometer.binder.jvm=false
quarkus.micrometer.binder.system=false

@gsmet
Copy link
Member

gsmet commented Aug 7, 2024

Ah, that's funny, I actually identified them as problematic here: #41233

Is it what you're seeing?

I really think we should try to tackle this issue and make this an error so that we can catch further issues.

@geoand
Copy link
Contributor

geoand commented Aug 8, 2024

Is it what you're seeing?

All JMX related Micrometer binder are causing the same issue. (see #42388)

I really think we should try to tackle this issue

What do you mean exactly, I don't follow

I really think we should try to tackle this issue and make this an error so that we can catch further issues.

Even if that is the case, it won't be done by me as I'm going to be off for a couple weeks :). Furthermore, in this specific case, I am pretty sure there is nothing we can do (with one caveat that I need to look into).

@geoand
Copy link
Contributor

geoand commented Aug 8, 2024

Turns out that this can be fixed and rather easily...

#42388 fixes the reproducer.

@mschorsch
Copy link
Contributor Author

@mschorsch my comment above means that you can get the reproducer working by setting:

quarkus.micrometer.binder.jvm=false
quarkus.micrometer.binder.system=false

I can confirm that this does indeed prevent the OOM in the reproducer 👍 .

In our real application, however, we still get an OOM even though I have deactivated the micrometer extension 😬 .

quarkus:
  micrometer:
    enabled: false
    binder:
      jvm: false
      system: false

Even the complete removal of io.quarkus:quarkus-micrometer-registry-prometheus did not help.

I will test #42388 but I suspect that there are other issues that are not covered by the repeoducer.

@geoand
Copy link
Contributor

geoand commented Aug 8, 2024

I will test #42388 but I suspect that there are other issues that are not covered by the repeoducer.

If you do find more OOMs, please open new issues with the relevent reproducers.
Thanks!

@mschorsch
Copy link
Contributor Author

mschorsch commented Aug 8, 2024

I can confirm that #42388 fixes the issue the reproducer 👍 .

In our real application we still get an OOM, seems a part 3 is needed...

Thanks for your work @geoand @gsmet

@geoand
Copy link
Contributor

geoand commented Aug 8, 2024

🙏🏼

geoand added a commit that referenced this issue Aug 8, 2024
Ensure that all AutoCloseable binders are closed
@quarkus-bot quarkus-bot bot added this to the 3.14 - main milestone Aug 8, 2024
@ebullient
Copy link
Member

I closed #42369 because it does not fix the issue.

I was however able to figure out to pinpoint the problem to the JVM and System related binders. Those use JMX which does not play nicely at all with multiple classloaders. One option here would be to disable these by default and have users explicitly opt-in to them. @ebullient WDYT?

cc @gsmet who was also interested in the outcome of this.

I know you're chasing other things, but I think not having this by default would be a surprise.

@geoand
Copy link
Contributor

geoand commented Aug 8, 2024

In any case, that proposal is no longer necessary :)

@gsmet gsmet modified the milestones: 3.14 - main, 3.13.2 Aug 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Something isn't working
Projects
None yet
4 participants