-
Notifications
You must be signed in to change notification settings - Fork 150
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
use ReentrantLock in LockUtil (Switch) #1092
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
lgtm |
I won't merge this yet but I'd appreciate some feedback. With Loom in mind, there doesn't appear to be a big reason not to start changing synchronized blocks to ReentrantLocks. There is a fair amount of documentation online encouraging this change. |
I think we should only do this where io is involved. The only issue of the current implementation is it introduce some allocation with the by name parameter. And a user can keep tracking where the pin happens with I have changed mostly of my $Work code, to |
I'm open to abandoning this. But if we abandon this, I think that #1089 should be changed to not be a 'good first issue'. It isn't a good first issue if it needs serious expertise to decide on which 'synchronized' blocks need to be changed. To flip this on it's head, maybe we need to get people to use Pekko with virtual threads and to report to us the pinned thread issues that they hit. |
I will back my hometown tomorrow, I will try to work on the virtual thread support. |
Completely agree with this, I am still heavily in the "lets just document how JDK 21 users can create their own VirtualThreadPool" camp rather than pushing these kind of changes in. |
@mdedetrich this change does not use any new classes - it uses existing classes. See https://softwaremill.com/what-is-blocking-in-loom/#retrofitting-blocking |
I wasn't talking about this PR but the other one with VirtualThreadPool. |
an example change for #1089