Skip to content

Commit

Permalink
(fix) refactor ThreadId and fix sofastack#781 (sofastack#783)
Browse files Browse the repository at this point in the history
* (fix) refactor ThreadId and fix sofastack#781

* (feat) Use ReentrantLock in replicator thread id
  • Loading branch information
killme2008 authored Mar 10, 2022
1 parent 0eaaf95 commit 530224e
Show file tree
Hide file tree
Showing 4 changed files with 30 additions and 77 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -1073,10 +1073,8 @@ public void onError(final ThreadId id, final Object data, final int errorCode) {
r.destroy();
}
} else if (errorCode == RaftError.ETIMEDOUT.getNumber()) {
id.unlock();
RpcUtils.runInThread(() -> sendHeartbeat(id));
} else {
id.unlock();
// noinspection ConstantConditions
Requires.requireTrue(false, "Unknown error code for replicator: " + errorCode);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -578,7 +578,7 @@ private void truncatePrefixInBackground(final long startIndex, final long firstI
long startMs = Utils.monotonicMs();
this.readLock.lock();
try {
RocksDB db =this.db;
RocksDB db = this.db;
if (db == null) {
LOG.warn(
"DB is null while truncating prefixed logs in data path: {}, the range is: [{}, {})",
Expand Down
97 changes: 29 additions & 68 deletions jraft-core/src/main/java/com/alipay/sofa/jraft/util/ThreadId.java
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,7 @@
*/
package com.alipay.sofa.jraft.util;

import java.util.ArrayList;
import java.util.List;
import java.util.concurrent.TimeUnit;

import java.util.concurrent.locks.ReentrantLock;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

Expand All @@ -32,15 +29,11 @@
*/
public class ThreadId {

private static final Logger LOG = LoggerFactory.getLogger(ThreadId.class);

private static final int TRY_LOCK_TIMEOUT_MS = 10;

private final Object data;
private final NonReentrantLock lock = new NonReentrantLock();
private final List<Integer> pendingErrors = new ArrayList<>();
private final OnError onError;
private volatile boolean destroyed;
private static final Logger LOG = LoggerFactory.getLogger(ThreadId.class);
private final Object data;
private final ReentrantLock lock = new ReentrantLock();
private final OnError onError;
private volatile boolean destroyed;

/**
* @author boyan ([email protected])
Expand All @@ -50,7 +43,7 @@ public class ThreadId {
public interface OnError {

/**
* Error callback, it will be called in lock, but should take care of unlocking it.
* Error callback, it will be called in lock.
*
* @param id the thread id
* @param data the data
Expand All @@ -74,16 +67,7 @@ public Object lock() {
if (this.destroyed) {
return null;
}
try {
while (!this.lock.tryLock(TRY_LOCK_TIMEOUT_MS, TimeUnit.MILLISECONDS)) {
if (this.destroyed) {
return null;
}
}
} catch (final InterruptedException e) {
Thread.currentThread().interrupt(); // reset
return null;
}
this.lock.lock();
// Got the lock, double checking state.
if (this.destroyed) {
// should release lock
Expand All @@ -95,30 +79,11 @@ public Object lock() {

public void unlock() {
if (!this.lock.isHeldByCurrentThread()) {
LOG.warn("Fail to unlock with {}, the lock is held by {} and current thread is {}.", this.data,
this.lock.getOwner(), Thread.currentThread());
LOG.warn("Fail to unlock with {}, the lock is not held by current thread {}.", this.data,
Thread.currentThread());
return;
}
// calls all pending errors before unlock
boolean doUnlock = true;
try {
final List<Integer> errors;
synchronized (this.pendingErrors) {
errors = new ArrayList<>(this.pendingErrors);
this.pendingErrors.clear();
}
for (final Integer code : errors) {
// The lock will be unlocked in onError.
doUnlock = false;
if (this.onError != null) {
this.onError.onError(this, this.data, code);
}
}
} finally {
if (doUnlock) {
this.lock.unlock();
}
}
this.lock.unlock();
}

public void join() {
Expand All @@ -137,37 +102,33 @@ public void unlockAndDestroy() {
return;
}
this.destroyed = true;
if (!this.lock.isHeldByCurrentThread()) {
LOG.warn("Fail to unlockAndDestroy with {}, the lock is held by {} and current thread is {}.", this.data,
this.lock.getOwner(), Thread.currentThread());
return;
}
this.lock.unlock();
unlock();
}

/**
* Set error code, if it tryLock success, run the onError callback
* with code immediately, else add it into pending errors and will
* be called before unlock.
*
* Set error code, run the onError callback
* with code immediately in lock.
* @param errorCode error code
*/
public void setError(final int errorCode) {
if (this.destroyed) {
LOG.warn("ThreadId: {} already destroyed, ignore error code: {}", this.data, errorCode);
return;
}
synchronized (pendingErrors) {
if (this.lock.tryLock()) {
if (this.destroyed) {
this.lock.unlock();
return;
}
if (this.onError != null) {
// The lock will be unlocked in onError.
this.onError.onError(this, this.data, errorCode);
}
} else {
this.pendingErrors.add(errorCode);
this.lock.lock();
try {
if (this.destroyed) {
LOG.warn("ThreadId: {} already destroyed, ignore error code: {}", this.data, errorCode);
return;
}
if (this.onError != null) {
this.onError.onError(this, this.data, errorCode);
}

} finally {
// Maybe destroyed in callback
if (!this.destroyed) {
this.lock.unlock();
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@ public class ThreadIdTest implements ThreadId.OnError {
public void onError(final ThreadId id, final Object data, final int errorCode) {
assertSame(id, this.id);
this.errorCode = errorCode;
id.unlock();
}

@Before
Expand Down Expand Up @@ -67,7 +66,6 @@ public void run() {
public void testSetError() throws Exception {
this.id.setError(100);
assertEquals(100, this.errorCode);
this.id.lock();
CountDownLatch latch = new CountDownLatch(1);
new Thread() {
@Override
Expand All @@ -77,10 +75,6 @@ public void run() {
}
}.start();
latch.await();
//just go into pending errors.
assertEquals(100, this.errorCode);
//invoke onError when unlock
this.id.unlock();
assertEquals(99, this.errorCode);
}

Expand Down

0 comments on commit 530224e

Please sign in to comment.