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

Avoid virtual thread pinning in RestClientsConfig #41926

Closed
wants to merge 1 commit into from

Conversation

mariofusco
Copy link
Contributor

This is a fix for the virtual thread pinning problem reported here.

As reported by @radcortez here the more structural fix there would be removing the implicit CDI dependency thus completely getting rid of the ConcurrentHashMap. However, as he also confirmed, a rewriting of the REST Client configuration could take a while, so I followed @franz1981's suggestion and fixed at least the symptom simply avoiding to call the computeIfAbsent method from a virtual thread.

@quarkus-bot quarkus-bot bot added area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins area/resteasy-classic labels Jul 16, 2024
Copy link
Contributor

@franz1981 franz1981 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would avoid creating a new class (which implies a new class loaded) and using a single lock for all of them (even if virtual)...
given that the stack trace reported by the user is performing some blocking/long running operation, it means that using a single lock will serialize all of the them regardless what the "good" CHM concurrency allow...
Why not using a CompletableFuture value instead without caring if the caller is a virtual one?
It will reuse CHM and won't serialize the computes...

@mariofusco
Copy link
Contributor Author

Why not using a CompletableFuture value instead without caring if the caller is a virtual one? It will reuse CHM and won't serialize the computes...

Do you mean having a ConcurrentHashMap<String, CompletableFuture<RestClientConfig>> ?

@franz1981
Copy link
Contributor

Yep, clearly it would add some cost in term of footprint to everyone (non virtual threads), but there won't be any other adverse effects

@mariofusco
Copy link
Contributor Author

Made obsolete and unnecessary by 4f31e0d

@mariofusco mariofusco closed this Aug 2, 2024
@quarkus-bot quarkus-bot bot added the triage/invalid This doesn't seem right label Aug 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins area/resteasy-classic triage/invalid This doesn't seem right
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants