search

mwiede / jsch

fork of the popular jsch library
Other
718 stars 133 forks source link

JSCH pins carrier threads when using virtual threads #503

Open bcluap opened 7 months ago

bcluap commented 7 months ago

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.

norrisjeremy commented 7 months ago

I object to this change. JSch uses traditional synchronized type locking in numerous places, and changing only this one location seems pointless.

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.

norrisjeremy commented 7 months ago

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.

bcluap commented 7 months ago

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

norrisjeremy commented 7 months ago

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?

bcluap commented 7 months ago

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: 1) Pass the call to the third party lib onto a platform thread pool 2) Do a PR to upstream project 3) Use an alternative library

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.106,"Thread[#42,ForkJoinPool-1-worker-2,5,CarrierThreads]" 2024-02-15 11:00:04.106, java.base/java.lang.VirtualThread$VThreadContinuation.onPinned(VirtualThread.java:183) 2024-02-15 11:00:04.106, java.base/jdk.internal.vm.Continuation.onPinned0(Continuation.java:393) 2024-02-15 11:00:04.106, java.base/java.lang.VirtualThread.tryYield(VirtualThread.java:756) 2024-02-15 11:00:04.106, java.base/java.lang.Thread.yield(Thread.java:443) 2024-02-15 11:00:04.106, com.jcraft.jsch.Session.disconnect(Session.java:2114) <== monitors:1

2024-02-15 11:00:04.025, java.base/sun.nio.ch.NioSocketImpl$1.read(NioSocketImpl.java:796) 2024-02-15 11:00:04.025, java.base/java.net.Socket$SocketInputStream.read(Socket.java:1099) 2024-02-15 11:00:04.025, com.jcraft.jsch.IO.getByte(IO.java:93) 2024-02-15 11:00:04.025, com.jcraft.jsch.Session.read(Session.java:1183) 2024-02-15 11:00:04.025, com.jcraft.jsch.UserAuthPublicKey._start(UserAuthPublicKey.java:209) 2024-02-15 11:00:04.025, com.jcraft.jsch.UserAuthPublicKey.start(UserAuthPublicKey.java:114) <== monitors:1 2024-02-15 11:00:04.025, com.jcraft.jsch.Session.connect(Session.java:480)

norrisjeremy commented 7 months ago

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.

sonarcloud[bot] commented 6 months ago

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

norrisjeremy commented 3 months ago

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.