Skip to content

Commit f492f0d

Browse files
authored
[BugFix] Fix LockManager release not notify all waiters that meet the conditions (backport #54922) (#54952)
(cherry picked from commit 1bd5937)
1 parent 09cf229 commit f492f0d

File tree

4 files changed

+124
-27
lines changed

4 files changed

+124
-27
lines changed

fe/fe-core/src/main/java/com/starrocks/common/util/concurrent/lock/LockManager.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -305,7 +305,7 @@ public boolean isOwner(long rid, Locker locker, LockType lockType) {
305305
}
306306
}
307307

308-
public boolean isOwnerInternal(long rid, Locker locker, LockType lockType, int lockTableIndex) {
308+
private boolean isOwnerInternal(long rid, Locker locker, LockType lockType, int lockTableIndex) {
309309
final Map<Long, Lock> lockTable = lockTables[lockTableIndex];
310310
final Lock lock = lockTable.get(rid);
311311
return lock != null && lock.isOwner(locker, lockType);

fe/fe-core/src/main/java/com/starrocks/common/util/concurrent/lock/MultiUserLock.java

Lines changed: 15 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,7 @@ public MultiUserLock(LockHolder lockHolder) {
4848
@Override
4949
public LockGrantType lock(Locker locker, LockType lockType) throws LockException {
5050
LockHolder lockHolderRequest = new LockHolder(locker, lockType);
51-
LockGrantType lockGrantType = tryLock(lockHolderRequest);
51+
LockGrantType lockGrantType = tryLock(lockHolderRequest, waiterNum() == 0);
5252
if (lockGrantType == LockGrantType.NEW) {
5353
addOwner(lockHolderRequest);
5454
} else if (lockGrantType == LockGrantType.WAIT) {
@@ -58,7 +58,17 @@ public LockGrantType lock(Locker locker, LockType lockType) throws LockException
5858
return lockGrantType;
5959
}
6060

61-
private LockGrantType tryLock(LockHolder lockHolderRequest) throws LockException {
61+
/**
62+
* @param noWaiters indicates whether there are other waiters. This will determine whether the lock
63+
* can be directly acquired. If there are other waiters, the current locker cannot jump in
64+
* line to acquire the lock first. A special scenario is to notify waiters in the
65+
* existing wait list during release. At this time, the wait list needs to be ignored and
66+
* as many waiters as possible need to be awakened.
67+
* @return LockGrantType.NEW means that the lock ownership can be obtained.
68+
* LockGrantType.EXISTING means that the current lock already exists and needs to be re-entered.
69+
* LockGrantType.WAIT means that there is a lock conflict with the current owner and it is necessary to wait.
70+
*/
71+
private LockGrantType tryLock(LockHolder lockHolderRequest, boolean noWaiters) throws LockException {
6272
if (ownerNum() == 0) {
6373
return LockGrantType.NEW;
6474
}
@@ -131,7 +141,7 @@ private LockGrantType tryLock(LockHolder lockHolderRequest) throws LockException
131141
}
132142
}
133143

134-
if (!hasConflicts && (hasSameLockerWithDifferentLockType || waiterNum() == 0)) {
144+
if (!hasConflicts && (hasSameLockerWithDifferentLockType || noWaiters)) {
135145
return LockGrantType.NEW;
136146
} else {
137147
return LockGrantType.WAIT;
@@ -205,7 +215,7 @@ public Set<Locker> release(Locker locker, LockType lockType) throws LockExceptio
205215
}
206216

207217
while (lockWaiter != null) {
208-
LockGrantType lockGrantType = tryLock(lockWaiter);
218+
LockGrantType lockGrantType = tryLock(lockWaiter, true);
209219

210220
if (lockGrantType == LockGrantType.NEW
211221
|| lockGrantType == LockGrantType.EXISTING) {
@@ -227,6 +237,7 @@ public Set<Locker> release(Locker locker, LockType lockType) throws LockExceptio
227237

228238
if (lockWaiterIterator != null && lockWaiterIterator.hasNext()) {
229239
lockWaiter = lockWaiterIterator.next();
240+
isFirstWaiter = false;
230241
} else {
231242
break;
232243
}

fe/fe-core/src/test/java/com/starrocks/common/lock/LockManagerAllLockModesRandomTest.java

Lines changed: 42 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,10 @@
2424
import java.util.ArrayList;
2525
import java.util.List;
2626
import java.util.Random;
27+
import java.util.concurrent.ExecutionException;
28+
import java.util.concurrent.ExecutorService;
29+
import java.util.concurrent.Executors;
30+
import java.util.concurrent.Future;
2731
import java.util.concurrent.atomic.AtomicBoolean;
2832

2933
/**
@@ -34,20 +38,20 @@
3438
* - Each TestDBResource starts a group of threads (16 in total) for concurrent testing,
3539
* with the following responsibilities:
3640
* <p>
37-
* - 5 threads perform concurrent write operations. Each thread randomly selects a table and executes
38-
* 1 million updateOneRandomTable operations on that table. Acquiring the intent write lock on the db and
39-
* then the write lock on the table is required before updating the table.
41+
* - 5 threads perform concurrent write operations. Each thread randomly selects a table and executes
42+
* 1 million updateOneRandomTable operations on that table. Acquiring the intent write lock on the db and
43+
* then the write lock on the table is required before updating the table.
4044
* <p>
41-
* - 5 threads perform concurrent write operations. Each thread executes 1 million updateAllTables
42-
* operations on the db. Acquiring the write lock on the db is required before executing this action.
45+
* - 5 threads perform concurrent write operations. Each thread executes 1 million updateAllTables
46+
* operations on the db. Acquiring the write lock on the db is required before executing this action.
4347
* <p>
44-
* - 3 threads perform concurrent read operations. Each thread acquires a read lock on the db,
45-
* then randomly selects a table and verifies whether the two counters of that table are equal.
46-
* If not, Assert.assertEquals() fails.
48+
* - 3 threads perform concurrent read operations. Each thread acquires a read lock on the db,
49+
* then randomly selects a table and verifies whether the two counters of that table are equal.
50+
* If not, Assert.assertEquals() fails.
4751
* <p>
48-
* - 3 threads perform concurrent read operations. Each thread acquires an intent read lock on the db and
49-
* a read lock on the table. It then verifies whether the two counters of that table are equal.
50-
* If not, Assert.assertEquals() fails.
52+
* - 3 threads perform concurrent read operations. Each thread acquires an intent read lock on the db and
53+
* a read lock on the table. It then verifies whether the two counters of that table are equal.
54+
* If not, Assert.assertEquals() fails.
5155
* <p>
5256
* - Finally, verify that the sum of counter1 for all tables under each db is 2 million.
5357
* If not, Assert.assertEquals() fails.
@@ -62,7 +66,6 @@ public class LockManagerAllLockModesRandomTest {
6266
private static final long TABLE_ID_START = 10000;
6367
private static final long DB_ID_START = 20000;
6468

65-
6669
/**
6770
* We will acquire intensive lock or non-intensive lock on DB resource.
6871
* When acquiring intensive lock, we will also acquire specific non-intensive lock on the table to update it.
@@ -127,6 +130,7 @@ public void incrTwoCounters() {
127130
/**
128131
* We always increase the two counters together, so we will assert whether the two counters are equal to test
129132
* the correctness of the lock manager.
133+
*
130134
* @return the two counters in a pair
131135
*/
132136
public Pair<Long, Long> getTwoCounters() {
@@ -197,7 +201,7 @@ public void testAllLockModesConcurrent() throws InterruptedException {
197201
locker = new Locker();
198202
locker.lock(db.getId(), LockType.INTENTION_SHARED);
199203
locker.lock(db.getTableByIndex(tableIndex).getId(), LockType.READ);
200-
Pair<Long, Long> result = db.getOneRandomTable().getTwoCounters();
204+
Pair<Long, Long> result = db.getTableByIndex(tableIndex).getTwoCounters();
201205
Assert.assertEquals(result.first, result.second);
202206
} catch (LockException e) {
203207
Assert.fail();
@@ -279,24 +283,41 @@ public void testAllLockModesConcurrent() throws InterruptedException {
279283
} // end for single db
280284
} // enf for all dbs
281285

282-
// Start all threads.
283-
for (Thread t : allThreadList) {
284-
t.start();
286+
ExecutorService executor = Executors.newFixedThreadPool(100);
287+
288+
List<Future<?>> wf = new ArrayList<>();
289+
for (Thread thread : writeThreadList) {
290+
Future<?> f = executor.submit(thread);
291+
wf.add(f);
292+
}
293+
294+
List<Future<?>> rf = new ArrayList<>();
295+
for (Thread thread : readThreadList) {
296+
Future<?> f = executor.submit(thread);
297+
rf.add(f);
285298
}
286299

287300
// Wait for write threads end.
288-
for (Thread t : writeThreadList) {
289-
t.join();
301+
for (Future<?> f : wf) {
302+
try {
303+
f.get();
304+
} catch (ExecutionException e) {
305+
Assert.fail(e.getMessage());
306+
}
290307
}
291308
System.out.println("All write threads end.");
292309

293310
readerStop.set(true);
294311
// Wait for read threads end.
295-
for (Thread t : readThreadList) {
296-
t.join();
312+
for (Future<?> f : rf) {
313+
try {
314+
f.get();
315+
} catch (ExecutionException e) {
316+
Assert.fail(e.getMessage());
317+
}
297318
}
298-
System.out.println("All read threads end.");
299319

320+
System.out.println("All read threads end.");
300321

301322
// Verify the correctness of the lock manager.
302323
for (TestDBResource db : dbs) {
@@ -311,6 +332,5 @@ public void testAllLockModesConcurrent() throws InterruptedException {
311332
Assert.assertEquals(30 * NUM_TEST_OPERATIONS, counter1Sum);
312333
Assert.assertEquals(30 * NUM_TEST_OPERATIONS, counter2Sum);
313334
}
314-
315335
}
316336
}
Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,66 @@
1+
// Copyright 2021-present StarRocks, Inc. All rights reserved.
2+
//
3+
// Licensed under the Apache License, Version 2.0 (the "License");
4+
// you may not use this file except in compliance with the License.
5+
// You may obtain a copy of the License at
6+
//
7+
// https://www.apache.org/licenses/LICENSE-2.0
8+
//
9+
// Unless required by applicable law or agreed to in writing, software
10+
// distributed under the License is distributed on an "AS IS" BASIS,
11+
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
12+
// See the License for the specific language governing permissions and
13+
// limitations under the License.
14+
15+
package com.starrocks.common.lock;
16+
17+
import com.starrocks.common.util.concurrent.lock.LockInfo;
18+
import com.starrocks.common.util.concurrent.lock.LockManager;
19+
import com.starrocks.common.util.concurrent.lock.LockType;
20+
import com.starrocks.server.GlobalStateMgr;
21+
import org.junit.Assert;
22+
import org.junit.Before;
23+
import org.junit.Test;
24+
25+
import java.util.concurrent.Future;
26+
27+
import static com.starrocks.common.lock.LockTestUtils.assertLockSuccess;
28+
import static com.starrocks.common.lock.LockTestUtils.assertLockWait;
29+
30+
public class ReleaseTest {
31+
@Before
32+
public void setUp() {
33+
GlobalStateMgr.getCurrentState().setLockManager(new LockManager());
34+
}
35+
36+
@Test
37+
public void testReleaseInvoke() throws Exception {
38+
long rid = 1L;
39+
40+
TestLocker testLocker1 = new TestLocker();
41+
assertLockSuccess(testLocker1.lock(rid, LockType.WRITE));
42+
43+
TestLocker testLocker2 = new TestLocker();
44+
Future<LockResult> f2 = testLocker2.lock(rid, LockType.READ);
45+
assertLockWait(f2);
46+
47+
TestLocker testLocker3 = new TestLocker();
48+
Future<LockResult> f3 = testLocker3.lock(rid, LockType.READ);
49+
assertLockWait(f3);
50+
51+
TestLocker testLocker4 = new TestLocker();
52+
Future<LockResult> f4 = testLocker4.lock(rid, LockType.READ);
53+
assertLockWait(f4);
54+
55+
assertLockSuccess(testLocker1.release(rid, LockType.WRITE));
56+
LockTestUtils.assertLockSuccess(f2);
57+
LockTestUtils.assertLockSuccess(f3);
58+
LockTestUtils.assertLockSuccess(f4);
59+
60+
LockManager lockManager = GlobalStateMgr.getCurrentState().getLockManager();
61+
LockInfo lockInfo = lockManager.dumpLockManager().get(0);
62+
Assert.assertEquals(1, lockInfo.getRid().longValue());
63+
Assert.assertEquals(3, lockInfo.getOwners().size());
64+
Assert.assertEquals(0, lockInfo.getWaiters().size());
65+
}
66+
}

0 commit comments

Comments
 (0)