google / google/conscrypt

Virtual thread pinning using Conscrypt with Socket APIs

Open
#1,170 7 comments 3 reactions 0 assignees View on GitHub
Dominant language
Java
Stars
1.4k
Forks
326
Avg merge
16h 22m
Merged PRs (30d)
17

Description

## Background

Conscrypt offers excellent performance, which is helpful now more than ever given the [TLS performance regressions on x86_64 in JDK-18+](https://mail.openjdk.org/pipermail/security-dev/2022-November/032757.html). Unfortunately the Conscrypt Socket stream implementations synchronize over Socket I/O pinning virtual threads to carriers as described in [jep-444](https://openjdk.org/jeps/444). This prevents Conscrypt users from taking advantage of virtual threads (in fact introduces the deadlocks as all carriers become pinned).

Snippet from a unit test where enabling virtual threads resulted in a deadlock:
```
java.base/sun.nio.ch.NioSocketImpl.read(NioSocketImpl.java:346)
java.base/sun.nio.ch.NioSocketImpl$1.read(NioSocketImpl.java:796)
java.base/java.net.Socket$SocketInputStream.read(Socket.java:1099)
org.conscrypt.ConscryptEngineSocket$SSLInputStream.readFromSocket(ConscryptEngineSocket.java:920)
org.conscrypt.ConscryptEngineSocket$SSLInputStream.processDataFromSocket(ConscryptEngineSocket.java:884)
org.conscrypt.ConscryptEngineSocket$SSLInputStream.readUntilDataAvailable(ConscryptEngineSocket.java:799)
org.conscrypt.ConscryptEngineSocket$SSLInputStream.read(ConscryptEngineSocket.java:772) <== monitors:1
org.apache.hc.core5.http.impl.io.SessionInputBufferImpl.fillBuffer(SessionInputBufferImpl.java:149)
org.apache.hc.core5.http.impl.io.SessionInputBufferImpl.readLine(SessionInputBufferImpl.java:280)
org.apache.hc.core5.http.impl.io.ChunkedInputStream.getChunkSize(ChunkedInputStream.java:261)
```

## Proposed Solution

We should be able to retain existing behavior by replacing Object monitor locks with ReentrantLocks, which do not pin virtual threads. Such a change would need to be performance tested, and may impact observability (e.g. deadlock detection, thread dump metadata).

Does this seem reasonable? Would you accept such a contribution if I were to implement it?

Thanks!

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.