Skip to content

Conversation

@InforBG
Copy link

@InforBG InforBG commented Sep 26, 2024

Issue #115

Description of changes:
To support Virtual Thread, replace synchronized with ReentrantLock in SecretCacheObject.java.

Please see the document about virtual thread.
https://docs.oracle.com/en/java/javase/21/core/virtual-threads.html#GUID-04C03FFC-066D-4857-85B9-E5A27A875AF9

A current limitation of the implementation of virtual threads is that performing a blocking operation while inside a synchronized block or method causes the JDK's virtual thread scheduler to block a precious OS thread, whereas it wouldn't if the blocking operation were done outside of a synchronized block or method.
If these mechanisms detect places where pinning is both long-lived and frequent, replace the use of synchronized with [ReentrantLock](https://docs.oracle.com/en/java/javase/21/docs/api/java.base/java/util/concurrent/locks/ReentrantLock.html) in those particular places (again, there is no need to replace synchronized where it guards a short lived or infrequent operations).

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@InforBG InforBG requested a review from a team as a code owner September 26, 2024 04:14
@simonmarty
Copy link
Contributor

Thanks for the contribution. Looks like our internal cache implementation will also have to be updated.

Besides that, we'll need to run a load test to ensure this doesn't impact performance on Java versions < 21

@simonmarty simonmarty added the bug Something isn't working label Nov 25, 2024
long wait = this.nextRetryTime - System.currentTimeMillis();
sleep = Math.max(wait, sleep);
}
Thread.sleep(sleep);
Copy link
Contributor

Choose a reason for hiding this comment

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

I haven't had the opportunity to explore how Virtual Threads work, does the regular Thread class sleep() call still work the same way with them?

Copy link
Author

Choose a reason for hiding this comment

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

The regualar Thread.sleep() still works here. I tested it and didn't find problem.
The problem only occurs when the synchronized block has blocking operation.
You can reproduce the issue in Linux environment with the code in #115.

@InforBG
Copy link
Author

InforBG commented Nov 26, 2024

Thanks for the contribution. Looks like our internal cache implementation will also have to be updated.

Besides that, we'll need to run a load test to ensure this doesn't impact performance on Java versions < 21

I think the internal cache implementation doesn't need to be updated. They are all about the map and not blocking operation.
According to https://docs.oracle.com/en/java/javase/21/core/virtual-threads.html#GUID-04C03FFC-066D-4857-85B9-E5A27A875AF9, Guarding short-lived operations, such as in-memory operations, or infrequent ones with synchronized blocks or methods should have no adverse effect.

That's good idea to run a load test.

@ThirdEyeSqueegee ThirdEyeSqueegee linked an issue Mar 18, 2025 that may be closed by this pull request
@ThirdEyeSqueegee
Copy link
Member

The blocking behavior you're experiencing should be fixed in JDK 24 (see JEP 491). Since JDK 22 and 23 are out of support as of March 2025, we recommend upgrading to JDK 24.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

SecretCache doesn't support virtual threads in JDK 22/23 Linux

3 participants