search

quarkusio / quarkus

Quarkus: Supersonic Subatomic Java.
https://quarkus.io
Apache License 2.0
13.64k stars 2.64k forks source link

Increased augmentation times in Quarkus 3.8.4 #40780

Closed cwelch-rh closed 3 weeks ago

cwelch-rh commented 4 months ago

Describe the bug

Upgrading from 3.8.3 to 3.8.4 increased augmentation times mostly due to the ResteasyReactiveJacksonProcessor#handleFieldSecurity method. Keycloak allows custom extensions to be included in re-augmentation, which causes even more increase in augmentation time. It appears that the issue is fixed in 3.10.1.

3.8.3 No custom extensions:

Finished step "io.quarkus.resteasy.reactive.jackson.deployment.processor.ResteasyReactiveJacksonProcessor#handleFieldSecurity" in 17 ms

3.8.3 With custom extensions:

Finished step "io.quarkus.resteasy.reactive.jackson.deployment.processor.ResteasyReactiveJacksonProcessor#handleFieldSecurity" in 19 ms

3.8.4 No custom extensions:

Finished step "io.quarkus.resteasy.reactive.jackson.deployment.processor.ResteasyReactiveJacksonProcessor#handleFieldSecurity" in 827 ms

3.8.4 With custom extensions:

Finished step "io.quarkus.resteasy.reactive.jackson.deployment.processor.ResteasyReactiveJacksonProcessor#handleFieldSecurity" in 44565 ms

3.10.1 No custom extensions:

Finished step "io.quarkus.resteasy.reactive.jackson.deployment.processor.ResteasyReactiveJacksonProcessor#handleFieldSecurity" in 0 ms

3.10.1 With custom extensions:

Finished step "io.quarkus.resteasy.reactive.jackson.deployment.processor.ResteasyReactiveJacksonProcessor#handleFieldSecurity" in 1 ms

Expected behavior

No response

Actual behavior

No response

How to Reproduce?

Reproduction steps with Keycloak are in https://github.com/keycloak/keycloak/issues/29579

Output of uname -a or ver

No response

Output of java -version

No response

Quarkus version or git rev

No response

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

No response

Additional information

No response

gsmet commented 4 months ago

Any chance you could try with this branch: https://github.com/quarkusio/quarkus/pull/40792 ?

The version will be 3.8.999-SNAPSHOT. Make sure you use the io.quarkus:quarkus-bom and not the platform one.

cwelch-rh commented 4 months ago

Looks good to me. Thanks!

geoand commented 4 months ago

Looks good to me. Thanks!

Meaning that it's better now and more inline with what it used to be?

cwelch-rh commented 4 months ago

Yes. Timings are more in line with what we were experiencing with 3.8.3. Actually, a bit better.

geoand commented 4 months ago

That's great, thanks!

patrickbadleyupstart commented 4 months ago

Hello, we are attempting to upgrade from keycloak 23.0.7 to 24.0.4 and are getting this error. It looks maybe related to this thread so I was hoping someone here could give me some context on this code and how we may go about resolving the error? It occurs in a deployed environment, but not a local dev environment.

ERROR: Failed to run 'build' command.
ERROR: io.quarkus.builder.BuildException: Build failure: Build failed due to errors
    [error]: Build step io.quarkus.resteasy.reactive.jackson.deployment.processor.ResteasyReactiveJacksonProcessor#handleFieldSecurity threw an exception: java.lang.StackOverflowError
    at org.jboss.jandex.DotName.buildString(DotName.java:305)
    at org.jboss.jandex.DotName.buildString(DotName.java:305)
    at org.jboss.jandex.DotName.toString(DotName.java:296)
    at org.jboss.jandex.DotName.toString(DotName.java:283)
    at org.jboss.jandex.DotName.packagePrefix(DotName.java:217)
    at io.quarkus.resteasy.reactive.jackson.deployment.processor.ResteasyReactiveJacksonProcessor.fieldTypeHasSecureFields(ResteasyReactiveJacksonProcessor.java:524)
    at io.quarkus.resteasy.reactive.jackson.deployment.processor.ResteasyReactiveJacksonProcessor.lambda$fieldTypeHasSecureFields$3(ResteasyReactiveJacksonProcessor.java:539)
    at java.base/java.util.stream.MatchOps$1MatchSink.accept(MatchOps.java:90)
    at java.base/java.util.Spliterators$IteratorSpliterator.tryAdvance(Spliterators.java:1856)
    at java.base/java.util.stream.ReferencePipeline.forEachWithCancel(ReferencePipeline.java:129)
    at java.base/java.util.stream.AbstractPipeline.copyIntoWithCancel(AbstractPipeline.java:527)
    at java.base/java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:513)
    at java.base/java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:499)
    at java.base/java.util.stream.MatchOps$MatchOp.evaluateSequential(MatchOps.java:230)
    at java.base/java.util.stream.MatchOps$MatchOp.evaluateSequential(MatchOps.java:196)
    at java.base/java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234)
    at java.base/java.util.stream.ReferencePipeline.anyMatch(ReferencePipeline.java:632)
    at io.quarkus.resteasy.reactive.jackson.deployment.processor.ResteasyReactiveJacksonProcessor.fieldTypeHasSecureFields(ResteasyReactiveJacksonProcessor.java:539)
    at io.quarkus.resteasy.reactive.jackson.deployment.processor.ResteasyReactiveJacksonProcessor.lambda$anyFieldHasSecureFields$2(ResteasyReactiveJacksonProcessor.java:517)
    at java.base/java.util.stream.MatchOps$1MatchSink.accept(MatchOps.java:90)
    at java.base/java.util.stream.ReferencePipeline$3$1.accept(ReferencePipeline.java:197)
    at java.base/java.util.Spliterators$IteratorSpliterator.tryAdvance(Spliterators.java:1856)
    at java.base/java.util.stream.ReferencePipeline.forEachWithCancel(ReferencePipeline.java:129)
    at java.base/java.util.stream.AbstractPipeline.copyIntoWithCancel(AbstractPipeline.java:527)
    at java.base/java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:513)
    at java.base/java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:499)
    at java.base/java.util.stream.MatchOps$MatchOp.evaluateSequential(MatchOps.java:230)
    at java.base/java.util.stream.MatchOps$MatchOp.evaluateSequential(MatchOps.java:196)
    at java.base/java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234)
    at java.base/java.util.stream.ReferencePipeline.anyMatch(ReferencePipeline.java:632)
    at io.quarkus.resteasy.reactive.jackson.deployment.processor.ResteasyReactiveJacksonProcessor.anyFieldHasSecureFields(ResteasyReactiveJacksonProcessor.java:517)
gsmet commented 4 months ago

@patrickbadleyupstart it's definitely related. You have that with plain Keycloak? Or you have some specific stuff?

@michalvavrik I wonder if you could get in a loop if an object references itself in the tree? Maybe you should keep a Set of the types you have already treated?

michalvavrik commented 4 months ago

@michalvavrik I wonder if you could get in a loop if an object references itself in the tree? Maybe you should keep a Set of the types you have already treated?

I lived in impression that's what I already did here: https://github.com/quarkusio/quarkus/blob/main/extensions/resteasy-reactive/rest-jackson/deployment/src/main/java/io/quarkus/resteasy/reactive/jackson/deployment/processor/ResteasyReactiveJacksonProcessor.java#L487 but what I can see from the stacktrace it is in fieldTypeHasSecureFields recursion. This is a real problem in general and I need to fix it and I am terribly sorry.

As far as Keycloak is concerned, even with the custom extensions (providers or whatever is proper Keycloak term), it's fixed. They cannot really use SecureField because they do not rely on Quarkus Security.

patrickbadleyupstart commented 4 months ago

@patrickbadleyupstart it's definitely related. You have that with plain Keycloak? Or you have some specific stuff?

@gsmet We do have some custom plugins, but it's hard to say what is causing the error from our logs. The pod crashloops with a stackoverflow exception and the logs don't give an indication of where this error is getting triggered. I do think it's interesting the same image runs fine locally, but not in a deployed environment.

Can either of you describe what is going on in this code? Is this parsing some configuration JSON or what that may help me troubleshoot.

@michalvavrik You mentioned it's fixed, but I assume to get that fix we would need a new release of keycloak. Do either of you know when this may have been introduced?

michalvavrik commented 4 months ago

@michalvavrik You mentioned it's fixed, but I assume to get that fix we would need a new release of keycloak. Do either of you know when this may have been introduced?

@gsmet is your man to answer this; I am merely the one who caused this nightmare.

michalvavrik commented 4 months ago

Can either of you describe what is going on in this code? Is this parsing some configuration JSON or what that may help me troubleshoot.

Annotate your resources with DisableSecureSerialization as workaround. It won't have negative effect as I presume you do not use Quarkus Security.

gsmet commented 4 months ago

We are in the process of finalizing Quarkus 3.8.5, that will be consumed by Keycloak afterwards. It will probably take a few weeks.

What I don't understand is why you would have the problem in one environment and not the others?

patrickbadleyupstart commented 4 months ago

Annotate your resources with DisableSecureSerialization as workaround. It won't have negative effect as I presume you do not use Quarkus Security.

We can try this in the meantime, thank you!

Yes the difference in behavior between environments is puzzling.

patrickbadleyupstart commented 4 months ago

DisableSecureSerialization Didn't seem to have any effect for us :/

michalvavrik commented 4 months ago

DisableSecureSerialization Didn't seem to have any effect for us :/

Flow from your stacktrace is never run for resources annotated with DisableSecureSerialization. Plain Keycloak doesn't have recursion problem IIUC (I tested it in when the original issue got reported). Therefore I think it must be in code that is non-plain Keycloak. Hard to say more without seeing code or having more context.

patrickbadleyupstart commented 4 months ago

Is this the correct way to apply that annotation? (this is Kotlin code FYI)

import io.quarkus.resteasy.reactive.jackson.DisableSecureSerialization

@DisableSecureSerialization
class RealmResource(
michalvavrik commented 4 months ago

Is this the correct way to apply that annotation? (this is Kotlin code FYI)

import io.quarkus.resteasy.reactive.jackson.DisableSecureSerialization

@DisableSecureSerialization
class RealmResource(

yes; I have no doubt you did what I suggested correctly. For me it is rather hard to be helpful without reproducer. I suppose it is possible that some of your classes is assignable from plain Keycloak Jakarta REST resources, in which case your options are limited.

michalvavrik commented 4 months ago

I have analyzed https://github.com/quarkusio/quarkus/issues/40780#issuecomment-2127610347 and I didn't find scenario in which recursion could lead into stack overflow with current implementation. I don't say it is impossible in theory, but we should probably refactor in the future with getKnownUsers rather than pursuing very unlikely scenarios with the current one.

Not really related, but I found a place where exclusions can be improved and I'll open tiny PR for that.

patrickbadleyupstart commented 4 months ago

We downgraded from keycloak 24.0.4 to 24.0.3 and no longer get our stackoverflow error. We should be ok on this version for some time in case the above fix addresses our problem with the quarkus version next time we attempt an upgrade. Thank you for looking into this for us.

geoand commented 3 months ago

Is there anything more to be done on this?

michalvavrik commented 2 months ago

Is there anything more to be done on this?

This issue is fixed. I plan to rewrite how detection is done to improve different scenarios and having some ticket open like this will remind me eventually. I have more pressing tasks on my list, but will get it done eventually. So if you want to close the issue, please go ahead.

geoand commented 2 months ago

Thanks for the update.

We can leave it open no problem.

gsmet commented 3 weeks ago

Let's close the issue, we will forget about it anyway. The issue by itself is fixed and further improvements can come in a follow-up PR.