Deadlock while calling write-locked method from read-locked method

[bh-tt]

Describe the bug

When a method with io.quarkus.arc.Lock.Type.READ lock calls a (public) method in the same class with io.quarkus.arc.Lock.Type.WRITE, a deadlock occurs.

Expected behavior

I'd expect the read lock to be released and a write lock to be acquired until the method with write lock is finished, at which point a read lock is acquired again.

As an alternative the lock interceptor might be able to detect a deadlock will occur and immediately throw if the above option is difficult to implement.

Actual behavior

A deadlock occurs, as the call to the write method waits until all read locks are released, including the lock held by the thread itself.

How to Reproduce?

Run the test in the attached gradle project.
reproducer.zip

Output of uname -a or ver

Linux devrd0 6.1.0-10-amd64 #1 SMP PREEMPT_DYNAMIC Debian 6.1.37-1 (2023-07-03) x86_64 GNU/Linux

Output of java -version

openjdk version "11.0.20" 2023-07-18 LTS OpenJDK Runtime Environment Zulu11.66+15-CA (build 11.0.20+8-LTS) OpenJDK 64-Bit Server VM Zulu11.66+15-CA (build 11.0.20+8-LTS, mixed mode)

GraalVM version (if different from Java)

No response

Quarkus version or git rev

3.2.0.Final, also happens on 2.16.8.Final

Build tool (ie. output of mvnw --version or gradlew --version)

gradle 8.0.2

Additional information

Note that when both methods have the same lock type (either read or write) everything is fine.

[quarkus-bot]

/cc @Ladicek (arc), @manovotn (arc), @mkouba (arc)

[bh-tt]

Stacktrace showing the readLock and writeLock calls:

     2023-08-01 14:17:34,275 WARN  [io.ver.cor.imp.BlockedThreadChecker] (vertx-blocked-thread-checker) Thread Thread[vert.x-worker-thread-1,5,main] has been blocked for 3266 ms, time limit is 3000 ms: io.vertx.core.VertxException: Thread blocked
        at java.base@11.0.20/jdk.internal.misc.Unsafe.park(Native Method)
        at java.base@11.0.20/java.util.concurrent.locks.LockSupport.park(LockSupport.java:194)
        at java.base@11.0.20/java.util.concurrent.locks.AbstractQueuedSynchronizer.parkAndCheckInterrupt(AbstractQueuedSynchronizer.java:885)
        at java.base@11.0.20/java.util.concurrent.locks.AbstractQueuedSynchronizer.acquireQueued(AbstractQueuedSynchronizer.java:917)
        at java.base@11.0.20/java.util.concurrent.locks.AbstractQueuedSynchronizer.acquire(AbstractQueuedSynchronizer.java:1240)
        at java.base@11.0.20/java.util.concurrent.locks.ReentrantReadWriteLock$WriteLock.lock(ReentrantReadWriteLock.java:959)
        at io.quarkus.arc.impl.LockInterceptor.writeLock(LockInterceptor.java:50)
        at io.quarkus.arc.impl.LockInterceptor.lock(LockInterceptor.java:31)
        at io.quarkus.arc.impl.LockInterceptor_Bean.intercept(Unknown Source)
        at io.quarkus.arc.impl.InterceptorInvocation.invoke(InterceptorInvocation.java:42)
        at io.quarkus.arc.impl.AroundInvokeInvocationContext.perform(AroundInvokeInvocationContext.java:30)
        at io.quarkus.arc.impl.InvocationContexts.performAroundInvoke(InvocationContexts.java:27)
        at com.example.reproducer.Endpoint_Subclass.writeLock(Unknown Source)
        at com.example.reproducer.Endpoint.readLock(Endpoint.java:14)
        at com.example.reproducer.Endpoint_Subclass.readLock$$superforward(Unknown Source)
        at com.example.reproducer.Endpoint_Subclass$$function$$2.apply(Unknown Source)
        at io.quarkus.arc.impl.AroundInvokeInvocationContext.proceed(AroundInvokeInvocationContext.java:73)
        at io.quarkus.arc.impl.AroundInvokeInvocationContext.proceed(AroundInvokeInvocationContext.java:62)
        at io.quarkus.arc.impl.LockInterceptor.readLock(LockInterceptor.java:74)
        at io.quarkus.arc.impl.LockInterceptor.lock(LockInterceptor.java:33)
        at io.quarkus.arc.impl.LockInterceptor_Bean.intercept(Unknown Source)
        at io.quarkus.arc.impl.InterceptorInvocation.invoke(InterceptorInvocation.java:42)
        at io.quarkus.arc.impl.AroundInvokeInvocationContext.perform(AroundInvokeInvocationContext.java:30)
        at io.quarkus.arc.impl.InvocationContexts.performAroundInvoke(InvocationContexts.java:27)
        at com.example.reproducer.Endpoint_Subclass.readLock(Unknown Source)
        at com.example.reproducer.Endpoint_ClientProxy.readLock(Unknown Source)
        at com.example.reproducer.Endpoint_ScheduledInvoker_readLock_214bd89eb7229b7886fbd6b07db29f23fb1a5294.invokeBean(Unknown Source)
        at io.quarkus.scheduler.common.runtime.DefaultInvoker.invoke(DefaultInvoker.java:24)
        at io.quarkus.scheduler.common.runtime.StatusEmitterInvoker.invoke(StatusEmitterInvoker.java:35)
        at io.quarkus.scheduler.common.runtime.SkipConcurrentExecutionInvoker.invoke(SkipConcurrentExecutionInvoker.java:37)
        at io.quarkus.scheduler.runtime.SimpleScheduler$ScheduledTask.doInvoke(SimpleScheduler.java:416)
        at io.quarkus.scheduler.runtime.SimpleScheduler$ScheduledTask$1.handle(SimpleScheduler.java:397)
        at io.quarkus.scheduler.runtime.SimpleScheduler$ScheduledTask$1.handle(SimpleScheduler.java:393)
        at io.vertx.core.impl.ContextBase.lambda$null$0(ContextBase.java:137)
        at io.vertx.core.impl.ContextBase$$Lambda$722/0x0000000800497040.handle(Unknown Source)
        at io.vertx.core.impl.ContextInternal.dispatch(ContextInternal.java:264)
        at io.vertx.core.impl.ContextBase.lambda$executeBlocking$1(ContextBase.java:135)
        at io.vertx.core.impl.ContextBase$$Lambda$721/0x0000000800497840.run(Unknown Source)
        at org.jboss.threads.ContextHandler$1.runWith(ContextHandler.java:18)
        at org.jboss.threads.EnhancedQueueExecutor$Task.run(EnhancedQueueExecutor.java:2513)
        at org.jboss.threads.EnhancedQueueExecutor$ThreadBody.run(EnhancedQueueExecutor.java:1512)
        at org.jboss.threads.DelegatingRunnable.run(DelegatingRunnable.java:29)
        at org.jboss.threads.ThreadLocalResettingRunnable.run(ThreadLocalResettingRunnable.java:29)
        at io.netty.util.concurrent.FastThreadLocalRunnable.run(FastThreadLocalRunnable.java:30)
        at java.base@11.0.20/java.lang.Thread.run(Thread.java:829)

[mkouba]

Yep, I was able to reproduce the problem easily...

[manovotn]

Hm, current impl doesn't really let us peek into the Lock to determine if there was a read when you attempt to get a write lock.
Furthermore, in theory you can have a chain of n method invocation, all of which acquire READ locks and then one method which need WRITE. Reshuffling read locks wouldn't be as easy in that case.

FTR, we are currently using ReentrantReadWriteLock which states (on the topic of reentrancy):

Additionally, a writer can acquire the read lock, but not vice-versa. Among other applications, reentrancy can be useful when write locks are held during calls or callbacks to methods that perform reads under read locks. If a reader tries to acquire the write lock it will never succeed.

[mkouba]

I'm not sure about the correctness but this fix based on java.util.concurrent.locks.ReentrantReadWriteLock.getReadHoldCount() seems to work.

[manovotn]

You beat me to it, I was just trying out pretty much the same thing.

I think it is correct as you only ask for hold count by current thread and renew that.
My only concern was if there were locks held by other thread(s) but those you wouldn't be able to release in the first place, so it'd behave just like before this fix.

[bh-tt]

Another thread might be able to acquire the write lock in the short span between the last read unlock and the write lock, which I think can cause concurrency issues if the read locked method expects only the method it calls to modify things and not someone else.

1. read lock method is called, read lock granted
2. read lock method calls a write lock method
3. read lock is released
4. a 2nd thread manages to acquire the write lock before the first thread locks it and makes some changes
5. the first thread does its writes, write lock method returns
6. read lock semantics are partially invalid, as a write was performed by a different thread than the thread with the read lock

[bh-tt]

Perhaps take a look at StampedLock? It does have methods for atomically upgrading a read lock to a write lock (or back), although I guess the implementation would end up a bit more complex as it does not support reentrancy.

[mkouba]

I've added a ReentrantLock to synchronize the block where we release all read locks and aquire the write lock. It's probably not 100% correct and definitely bad in terms of performance but better than the current solution 🤷.