Skip to content

Commit f597662

Browse files
dizzzzdizzzz
authored
Merge pull request #2244 from adamretter/hotfix/perm-lock-lifetime
Fix an issue with lock lifetime when changing permissions
2 parents e78ae1a + 5bb432e commit f597662

File tree

6 files changed

+277
-41
lines changed

6 files changed

+277
-41
lines changed

‎src/org/exist/management/impl/LockTable.java‎

Lines changed: 61 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -24,14 +24,22 @@
2424
import org.exist.storage.BrokerPool;
2525
import org.exist.storage.lock.Lock;
2626
import org.exist.storage.lock.Lock.LockType;
27+
import org.exist.storage.lock.LockTable.LockCountTraces;
2728
import org.exist.storage.lock.LockTable.LockModeOwner;
2829
import org.exist.storage.lock.LockTableUtils;
30+
import org.exist.util.io.FastByteArrayOutputStream;
2931

3032
import javax.management.MalformedObjectNameException;
3133
import javax.management.ObjectName;
34+
import javax.xml.stream.XMLStreamException;
35+
import java.io.IOException;
36+
import java.io.OutputStreamWriter;
37+
import java.io.Writer;
3238
import java.util.List;
3339
import java.util.Map;
3440

41+
import static java.nio.charset.StandardCharsets.UTF_8;
42+
3543
/**
3644
* JMX MXBean for examining the LockTable
3745
*
@@ -64,7 +72,7 @@ public String getInstanceId() {
6472
}
6573

6674
@Override
67-
public Map<String, Map<LockType, Map<Lock.LockMode, Map<String, Integer>>>> getAcquired() {
75+
public Map<String, Map<LockType, Map<Lock.LockMode, Map<String, LockCountTraces>>>> getAcquired() {
6876
return pool.getLockManager().getLockTable().getAcquired();
6977
}
7078

@@ -75,13 +83,63 @@ public Map<String, Map<LockType, List<LockModeOwner>>> getAttempting() {
7583

7684
@Override
7785
public void dumpToConsole() {
78-
System.out.println(LockTableUtils.stateToString(pool.getLockManager().getLockTable()));
86+
System.out.println(LockTableUtils.stateToString(pool.getLockManager().getLockTable(), false));
87+
}
88+
89+
@Override
90+
public void fullDumpToConsole() {
91+
System.out.println(LockTableUtils.stateToString(pool.getLockManager().getLockTable(), true));
92+
}
93+
94+
@Override
95+
public void xmlDumpToConsole() {
96+
try {
97+
LockTableUtils.stateToXml(pool.getLockManager().getLockTable(), false, new OutputStreamWriter(System.out));
98+
} catch (final XMLStreamException e) {
99+
throw new RuntimeException(e);
100+
}
101+
}
102+
103+
@Override
104+
public void xmlFullDumpToConsole() {
105+
try {
106+
LockTableUtils.stateToXml(pool.getLockManager().getLockTable(), true, new OutputStreamWriter(System.out));
107+
} catch (final XMLStreamException e) {
108+
throw new RuntimeException(e);
109+
}
79110
}
80111

81112
private final static Logger LOCK_LOG = LogManager.getLogger(org.exist.storage.lock.LockTable.class);
82113

83114
@Override
84115
public void dumpToLog() {
85-
LOCK_LOG.info(LockTableUtils.stateToString(pool.getLockManager().getLockTable()));
116+
LOCK_LOG.info(LockTableUtils.stateToString(pool.getLockManager().getLockTable(), false));
117+
}
118+
119+
@Override
120+
public void fullDumpToLog() {
121+
LOCK_LOG.info(LockTableUtils.stateToString(pool.getLockManager().getLockTable(), true));
122+
}
123+
124+
@Override
125+
public void xmlDumpToLog() {
126+
try (final FastByteArrayOutputStream bos = new FastByteArrayOutputStream();
127+
final Writer writer = new OutputStreamWriter(bos)) {
128+
LockTableUtils.stateToXml(pool.getLockManager().getLockTable(), false, writer);
129+
LOCK_LOG.info(new String(bos.toByteArray(), UTF_8));
130+
} catch (final IOException | XMLStreamException e) {
131+
throw new RuntimeException(e);
132+
}
133+
}
134+
135+
@Override
136+
public void xmlFullDumpToLog() {
137+
try (final FastByteArrayOutputStream bos = new FastByteArrayOutputStream();
138+
final Writer writer = new OutputStreamWriter(bos)) {
139+
LockTableUtils.stateToXml(pool.getLockManager().getLockTable(), true, writer);
140+
LOCK_LOG.info(new String(bos.toByteArray(), UTF_8));
141+
} catch (final IOException | XMLStreamException e) {
142+
throw new RuntimeException(e);
143+
}
86144
}
87145
}

‎src/org/exist/management/impl/LockTableMXBean.java‎

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -22,11 +22,11 @@
2222

2323
import org.exist.storage.lock.Lock;
2424
import org.exist.storage.lock.LockTable;
25+
import org.exist.storage.lock.LockTable.LockCountTraces;
2526
import org.exist.storage.lock.LockTable.LockModeOwner;
2627

2728
import java.util.List;
2829
import java.util.Map;
29-
import java.util.Set;
3030

3131
/**
3232
* JMX MXBean interface for examining the LockTable
@@ -40,7 +40,7 @@ public interface LockTableMXBean extends PerInstanceMBean {
4040
*
4141
* @return information about acquired locks
4242
*/
43-
Map<String, Map<Lock.LockType, Map<Lock.LockMode, Map<String, Integer>>>> getAcquired();
43+
Map<String, Map<Lock.LockType, Map<Lock.LockMode, Map<String, LockCountTraces>>>> getAcquired();
4444

4545
/**
4646
* Get information about outstanding attempts to acquire locks
@@ -52,4 +52,16 @@ public interface LockTableMXBean extends PerInstanceMBean {
5252
void dumpToConsole();
5353

5454
void dumpToLog();
55+
56+
void xmlDumpToConsole();
57+
58+
void xmlDumpToLog();
59+
60+
void fullDumpToConsole();
61+
62+
void fullDumpToLog();
63+
64+
void xmlFullDumpToConsole();
65+
66+
void xmlFullDumpToLog();
5567
}

‎src/org/exist/security/PermissionFactory.java‎

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,6 @@
3939
import org.exist.storage.lock.Lock.LockMode;
4040
import org.exist.storage.txn.Txn;
4141
import com.evolvedbinary.j8fu.function.ConsumerE;
42-
import org.exist.util.LockException;
4342
import org.exist.util.SyntaxException;
4443
import org.exist.xmldb.XmldbURI;
4544
import org.exist.xquery.XPathException;
@@ -134,8 +133,8 @@ private static void updatePermissions(final DBBroker broker, final Txn transacti
134133

135134
final DocumentImpl doc = lockedDoc.getDocument();
136135

137-
// keep a write lock in the transaction
138-
transaction.acquireDocumentLock(() -> brokerPool.getLockManager().acquireDocumentWriteLock(doc.getURI()));
136+
// // keep a write lock in the transaction
137+
// transaction.acquireDocumentLock(() -> brokerPool.getLockManager().acquireDocumentWriteLock(doc.getURI()));
139138

140139

141140
final Permission permissions = doc.getPermissions();
@@ -144,8 +143,8 @@ private static void updatePermissions(final DBBroker broker, final Txn transacti
144143
broker.storeXMLResource(transaction, doc);
145144
}
146145
} else {
147-
// keep a write lock in the transaction
148-
transaction.acquireCollectionLock(() -> brokerPool.getLockManager().acquireCollectionWriteLock(collection.getURI()));
146+
// // keep a write lock in the transaction
147+
// transaction.acquireCollectionLock(() -> brokerPool.getLockManager().acquireCollectionWriteLock(collection.getURI()));
149148

150149
final Permission permissions = collection.getPermissionsNoLock();
151150
permissionModifier.accept(permissions);
@@ -154,7 +153,7 @@ private static void updatePermissions(final DBBroker broker, final Txn transacti
154153
}
155154
broker.flush();
156155
}
157-
} catch(final XPathException | PermissionDeniedException | IOException | LockException e) {
156+
} catch(final XPathException | PermissionDeniedException | IOException e) {
158157
throw new PermissionDeniedException("Permission to modify permissions is denied for user '" + broker.getCurrentSubject().getName() + "' on '" + pathUri.toString() + "': " + e.getMessage(), e);
159158
}
160159
}

‎src/org/exist/storage/lock/LockTable.java‎

Lines changed: 65 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@
3434
import java.util.concurrent.*;
3535
import java.util.stream.Collectors;
3636

37+
import static com.evolvedbinary.j8fu.tuple.Tuple.Tuple;
3738
import static org.exist.storage.lock.LockTable.LockAction.Action.*;
3839
import static org.exist.util.ThreadUtils.newInstanceThread;
3940

@@ -84,9 +85,9 @@ public class LockTable {
8485
/**
8586
* Reference count of acquired locks by id and type
8687
*
87-
* Map<Id, Map<Lock Type, Map<Lock Mode, Map<Owner, HoldCount>>>>
88+
* Map<Id, Map<Lock Type, Map<Lock Mode, Map<Owner, LockCountTraces>>>>
8889
*/
89-
private final ConcurrentMap<String, Map<LockType, Map<LockMode, Map<String, Integer>>>> acquired = new ConcurrentHashMap<>();
90+
private final ConcurrentMap<String, Map<LockType, Map<LockMode, Map<String, LockCountTraces>>>> acquired = new ConcurrentHashMap<>();
9091

9192
/**
9293
* The {@link #queue} holds lock events and lock listener events
@@ -269,7 +270,7 @@ public Map<String, Map<LockType, List<LockModeOwner>>> getAttempting() {
269270
*
270271
* @return acquired lock information
271272
*/
272-
public Map<String, Map<LockType, Map<LockMode, Map<String, Integer>>>> getAcquired() {
273+
public Map<String, Map<LockType, Map<LockMode, Map<String, LockCountTraces>>>> getAcquired() {
273274
return new HashMap<>(acquired);
274275
}
275276

@@ -291,15 +292,34 @@ public String getOwnerThread() {
291292
}
292293
}
293294

295+
public static class LockCountTraces {
296+
int count;
297+
@Nullable final List<StackTraceElement[]> traces;
298+
299+
public LockCountTraces(final int count, @Nullable final List<StackTraceElement[]> traces) {
300+
this.count = count;
301+
this.traces = traces;
302+
}
303+
304+
public int getCount() {
305+
return count;
306+
}
307+
308+
@Nullable
309+
public List<StackTraceElement[]> getTraces() {
310+
return traces;
311+
}
312+
}
313+
294314
private static class QueueConsumer implements Runnable {
295315
private final TransferQueue<Either<ListenerAction, LockAction>> queue;
296316
private final ConcurrentMap<String, Map<LockType, List<LockModeOwner>>> attempting;
297-
private final ConcurrentMap<String, Map<LockType, Map<LockMode, Map<String, Integer>>>> acquired;
317+
private final ConcurrentMap<String, Map<LockType, Map<LockMode, Map<String, LockCountTraces>>>> acquired;
298318
private final List<LockEventListener> listeners = new ArrayList<>();
299319

300320
QueueConsumer(final TransferQueue<Either<ListenerAction, LockAction>> queue,
301321
final ConcurrentMap<String, Map<LockType, List<LockModeOwner>>> attempting,
302-
final ConcurrentMap<String, Map<LockType, Map<LockMode, Map<String, Integer>>>> acquired) {
322+
final ConcurrentMap<String, Map<LockType, Map<LockMode, Map<String, LockCountTraces>>>> acquired) {
303323
this.queue = queue;
304324
this.attempting = attempting;
305325
this.acquired = acquired;
@@ -458,12 +478,16 @@ private void incrementAcquired(final LockAction lockAction) {
458478

459479
ownerHolds.compute(lockAction.threadName, (threadName, holdCount) -> {
460480
if(holdCount == null) {
461-
holdCount = 0;
481+
holdCount = new LockCountTraces(1, List(lockAction.stackTrace));
482+
} else {
483+
holdCount = append(holdCount, lockAction.stackTrace);
462484
}
463-
return ++holdCount;
485+
return holdCount;
464486
});
465487

466-
final int lockModeHolds = ownerHolds.values().stream().collect(Collectors.summingInt(Integer::intValue));
488+
final int lockModeHolds = ownerHolds.values().stream()
489+
.map(LockCountTraces::getCount)
490+
.collect(Collectors.summingInt(Integer::intValue));
467491
notifyListenersOfAcquire(lockAction, lockModeHolds);
468492

469493
return ownerHolds;
@@ -476,6 +500,34 @@ private void incrementAcquired(final LockAction lockAction) {
476500
});
477501
}
478502

503+
private static @Nullable <T> List<T> List(@Nullable final T item) {
504+
if (item == null) {
505+
return null;
506+
}
507+
508+
final List<T> list = new ArrayList<>();
509+
list.add(item);
510+
return list;
511+
}
512+
513+
private static LockCountTraces append(final LockCountTraces holdCount, @Nullable final StackTraceElement[] trace) {
514+
List<StackTraceElement[]> traces = holdCount.traces;
515+
if (traces != null) {
516+
traces.add(trace);
517+
}
518+
holdCount.count++;
519+
return holdCount;
520+
}
521+
522+
private static LockCountTraces removeLast(final LockCountTraces holdCount) {
523+
List<StackTraceElement[]> traces = holdCount.traces;
524+
if (traces != null) {
525+
traces.remove(traces.size() - 1);
526+
}
527+
holdCount.count--;
528+
return holdCount;
529+
}
530+
479531
private void decrementAcquired(final LockAction lockAction) {
480532
acquired.compute(lockAction.id, (id, acqu) -> {
481533
if (acqu == null) {
@@ -498,17 +550,17 @@ private void decrementAcquired(final LockAction lockAction) {
498550
if(holdCount == null) {
499551
LOG.error("No entry found when trying to decrementAcquired for: id={}, lockType={}, lockMode={}, threadName={}", lockAction.id, lockAction.lockType, lockAction.mode, lockAction.threadName);
500552
return null;
501-
} else if(holdCount == 0) {
553+
} else if(holdCount.count == 0) {
502554
LOG.error("Negative release when trying to decrementAcquired for: id={}, lockType={}, lockMode={}, threadName={}", lockAction.id, lockAction.lockType, lockAction.mode, lockAction.threadName);
503555
return null;
504-
} else if(holdCount == 1) {
556+
} else if(holdCount.count == 1) {
505557
return null;
506558
} else {
507-
return --holdCount;
559+
return removeLast(holdCount);
508560
}
509561
});
510562

511-
final int lockModeHolds = ownerHolds.values().stream().collect(Collectors.summingInt(Integer::intValue));
563+
final int lockModeHolds = ownerHolds.values().stream().map(LockCountTraces::getCount).collect(Collectors.summingInt(Integer::intValue));
512564

513565
notifyListenersOfRelease(lockAction, lockModeHolds);
514566

@@ -701,7 +753,7 @@ private void sanityCheckLockLifecycles(final LockAction lockAction) {
701753
LOG.trace("QUEUE: {} (read={} write={})", lockAction.toString(), read, write);
702754
}
703755

704-
lockCounts.put(lockAction.id, new Tuple2<>(read, write));
756+
lockCounts.put(lockAction.id, Tuple(read, write));
705757
}
706758
}
707759
}

0 commit comments

Comments
 (0)