-
Notifications
You must be signed in to change notification settings - Fork 144
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
JSCH pins carrier threads when using virtual threads #503
base: master
Are you sure you want to change the base?
Conversation
I object to this change. Additionally, the entire issue with carrier thread pinning is going to be fixed in a future Java release (see https://mail.openjdk.org/pipermail/loom-dev/2024-February/006433.html), thus making wholesale changes like this unnecessary. |
Also, JSch creates one or more traditional (non-virtual) threads per connection (including one of the code blocks you suggested to change to a ReentrantLock), so trying to use it in the context of a virtual thread seems questionable to me anyway. |
Hi. I appreciate your sentiments on this. I just changed to a lock instead of the synchronized block around the lock object as it was the simplest way of making the change with the least impact. I was hoping to use virtual threads with jsch before 23 or 24 but for now will wrap calls to it with with a platform thread |
Yes, I simply do not understand why Oracle chose to release Virtual threads as a non-preview feature in it's current state: the carrier thread pinning is significant impediment to safely using it for many (perhaps most?) folks, especially when interfacing with legacy code bases. I also would like to ask: did you actually encounter deadlocks that could be attributed to the carrier thread pinning that could happen when virtual threads are executing the synchronized portions of the JSch code that you proposed we change? |
Yes - entire JVM hangs and is completely non responsive. We have worked through these and have got to a point where currently we have no pinning. Basically we solved it in 1 of 2 ways:
The pinning issues we came across thus far are from dependencies we have added (we use Quarkus and its been pretty good at addressing pinning in its dependencies) :
JSCH pinning: 2024-02-15 11:00:04.025, java.base/sun.nio.ch.NioSocketImpl$1.read(NioSocketImpl.java:796) |
This only serves to prove my point that Virtual threads are simply not ready for prime-time and should not have been released as a non-preview feature in the first place until Oracle actually solves the carrier thread pinning issue within the JDK itself. |
Quality Gate passedIssues Measures |
FYI, it appears Oracle is making progress on solving the carrier thread pinning issue, see https://mail.openjdk.org/pipermail/loom-dev/2024-May/006632.html. |
FYI, it appears JEP-491 will be included as part of the OpenJDK 24 release, which should largely solve the issue with carrier thread pinning due to use of |
Virtual threads pin to carrier threads when a synchronized block has blocking code in it. We've come across 2 major occurrences of this so far. Using explicit locks is the way to resolve this.