Replace synchronized blocks with a ReentrantLock.

[janickr]

Summary

Replace synchronized blocks with a ReentrantLock, avoiding virtual threads pinning the carrier thread during IO.

Description

The synchronized blocks were removed and replaced by lock.lock() and lock.unlock(). This avoids that the carrier thread (OS thread) is pinned when running on virtual threads which were introduced as a preview feature in JDK 19.

The code inside the replaced synchronized blocks in ConnectionProxy performed IO, pinning the carrier thread for at least the duration of the IO operation.

https://openjdk.org/jeps/425
`There are two scenarios in which a virtual thread cannot be unmounted during blocking operations because it is pinned to its carrier:

When it executes code inside a synchronized block or method, or When it executes a native method or a foreign function.`

[karenc-bq]

Hi @janickr, thank you for the PR, could you please rebase the branch again?

[janickr]

Hi @karenc-bq ,

I looked at this, but the merge conflicts are not easy to solve, the locking changed significantly. Instead of using the object monitor of the ConnectionProxy instance and JdbcInterfaceProxy, it now synchronizes on the actual JdbcConnection and objects implementing the JDBC api. Is this an intended change? I don't understand the reasoning behind it, doesn't this increase the risk of lock contention?

To be fair I'm not sure why the synchronization/locking is needed in this class, so maybe I'm not seeing something.
Any ideas on this?

[karenc-bq]

Hi @janickr,

This method invokes a JDBC method on the JdbcConnection object, which is why the method is synchronized on the actual object. If we were to replace the synchronized blocks with reentrant locks, we would need to update all synchronized blocks on the JdbcConnection as well for consistency. This would affect community code.

Seeing that there is already a relevant PR replacing the synchronized blocks for the community code mysql/mysql-connector-j#95, we will postpone these changes on this driver until the changes have been introduced into the community code to avoid duplicated work and merge conflicts.

We will close this PR for now and create a backlog ticket for this update. Thank you for raising this.

[janickr]

Hi @karenc-bq,

Thank you for the answer! I also worked on of the PRs for the community code. The PR will not cover the synchronized blocks in this class though but it's ok to close this PR because the conflicts with main cannot be resolved easily. This issue will probably need to be addressed when jdk 21 is released later this year.

I'm still curious about the synchronization going on in the ConnectionProxy class. I see 2 issues here:

1. Calling a method on the JdbcConnection object is not enough justification for the synchronize blocks I think. Why does the ConnectionProxy bear the responsibility to synchronize access to the JdbcConnection? It cannot make a JdbcConnection thread-safe if it isn't already. So if the synchronized blocks are not there for the JdbcConnection, but for the PluginManager, why not make the pluginmanager (or individual plugins) thread-safe thus limiting the scope of the locks (maybe no locking at al is needed)
2. If I'm mistaken about (1) and the synchronization is needed in ConnectionProxy, why synchronize on objects that are outside the control of the ConnectionProxy? Anyone could synchronize on a JdbcConnection and block execution in the ConnectionProxy, wouldn't that increase the risk of deadlocks? So in case the locking is still needed, I think it's still better to have the lock local to the ConnectionProxy (as it was before)

Kind regards!