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

To support Virtual Thread, replace synchronized with ReentrantLock. #116

Open
wants to merge 2 commits into
base: v2
Choose a base branch
from

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
@@ -229,9 +230,12 @@ public boolean refreshNow() throws InterruptedException {
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.

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.

2 participants