Closed vsevel closed 1 month ago
This issue is caused by the synchronized block in the internal implementation of the computeIfAbsent
method of the ConcurrentHashMap
that is invoked here and here.
I honestly don't know if it makes sense at all to try to fix this issue, but if so I don't think we can get rid of that ConcurrentHashMap
or at least we would need to replace it with an equivalent data structure not using synchronization. Any advice on this?
thanks for the investigation @mariofusco
I did not realize ConcurrentHashMap
was actually relying on synchronized
. for some reasons I thought it was using lock free data structures.
that said, this is an overall issue with all config sources that request data from the outside (such as vault), when triggered from a virtual thread.
one way to solve this is to replace the computeIfAbsent
by something like this:
RestClientsConfig
, and the test+load+set within that lock, still avoiding the long call as a lambda passed to computeIfAbsent
ideally I would go ahead and improve the situation. we started evaluating virtual threads on numerous java 21 applications, and we get an alarm everytime there is some thread pinning on an app deployed in integration. then we investigate. that way we hope to chase all of the issues and build a high confidence.
if we can close those issues as they come, it will be easier to get focused on new occurrences. if we have to filter out false positives, it is harder to keep focused.
and it is actually a real case of thread pinning by the book: synchronized
+ long running IO operation.WDYT @radcortez ?
(the much better solution would for ConcurrentHashMap
to stop using synchronized
)
one way to solve this is to replace the
computeIfAbsent
by something like this:
Consider that computeIfAbsent
is only the tip of the iceberg, maybe the method where the problem is more evident, especially when the generation of the value for the missing key is time consuming and/or resource intensive. However, if you look at the actual implementation of the ConcurrentHashMap
, you will see that synchronized blocks are used in many other places including the more innocent looking put
.
it is not an issue to have a synchronized
in a virtual thread, as long as what you are doing in there is fast. thread pinning happens after 20 ms. the issue is when you have a synchronized
and a potentially long running operation within, which is the case for externally loaded config sources.
so calling a put
from a virtual thread is ok, but calling coputeIfAbsent
with a lambda calling an external service is an anti-pattern.
We should reconsider how the REST Client builds / retrieves the configuration.
Usually, a config object is populated at the start of the application. This allows us to validate the configuration and ensure we have everything we need. In this case, the boundary seems blurred because CDI creates injected REST Clients before the application starts, but this creates an invisible dependency between Config and CDI, which shouldn't exist.
If we get rid of that, there is no need to keep a ConcurrentHashMap
and the Vault source shouldn't be queried during runtime.
@radcortez I'd like to help fixing this issue but
this creates an invisible dependency between Config and CDI, which shouldn't exist.
it is not cleat at all to me how this is happening. Are you aware of any integration test reproducing this behavior or could you give me a few hints on how to develop a reproducer on my own?
Yes, check here:
These are the CDI parts that shouldn't be there:
Here the configuration is being created manually:
What we should do is remove any manual configuration creation and let the config system handle it. If we need to access the configuration values, we should use ConfigProvider.getConfig().unwrap(SmallRyeConfig.class).getConfigMapping(Class)
. The configuration class for the rest of the clients has to be migrated to use @ConfigMapping
.
The pinned event is troublesome not because of the sycnhronized
but the inner wait on the count down latch: I suggest to fix in 3 lines the pinning problems and address by taking the right time to do it right, the problem shown by @radcortez
Yes, it will take a while to redo the REST Client configuration.
I believe this issue can be closed since it has been structurally fixed with this commit https://github.com/quarkusio/quarkus/commit/4f31e0d923853fe15fc1da7664e52e2874a8b48b
I agree. we will reopen if we see new occurences after we upgrade. thanks for the effort @radcortez and @mariofusco
Describe the bug
I detected the following thread pinning:
we have not identified any native blocking calls in our business code. what could cause this pinning? I tried creating a reproducer, with no luck. may be that is an issue with the vault extension, but I do not see any native call. or in the business code? how can we investigate this issue?
Expected behavior
No response
Actual behavior
No response
How to Reproduce?
No response
Output of
uname -a
orver
No response
Output of
java -version
No response
Quarkus version or git rev
3.9.5
Build tool (ie. output of
mvnw --version
orgradlew --version
)No response
Additional information
No response