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

JSCH pins carrier threads when using virtual threads #503

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bcluap
Copy link

@bcluap bcluap commented Feb 15, 2024

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Author

bcluap commented Feb 16, 2024

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
Copy link
Contributor

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
Copy link
Author

bcluap commented Feb 16, 2024

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) :

  • Jetty websocket client
  • Hazelcast startup and shutdown API's
  • Apache commons email
  • AWS KMS sdk
  • Azure document analysis sdk
  • AWS recognition sdk
  • Jsch
  • Apache CXF
  • MySQL driver (We changed to MariaDB driver and it does not pin)

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
Copy link
Contributor

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.

Copy link

sonarcloud bot commented Mar 15, 2024

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
Copy link
Contributor

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.

@norrisjeremy
Copy link
Contributor

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 synchronize.
As such, I believe this PR can be closed now, as it should be unnecessary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants