Thoughts on virtual thread pinning caused by classic synchronization

Tue Aug 10 11:14:02 UTC 2021

Hello Alan,

thanks for your reply!

On Sun, Aug 8, 2021 at 3:35 PM Alan Bateman <Alan.Bateman at oracle.com> wrote:

> The java.io APIs are problematic as they have used synchronization since
> JDK 1.0/1.1. In most cases the synchronization is not specified and not
> enforced consistently. In other cases there is a lock object exposed as
> a protected field for sub-classes. In general, most of these APIs should
> not need synchronization but there are a few exceptions (PrintStream
> intended to be written too by concurrent threads, async close, cases
> where the underlying transport is packet based, and a few
> security-sensitive scenarios where corrupting the stream is a concern).
> So we have to proceed with caution. The approach that we've taken is to
> use j.u.c. locks when not sub-classed or when sub-classes by trusted/JDK
> classes. There is still a risk that there is code in the wild that makes
> assumptions on how synchronizes on the stream but I don't think we can
> do anything about that. At some point, and probably over several
> releases, we will need to address the technical debt in this area and
> gradually remove the often-broken and unspecified synchronization.
>
> You mentioned PushbackInputStream and you shouldn't have an issue there,
> unless you are wrapping something that is using synchronization.

Actually, for PushbackInputStream we see thread pinning when the wrapped
InputStream from Apache HttpComponents 5 is being closed and wants to
return the underlying connection to the connection pool (which may block):

"ForkJoinPool-1-worker-2" #87 [91] daemon prio=5 os_prio=0 cpu=164.20ms
elapsed=109.16s tid=0x00007fa8540119b0 nid=0x5b waiting on condition
 [0x00007fa832cea000]
   java.lang.Thread.State: WAITING (parking)
        at jdk.internal.misc.Unsafe.park(java.base at 17-loom/Native Method)
        - parking to wait for  <0x000010001e990c68> (a
java.util.concurrent.locks.AbstractQueuedSynchronizer$ConditionObject)
        at java.util.concurrent.locks.LockSupport.park(java.base at 17-loom
/LockSupport.java:371)
        at
java.util.concurrent.locks.AbstractQueuedSynchronizer$ConditionNode.block(java.base at 17-loom
/AbstractQueuedSynchronizer.java:506)
        at
java.util.concurrent.ForkJoinPool.compensatedBlock(java.base at 17-loom
/ForkJoinPool.java:3456)
        at java.util.concurrent.ForkJoinPool.managedBlock(java.base at 17-loom
/ForkJoinPool.java:3439)
        at
java.util.concurrent.locks.AbstractQueuedSynchronizer$ConditionObject.await(java.base at 17-loom
/AbstractQueuedSynchronizer.java:1623)
        at java.lang.VirtualThread.parkOnCarrierThread(java.base at 17-loom
/VirtualThread.java:469)
        at java.lang.VirtualThread.yieldContinuation(java.base at 17-loom
/VirtualThread.java:379)
        at java.lang.VirtualThread.park(java.base at 17-loom
/VirtualThread.java:534)
        at java.lang.System$2.parkVirtualThread(java.base at 17-loom
/System.java:2373)
        at jdk.internal.misc.VirtualThreads.park(java.base at 17-loom
/VirtualThreads.java:60)
        at java.util.concurrent.locks.LockSupport.park(java.base at 17-loom
/LockSupport.java:219)
        at
java.util.concurrent.locks.AbstractQueuedSynchronizer.acquire(java.base at 17-loom
/AbstractQueuedSynchronizer.java:715)
        at
java.util.concurrent.locks.AbstractQueuedSynchronizer.acquire(java.base at 17-loom
/AbstractQueuedSynchronizer.java:938)
        at
java.util.concurrent.locks.ReentrantLock$Sync.lock(java.base at 17-loom
/ReentrantLock.java:153)
        at java.util.concurrent.locks.ReentrantLock.lock(java.base at 17-loom
/ReentrantLock.java:322)
        at
org.apache.hc.core5.pool.StrictConnPool.release(StrictConnPool.java:238)
        at
org.apache.hc.client5.http.impl.io.PoolingHttpClientConnectionManager.release(PoolingHttpClientConnectionManager.java:383)
        at
org.apache.hc.client5.http.impl.classic.InternalExecRuntime.releaseEndpoint(InternalExecRuntime.java:261)
        at
org.apache.hc.client5.http.impl.classic.ResponseEntityProxy.releaseConnection(ResponseEntityProxy.java:80)
        at
org.apache.hc.client5.http.impl.classic.ResponseEntityProxy.streamClosed(ResponseEntityProxy.java:137)
        at
org.apache.hc.core5.http.io.EofSensorInputStream.checkClose(EofSensorInputStream.java:228)
        at
org.apache.hc.core5.http.io.EofSensorInputStream.close(EofSensorInputStream.java:172)
        at
com.example.ApacheHttpClientDownloadHandler$ResponseClosingInputStream.close(ApacheHttpClientDownloadHandler.java:91)
        at org.apache.commons.io.IOUtils.close(IOUtils.java:403)
        at
org.apache.commons.io.input.ProxyInputStream.close(ProxyInputStream.java:148)
        at java.io.PushbackInputStream.close(java.base at 17-loom
/PushbackInputStream.java:377)
        - locked <0x0000105fffb0e3c0> (a java.io.PushbackInputStream)
        at
org.apache.commons.compress.archivers.zip.ZipArchiveInputStream.close(ZipArchiveInputStream.java:673)
[...]

This is using a patched HttpComponents version that replaces synchronized
with ReetrantLock (hoping to eliminate all synchronized blocks on code
paths discovered during our tests) - the original version
of org.apache.hc.core5.pool.StrictConnPool.release is synchronized.

I guess the PushbackInputStream is one of those cases that can't be easily
migrated due to the synchronized object ("this") being accessible by
subclasses.

You also mentioned FileChannel - if you have concurrent threads accessing a
> file channel then they will probably be using the positional read/write
> methods that are designed to support concurrent access to different
> parts of a file.
>

sun.nio.ch.FileChannelImpl#write(java.nio.ByteBuffer) synchronizes on a
positionLock, so I guess your assumption is correct:

"ForkJoinPool-1-worker-42" #39299 [137] daemon prio=5 os_prio=0
cpu=2867.67ms elapsed=25.18s tid=0x00007f9f8909fb30 nid=0x89 runnable
 [0x00007f9ee64eb000]
   java.lang.Thread.State: RUNNABLE
        at jdk.internal.misc.Unsafe.unpark(java.base at 17-loom/Native Method)
        - parking to wait for  <0x0000105ffbe00180> (a
java.util.concurrent.ForkJoinPool)
        at java.util.concurrent.locks.LockSupport.unpark(java.base at 17-loom
/LockSupport.java:181)
        at java.util.concurrent.ForkJoinPool.tryCompensate(java.base at 17-loom
/ForkJoinPool.java:1791)
        at
java.util.concurrent.ForkJoinPool.compensatedBlock(java.base at 17-loom
/ForkJoinPool.java:3453)
        at
java.lang.invoke.LambdaForm$DMH/0x0000000801001400.invokeSpecial(java.base at 17-loom
/LambdaForm$DMH)
        at
java.lang.invoke.LambdaForm$MH/0x0000000801934000.invoke_MT(java.base at 17-loom
/LambdaForm$MH)
        at jdk.internal.misc.Blocker.managedBlock(java.base at 17-loom
/Blocker.java:159)
        at sun.nio.ch.FileChannelImpl.write(java.base at 17-loom
/FileChannelImpl.java:296)
        - locked <0x0000100148580948> (a java.lang.Object)
        at java.nio.channels.Channels.writeFullyImpl(java.base at 17-loom
/Channels.java:74)
        at java.nio.channels.Channels.writeFully(java.base at 17-loom
/Channels.java:96)
        at java.nio.channels.Channels$1.write(java.base at 17-loom
/Channels.java:171)
        - locked <0x00001001485809d0> (a java.nio.channels.Channels$1)
        at org.apache.commons.io.IOUtils.copyLarge(IOUtils.java:1310)
        at org.apache.commons.io.IOUtils.copy(IOUtils.java:978)
        at org.apache.commons.io.IOUtils.copyLarge(IOUtils.java:1282)
        at org.apache.commons.io.IOUtils.copy(IOUtils.java:953)
[...]

For Channels, it's an anonymous inner class.

Both could be converted to ReentrantLocks, as they are not accessible via
inheritance and thus not part of a contract, right?

What would be your recommendation? Wait until synchronized support is in
place or try to migrate problematic libraries to ReentrantLock? I
understand that the latter may not be feasible for some JDK classes due to
possible contract violations...

Thanks,
Thilo