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

Virtual thread support for sync api's #3085

Open
luvk1412 opened this issue Dec 17, 2024 · 3 comments · May be fixed by #3116
Open

Virtual thread support for sync api's #3085

luvk1412 opened this issue Dec 17, 2024 · 3 comments · May be fixed by #3116
Labels
type: improvement An improvement to the existing implementation

Comments

@luvk1412
Copy link

Feature Request

Is your feature request related to a problem? Please describe

I am writing a server which would run on virtual threads and i am also using lettuce sync api's in my app. On a quick search i can see there are usages of synchronized in the code ( check here ), I am worried that these usages can lead to platform thread pinning especially the usages in MasterReplicaConnectionProvider.java, I am not sure about the fact if platform thread would pin, as i am not aware if the methods using synchronized would run on eventloop or the virtual thread in some case, so please correct me if I am wrong here.
In case these usages are potential cause for thread pinning, it would be great if we can remove those usages and make library virtual thread compatible.

Describe the solution you'd like

Replace synchronized with ReentrantLock as done in spring-projects/spring-data-redis#2690 as well.

@tishun tishun added for: team-attention An issue we need to discuss as a team to make progress status: waiting-for-triage labels Dec 20, 2024
@tishun
Copy link
Collaborator

tishun commented Dec 31, 2024

Hey @luvk1412 thanks for submitting this.

@mp911de already investigated this as part of #1429
As @mp911de mentioned in #2470 (comment) and is evident by netty/netty#12816 Netty does not currently support virtual threads in the EventLoop

I do, however, think we can address some of the places in this list, even if it is for future proofing.

I will keep this open and submit a few fixes. Please let me know if you run into specific instances of this issue.

@tishun tishun added type: improvement An improvement to the existing implementation and removed for: team-attention An issue we need to discuss as a team to make progress status: waiting-for-triage labels Dec 31, 2024
@tishun tishun linked a pull request Dec 31, 2024 that will close this issue
4 tasks
@luvk1412
Copy link
Author

luvk1412 commented Dec 31, 2024

@tishun thanks for replying, my concern wasn't around that netty's eventloop should use virtual threads, its totally fine if eventloop is using platform threads. My concern was more around if any of the mentioned synchronized usage was reachable by virtual threads while one is using sync api's of lettuce. As long as all these synchronized usages are only accessible by eventloop threads, and a virtual thread is only doing something like future.get() ultimately in sync api's, it should be totally fine ig, but i wasn't sure if this is the case as i mentioned in my post at

as i am not aware if the methods using synchronized would run on eventloop or the virtual thread in some case

Also came across JEP 491 after which maybe this is not a concern anymore as well.

@tishun
Copy link
Collaborator

tishun commented Dec 31, 2024

Also came across JEP 491 after which maybe this is not a concern anymore as well.

This JEP seems to be scheduled for JDK 24. JDK 21 to 23 would still have virtual threads that pin threads in case of synchronized blocks so the change seems reasonable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: improvement An improvement to the existing implementation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants