Thoughts on virtual thread pinning caused by classic synchronization

[Alan Bateman]

On 10/08/2021 12:14, Thilo-Alexander Ginkel wrote:
> :
>
> 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):
> 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.

Yes, PushbackInputStream.close invokes the close on the underlying 
stream while holding a monitor so this could be problematic. The current 
PushbackInputStream.close implementation dates from JDK 1.2 and 1998 
(JDK-4017193 if you are interested) and we've been cautious about 
changing it. There is a JEP size effort needed to re-examine and remove 
most of the unspecified and inconsistent synchronization from the 
java.io classes. We've put in workaround for the most of the common 
cases and maybe we have to give PushbackInputStream more attention. 
Thanks for brining it up, I need to think about it and see how far we 
can go without changing the behavior too much that it would break 
existing code.


> :
>
> sun.nio.ch.FileChannelImpl#write(java.nio.ByteBuffer) synchronizes on 
> a positionLock, so I guess your assumption is correct:
>
This one should be okay as it's not parking. The approach taken in many 
of the file I/O operations is to temporarily increase parallelism for 
the duration of the I/O operation because it's not possible to free up 
the underlying carrier thread to do other work.

-Alan.