Avoid virtual thread pinning in RestClientsConfig #41948
Conversation
| @@ -327,19 +328,27 @@ public RestClientConfig getClientConfig(String configKey) { | |||
| if (configKey == null) { | |||
| return RestClientConfig.EMPTY; | |||
| } | |||
| return configs.computeIfAbsent(configKey, RestClientConfig::load); | |||
| CompletableFuture<RestClientConfig> futureConfig = configs.computeIfAbsent(configKey, name -> new CompletableFuture<>()); | |||
| if (!futureConfig.isDone()) { | |||
There was a problem hiding this comment.
This won't work as expected.
You need to make sure that if there is competition, the load is performed just once; while here you risk 2 threads to try complete whatever they found or compute in the CHM.
Using a mix of get and putIfAbsent comparing on what is found would work better, unless you use a capturing lambda which make it possible to compare the computeIfAbsent result with the submitted one
There was a problem hiding this comment.
I agree that it is possible that load could be performed more than once by concurrent threads (which is indeed suboptimal), but in this case the CompletableFuture will act as a sort of AtomicReference and only the first thread completing the config loading will win and set that value inside the CompletableFuture.
the only way to to make sure they who performed the call has been the race to set the completable future as first, while here you risk 2 threads to try complete whatever they found or compute in the CHM
Sorry, but I'm not understanding what you're suggesting here. Can you please clarify, possibly with a practical example, how I could avoid that 2 threads could concurrently call the load method?
There was a problem hiding this comment.
The thing is that we shouldn't introduce unwanted behaviour from what it was before, or we risk to introduce bugs because, being concurrent is not always due that is well tested/have coverage.
My suggestion is to preserve the original load behaviour and to do it you need both some atomic way to set the completable future (which is trivial) and be aware that we have successfully published it/or not.
There are different ways to do it but I have no computer on my hand now to give you a detailed example.
Before CHM::computeIfAbsent was a thing it was used a fast path get and a slow path putIfAbsent which create a completable future upfront (not completed yet): iirc part of the putIfAbsent contract is the ability to verify if it has landed (or there must be another method which allow to do it, for sure) and who won the race to set it, will perform the load and complete it, who loose will just perform a waiting get. I hope it helps to clarify
There was a problem hiding this comment.
@franz1981 thanks for clarification, I wasn't thinking to use the putIfAbsent to make sure that the load is performed only once. I modified this pull request according to your suggestion, please review again.
@radcortez feedback is welcome.
|
For me this looks very weird and would prefer #41926 |
I agree that this looks weird at a first sight (and as I wrote I'm not strongly opinionated between these 2 implementations), but I adopted a similar solution when I made the |
|
Sure yeah, I'm not arguing that the impl is not good, I am just saying that it looks very weird for someone who has been used to the Quarkus config system |
|
It should be temporary :) |
|
Yep and at the same time; it is pushing further the idea that maybe it needs some concurrent structure...while maybe it doesn't need it. |
|
Please please please make it actually temporary :) Asking because I am the one that ends up with almost all the REST Client issues (not by design) |
This comment has been minimized.
This comment has been minimized.
|
Made obsolete and unnecessary by 4f31e0d |
This is the alternative solution suggested by @franz1981 for the problem reported here. I see pros and cons in both implementations, so I'm not strongly opinionated on any of the 2.
Please let me know which one of the 2 you want to move forward and I will close the other. /cc @radcortez