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

Incorrect evaluation of "keep-alive-enabled" parameter in RESTEasy Reactive client #31480

Closed
gefloh opened this issue Feb 28, 2023 · 9 comments
Closed
Labels
area/rest kind/bug Something isn't working

Comments

@gefloh
Copy link
Contributor

gefloh commented Feb 28, 2023

Describe the bug

The parameter keep-alive-enabled that was added in #31125 is currently evaluated incorrectly because a default value of true is set not only for the global configuration, but also for the class name/config key values. This leads e.g. to a global value of false being overwritten by the default value of true from the classname config which is not the expected behavior.

Evaluation:

Optional<Boolean> keepAliveEnabled = oneOf(clientConfigByClassName().keepAliveEnabled,
                clientConfigByConfigKey().keepAliveEnabled, configRoot.keepAliveEnabled);

resolves always to the clientConfigByClassName value if set (which is always the case because of the default value).

I will fix this directly myself by removing the default value for keepAliveEnabled in io.quarkus.restclient.config.RestClientConfig.

Expected behavior

Setting the global configuration to false for keep-alive-enabled without any other config value provided should disable the keep alive (same for config key).

Actual behavior

The global configuration is overwritten by the incorrect default value from clientConfigByClassName (same for config key).

How to Reproduce?

No response

Output of uname -a or ver

No response

Output of java -version

No response

GraalVM version (if different from Java)

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

@gefloh gefloh added the kind/bug Something isn't working label Feb 28, 2023
@quarkus-bot
Copy link

quarkus-bot bot commented Feb 28, 2023

/cc @FroMage (resteasy-reactive), @Sgitario (resteasy-reactive), @geoand (resteasy-reactive), @stuartwdouglas (resteasy-reactive)

@gefloh
Copy link
Contributor Author

gefloh commented Feb 28, 2023

Will be fixed with #31482.

@gefloh
Copy link
Contributor Author

gefloh commented Feb 28, 2023

@TomasHofman FYI

@TomasHofman
Copy link
Contributor

Right, this wasn't obvious. I agree with the solution. Thanks for pinging me :).

@gefloh
Copy link
Contributor Author

gefloh commented Mar 1, 2023

Yes, true. Unfortunately I am still not able to test these things with my local built Quarkus version. I am not sure if the contributing guide is correct/up-to-date.

@geoand
Copy link
Contributor

geoand commented Mar 1, 2023

What kind of problems are you having?

@gefloh
Copy link
Contributor Author

gefloh commented Mar 1, 2023

I did a ./mvnw -Dquickly, this worked fine. The quarkus.platform.group-id property in the pom.xml was already set to io.quarkus, so I changed only quarkus.platform.version to 999-SNAPSHOT.

mvn clean compile gives me several compilation errors like package javax.inject does not exist. This was a freshly created Quarkus Lambda, so nothing changed there from my side. Maybe I'm missing something?

@gsmet
Copy link
Member

gsmet commented Mar 1, 2023

The current main of Quarkus has been moved to Jakarta EE 10. So you need to use the jakarta. packages instead of the javax. ones.

@gefloh
Copy link
Contributor Author

gefloh commented Mar 1, 2023

Ah yes, that makes sense. Works perfectly. Thanks!

@gefloh gefloh closed this as completed Mar 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/rest kind/bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants