From a59045a008a661332525fcc0d4e170fe75235603 Mon Sep 17 00:00:00 2001 From: JingZhang Chen Date: Fri, 16 Aug 2024 11:39:34 +0800 Subject: [PATCH 1/3] fix:deadlock when reentrant exclusive lock #2905 --- .../io/lettuce/core/protocol/SharedLock.java | 11 ++++- .../lettuce/core/protocol/SharedLockTest.java | 42 +++++++++++++++++++ 2 files changed, 52 insertions(+), 1 deletion(-) create mode 100644 src/test/java/io/lettuce/core/protocol/SharedLockTest.java diff --git a/src/main/java/io/lettuce/core/protocol/SharedLock.java b/src/main/java/io/lettuce/core/protocol/SharedLock.java index 13a9cb8cfe..a99f153d0e 100644 --- a/src/main/java/io/lettuce/core/protocol/SharedLock.java +++ b/src/main/java/io/lettuce/core/protocol/SharedLock.java @@ -26,6 +26,8 @@ class SharedLock { private final Lock lock = new ReentrantLock(); + private final ThreadLocal sharedCnt = ThreadLocal.withInitial(() -> 0); + private volatile long writers = 0; private volatile Thread exclusiveLockOwner; @@ -45,6 +47,7 @@ void incrementWriters() { if (WRITERS.get(this) >= 0) { WRITERS.incrementAndGet(this); + sharedCnt.set(sharedCnt.get() + 1); return; } } @@ -63,6 +66,7 @@ void decrementWriters() { } WRITERS.decrementAndGet(this); + sharedCnt.set(sharedCnt.get() - 1); } /** @@ -125,6 +129,11 @@ private void lockWritersExclusive() { exclusiveLockOwner = Thread.currentThread(); return; } + // reentrant exclusive lock + if (WRITERS.compareAndSet(this, sharedCnt.get(), -1)) { + exclusiveLockOwner = Thread.currentThread(); + return; + } } } finally { lock.unlock(); @@ -137,7 +146,7 @@ private void lockWritersExclusive() { private void unlockWritersExclusive() { if (exclusiveLockOwner == Thread.currentThread()) { - if (WRITERS.incrementAndGet(this) == 0) { + if (WRITERS.compareAndSet(this, -1, sharedCnt.get())) { exclusiveLockOwner = null; } } diff --git a/src/test/java/io/lettuce/core/protocol/SharedLockTest.java b/src/test/java/io/lettuce/core/protocol/SharedLockTest.java new file mode 100644 index 0000000000..3dda418e66 --- /dev/null +++ b/src/test/java/io/lettuce/core/protocol/SharedLockTest.java @@ -0,0 +1,42 @@ +package io.lettuce.core.protocol; + +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; + +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; + +public class SharedLockTest { + + @Test + public void safety_on_reentrant_lock_exclusive_on_writers() throws InterruptedException { + final SharedLock sharedLock = new SharedLock(); + CountDownLatch cnt = new CountDownLatch(1); + try { + sharedLock.incrementWriters(); + + String result = sharedLock.doExclusive(() -> { + return sharedLock.doExclusive(() -> { + return "ok"; + }); + }); + if ("ok".equals(result)) { + cnt.countDown(); + } + } finally { + sharedLock.decrementWriters(); + } + + cnt.await(1, TimeUnit.SECONDS); + + // verify writers won't be negative after finally decrementWriters + String result = sharedLock.doExclusive(() -> { + return sharedLock.doExclusive(() -> { + return "ok"; + }); + }); + + Assertions.assertEquals("ok", result); + } + +} From c3db27f7be3b66045062c1286e03f80059a711d3 Mon Sep 17 00:00:00 2001 From: JingZhang Chen Date: Fri, 16 Aug 2024 18:15:02 +0800 Subject: [PATCH 2/3] confirm won't blocking other thread --- .../io/lettuce/core/protocol/SharedLock.java | 4 ++++ .../lettuce/core/protocol/SharedLockTest.java | 17 ++++++++++++++++- 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/src/main/java/io/lettuce/core/protocol/SharedLock.java b/src/main/java/io/lettuce/core/protocol/SharedLock.java index a99f153d0e..f2e90186ee 100644 --- a/src/main/java/io/lettuce/core/protocol/SharedLock.java +++ b/src/main/java/io/lettuce/core/protocol/SharedLock.java @@ -146,9 +146,13 @@ private void lockWritersExclusive() { private void unlockWritersExclusive() { if (exclusiveLockOwner == Thread.currentThread()) { + // check exclusive look not reentrant first if (WRITERS.compareAndSet(this, -1, sharedCnt.get())) { exclusiveLockOwner = null; + return; } + // otherwise unlock until no more reentrant left + WRITERS.incrementAndGet(this); } } diff --git a/src/test/java/io/lettuce/core/protocol/SharedLockTest.java b/src/test/java/io/lettuce/core/protocol/SharedLockTest.java index 3dda418e66..a10e712ae4 100644 --- a/src/test/java/io/lettuce/core/protocol/SharedLockTest.java +++ b/src/test/java/io/lettuce/core/protocol/SharedLockTest.java @@ -27,7 +27,8 @@ public void safety_on_reentrant_lock_exclusive_on_writers() throws InterruptedEx sharedLock.decrementWriters(); } - cnt.await(1, TimeUnit.SECONDS); + boolean await = cnt.await(1, TimeUnit.SECONDS); + Assertions.assertTrue(await); // verify writers won't be negative after finally decrementWriters String result = sharedLock.doExclusive(() -> { @@ -37,6 +38,20 @@ public void safety_on_reentrant_lock_exclusive_on_writers() throws InterruptedEx }); Assertions.assertEquals("ok", result); + + // and other writers should be passed after exclusive lock released + CountDownLatch cntOtherThread = new CountDownLatch(1); + new Thread(() -> { + try { + sharedLock.incrementWriters(); + cntOtherThread.countDown(); + } finally { + sharedLock.decrementWriters(); + } + }).start(); + + await = cntOtherThread.await(1, TimeUnit.SECONDS); + Assertions.assertTrue(await); } } From 6b66efb61a111c03a892150884ee87836d87ac2a Mon Sep 17 00:00:00 2001 From: JingZhang Chen Date: Sat, 31 Aug 2024 04:29:02 +0800 Subject: [PATCH 3/3] apply suggestions --- .../io/lettuce/core/protocol/SharedLock.java | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/src/main/java/io/lettuce/core/protocol/SharedLock.java b/src/main/java/io/lettuce/core/protocol/SharedLock.java index f2e90186ee..c3ad425c16 100644 --- a/src/main/java/io/lettuce/core/protocol/SharedLock.java +++ b/src/main/java/io/lettuce/core/protocol/SharedLock.java @@ -26,7 +26,7 @@ class SharedLock { private final Lock lock = new ReentrantLock(); - private final ThreadLocal sharedCnt = ThreadLocal.withInitial(() -> 0); + private final ThreadLocal threadWriters = ThreadLocal.withInitial(() -> 0); private volatile long writers = 0; @@ -47,7 +47,7 @@ void incrementWriters() { if (WRITERS.get(this) >= 0) { WRITERS.incrementAndGet(this); - sharedCnt.set(sharedCnt.get() + 1); + threadWriters.set(threadWriters.get() + 1); return; } } @@ -66,7 +66,7 @@ void decrementWriters() { } WRITERS.decrementAndGet(this); - sharedCnt.set(sharedCnt.get() - 1); + threadWriters.set(threadWriters.get() - 1); } /** @@ -125,12 +125,8 @@ private void lockWritersExclusive() { try { for (;;) { - if (WRITERS.compareAndSet(this, 0, -1)) { - exclusiveLockOwner = Thread.currentThread(); - return; - } - // reentrant exclusive lock - if (WRITERS.compareAndSet(this, sharedCnt.get(), -1)) { + // allow reentrant exclusive lock by comparing writers count and threadWriters count + if (WRITERS.compareAndSet(this, threadWriters.get(), -1)) { exclusiveLockOwner = Thread.currentThread(); return; } @@ -147,7 +143,7 @@ private void unlockWritersExclusive() { if (exclusiveLockOwner == Thread.currentThread()) { // check exclusive look not reentrant first - if (WRITERS.compareAndSet(this, -1, sharedCnt.get())) { + if (WRITERS.compareAndSet(this, -1, threadWriters.get())) { exclusiveLockOwner = null; return; }