Skip to content

Commit

Permalink
fix: log and remove a deadlock when logging a diff message using the …
Browse files Browse the repository at this point in the history
…blockchain

Signed-off-by: HashEngineering <[email protected]>
  • Loading branch information
HashEngineering committed Jul 24, 2024
1 parent a559bca commit ea5dfa5
Show file tree
Hide file tree
Showing 4 changed files with 72 additions and 3 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@
import org.bitcoinj.quorums.SimplifiedQuorumList;
import org.bitcoinj.store.BlockStoreException;
import org.bitcoinj.utils.ContextPropagatingThreadFactory;
import org.bitcoinj.utils.DebugReentrantLock;
import org.bitcoinj.utils.Threading;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
Expand Down Expand Up @@ -89,7 +90,8 @@ public abstract class AbstractQuorumState<Request extends AbstractQuorumRequest,
public static final int MIN_CACHE_SIZE = 1;

private static final Logger log = LoggerFactory.getLogger(AbstractQuorumState.class);
protected final ReentrantLock lock = Threading.lock("AbstractQuorumState");
// TODO: Revert to ReentrantLock with 21.0.1
protected final DebugReentrantLock lock = Threading.debugLock("AbstractQuorumState");

Context context;
DualBlockChain blockChain;
Expand Down Expand Up @@ -222,6 +224,7 @@ public abstract void processDiff(@Nullable Peer peer, DiffMessage difference,
public abstract void requestUpdate(Peer peer, StoredBlock block);

public void retryLastUpdate(Peer peer) {
log.info("retryLastUpdate: {}", lastRequest.getRequestMessage());

Check warning on line 227 in core/src/main/java/org/bitcoinj/evolution/AbstractQuorumState.java

View check run for this annotation

Codecov / codecov/patch

core/src/main/java/org/bitcoinj/evolution/AbstractQuorumState.java#L227

Added line #L227 was not covered by tests
if (peer != null) {
peer.sendMessage(lastRequest.getRequestMessage());
} else {
Expand Down Expand Up @@ -409,7 +412,8 @@ boolean requestNextMNListDiff() {
log.info("sending {} from {} to {}; \n From {}\n To {}", lastRequest.request.getClass().getSimpleName(),
getMasternodeListAtTip().getHeight(), nextBlock.getHeight(), getMasternodeListAtTip().getBlockHash(), nextBlock.getHeader().getHash());
requestUpdate(downloadPeer, nextBlock);
log.info("message = {}", lastRequest.getRequestMessage().toString(blockChain));
// This was causing a dead lock when using toString(DualBlockchain). Use toString() instead.
log.info("message = {}", lastRequest.getRequestMessage());

Check warning on line 416 in core/src/main/java/org/bitcoinj/evolution/AbstractQuorumState.java

View check run for this annotation

Codecov / codecov/patch

core/src/main/java/org/bitcoinj/evolution/AbstractQuorumState.java#L416

Added line #L416 was not covered by tests
waitingForMNListDiff = true;
return true;
} else {
Expand Down Expand Up @@ -745,7 +749,7 @@ private void retryLastRequest(Peer peer, Exception e) {
try {
if (isLocked) {
downloadPeer = context.peerGroup.getDownloadPeer();
log.info("{}: lock acquired, obtaining downloadPeer from peerGroup: {}",
log.info("{}: peergroup lock acquired, obtaining downloadPeer from peerGroup: {}",

Check warning on line 752 in core/src/main/java/org/bitcoinj/evolution/AbstractQuorumState.java

View check run for this annotation

Codecov / codecov/patch

core/src/main/java/org/bitcoinj/evolution/AbstractQuorumState.java#L752

Added line #L752 was not covered by tests
Thread.currentThread().getName(), downloadPeer);
if (downloadPeer == null) {
chooseRandomDownloadPeer();
Expand Down
21 changes: 21 additions & 0 deletions core/src/main/java/org/bitcoinj/evolution/QuorumState.java
Original file line number Diff line number Diff line change
Expand Up @@ -265,6 +265,27 @@ public void processDiff(@Nullable Peer peer, SimplifiedMasternodeListDiff mnlist
mnlistdiff.dump(mnList.getHeight(), newHeight);
lastRequest.setReceived();

// TODO: Remove with v21.0.1
if (lock.isLocked()) {
log.warn("the lock is held. holdCount {}, queue length {}, held by this thread {}", lock.getHoldCount(), lock.getQueueLength(), lock.isHeldByCurrentThread());
log.warn("the current thread is {} with stack trace:", Thread.currentThread().getName());
StackTraceElement [] trace = lock.getOwner().getStackTrace();

Check warning on line 272 in core/src/main/java/org/bitcoinj/evolution/QuorumState.java

View check run for this annotation

Codecov / codecov/patch

core/src/main/java/org/bitcoinj/evolution/QuorumState.java#L270-L272

Added lines #L270 - L272 were not covered by tests
for (StackTraceElement e : trace) {
log.info(" {}", e);

Check warning on line 274 in core/src/main/java/org/bitcoinj/evolution/QuorumState.java

View check run for this annotation

Codecov / codecov/patch

core/src/main/java/org/bitcoinj/evolution/QuorumState.java#L274

Added line #L274 was not covered by tests
}
log.warn("the lock is held by {} with stack trace:", lock.getOwner());
trace = lock.getOwner().getStackTrace();

Check warning on line 277 in core/src/main/java/org/bitcoinj/evolution/QuorumState.java

View check run for this annotation

Codecov / codecov/patch

core/src/main/java/org/bitcoinj/evolution/QuorumState.java#L276-L277

Added lines #L276 - L277 were not covered by tests
for (StackTraceElement e : trace) {
log.info(" {}", e);

Check warning on line 279 in core/src/main/java/org/bitcoinj/evolution/QuorumState.java

View check run for this annotation

Codecov / codecov/patch

core/src/main/java/org/bitcoinj/evolution/QuorumState.java#L279

Added line #L279 was not covered by tests
}
for (Thread thread : lock.getQueuedThreads()) {
log.info("queued thread: {}\n stack trace:", thread.getName());
trace = thread.getStackTrace();

Check warning on line 283 in core/src/main/java/org/bitcoinj/evolution/QuorumState.java

View check run for this annotation

Codecov / codecov/patch

core/src/main/java/org/bitcoinj/evolution/QuorumState.java#L282-L283

Added lines #L282 - L283 were not covered by tests
for (StackTraceElement e : trace) {
log.info(" {}", e);

Check warning on line 285 in core/src/main/java/org/bitcoinj/evolution/QuorumState.java

View check run for this annotation

Codecov / codecov/patch

core/src/main/java/org/bitcoinj/evolution/QuorumState.java#L285

Added line #L285 was not covered by tests
}
}

Check warning on line 287 in core/src/main/java/org/bitcoinj/evolution/QuorumState.java

View check run for this annotation

Codecov / codecov/patch

core/src/main/java/org/bitcoinj/evolution/QuorumState.java#L287

Added line #L287 was not covered by tests
}
lock.lock();
try {
log.info("lock acquired when processing mnlistdiff");
Expand Down
39 changes: 39 additions & 0 deletions core/src/main/java/org/bitcoinj/utils/DebugReentrantLock.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
/*
* Copyright 2022 Dash Core Group
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package org.bitcoinj.utils;

import java.util.Collection;
import java.util.concurrent.locks.ReentrantLock;

/** a subclass of ReentrantLock that provides access to the owner thread
* and queued threads.
*/

public class DebugReentrantLock extends ReentrantLock {

public DebugReentrantLock(String name) {
super(true);
}

public Collection<Thread> getQueuedThreads() {
return super.getQueuedThreads();

Check warning on line 33 in core/src/main/java/org/bitcoinj/utils/DebugReentrantLock.java

View check run for this annotation

Codecov / codecov/patch

core/src/main/java/org/bitcoinj/utils/DebugReentrantLock.java#L33

Added line #L33 was not covered by tests
}

public Thread getOwner() {
return super.getOwner();

Check warning on line 37 in core/src/main/java/org/bitcoinj/utils/DebugReentrantLock.java

View check run for this annotation

Codecov / codecov/patch

core/src/main/java/org/bitcoinj/utils/DebugReentrantLock.java#L37

Added line #L37 was not covered by tests
}
}
5 changes: 5 additions & 0 deletions core/src/main/java/org/bitcoinj/utils/Threading.java
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,11 @@ public static ReentrantLock lock(String name) {
return factory.newReentrantLock(name);
}

/** creates a lock that gives access to the owner thread **/
public static DebugReentrantLock debugLock(String name) {
return new DebugReentrantLock(name);
}

public static void setUseDefaultAndroidPolicy(boolean use) {
useDefaultAndroidPolicy = use;
}
Expand Down

0 comments on commit ea5dfa5

Please sign in to comment.