You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ratis.apache.org by "SzyWilliam (via GitHub)" <gi...@apache.org> on 2024/03/17 16:30:40 UTC
[PR] RATIS-2044. Fix ReadIndex loss caused by data race in AppendEntriesListeners [ratis]
SzyWilliam opened a new pull request, #1052:
URL: https://github.com/apache/ratis/pull/1052
# Issue
One linearizable read (read-index based) thread is stuck and never returned. jstack shows
```
"pool-75-IoTDB-ClientRPC-Processor-35$20240317_005430_35720_4.1.0" #14477 prio=5 os_prio=0 tid=0x00007ffafc049800 nid=0x3eb65e waiting on condition [0x00007ff8f5bc6000]
java.lang.Thread.State: WAITING (parking)
at sun.misc.Unsafe.park(Native Method)
- parking to wait for <0x000000031efd8348> (a java.util.concurrent.CompletableFuture$Signaller)
at java.util.concurrent.locks.LockSupport.park(LockSupport.java:175)
at java.util.concurrent.CompletableFuture$Signaller.block(CompletableFuture.java:1707)
at java.util.concurrent.ForkJoinPool.managedBlock(ForkJoinPool.java:3323)
at java.util.concurrent.CompletableFuture.waitingGet(CompletableFuture.java:1742)
at java.util.concurrent.CompletableFuture.get(CompletableFuture.java:1908)
at org.apache.ratis.server.impl.RaftServerImpl.waitForReply(RaftServerImpl.java:1153)
at org.apache.ratis.server.impl.RaftServerImpl.waitForReply(RaftServerImpl.java:1145)
at org.apache.ratis.server.impl.RaftServerImpl.submitClientRequest(RaftServerImpl.java:1140)
at org.apache.ratis.server.impl.RaftServerProxy.submitClientRequest(RaftServerProxy.java:462)
at org.apache.iotdb.consensus.ratis.utils.Retriable.attempt(Retriable.java:104)
at org.apache.iotdb.consensus.ratis.RatisConsensus.doRead(RatisConsensus.java:385)
at org.apache.iotdb.consensus.ratis.RatisConsensus.read(RatisConsensus.java:350)
at org.apache.iotdb.db.queryengine.execution.executor.RegionReadExecutor.execute(RegionReadExecutor.java:82)
at org.apache.iotdb.db.queryengine.plan.scheduler.FragmentInstanceDispatcherImpl.dispatchLocally(FragmentInstanceDispatcherImpl.java:413)
......
```
The read thread is forever stuck because the read-index future is never completed. The future loss is caused by the data race in `AppendEntriesListeners`. Specifically, the leader may add a new AppendEntriesListener [code here](https://github.com/apache/ratis/blob/422cb9d4cb3f6be8e3169797d11a3c2e666c138f/ratis-server/src/main/java/org/apache/ratis/server/impl/LeaderStateImpl.java#L1154) after the leaderStateImpl already being shutting down [code here](https://github.com/apache/ratis/blob/422cb9d4cb3f6be8e3169797d11a3c2e666c138f/ratis-server/src/main/java/org/apache/ratis/server/impl/LeaderStateImpl.java#L448). This AppendEntriesListener future then will never be completed.
# How to Solve
We can add `synchronized` to both `addAppendEntriesListener` and `failListeners` methods.
Or we can simply check the leader running state before the read index returns, as in this patch.
@szetszwo Could you please take a look? What solution do you prefer?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2044. Fix ReadIndex loss caused by data race in AppendEntriesListeners [ratis]
Posted by "SzyWilliam (via GitHub)" <gi...@apache.org>.
SzyWilliam commented on PR #1052:
URL: https://github.com/apache/ratis/pull/1052#issuecomment-2004050347
@szetszwo Thanks for your time! Your are right we should check it in `AppendEntriesListeners`. The original patch have memory leak problems. Let me apply the suggested changes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2044. Fix ReadIndex loss caused by data race in AppendEntriesListeners [ratis]
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on PR #1052:
URL: https://github.com/apache/ratis/pull/1052#issuecomment-2002727204
@SzyWilliam , Since the methods in `AppendEntriesListeners` are `synchronized`, let check it there?
```java
diff --git a/ratis-server/src/main/java/org/apache/ratis/server/impl/ReadIndexHeartbeats.java b/ratis-server/src/main/java/org/apache/ratis/server/impl/ReadIndexHeartbeats.java
index d08a1ea406..4ff1460d7d 100644
--- a/ratis-server/src/main/java/org/apache/ratis/server/impl/ReadIndexHeartbeats.java
+++ b/ratis-server/src/main/java/org/apache/ratis/server/impl/ReadIndexHeartbeats.java
@@ -23,6 +23,7 @@ import org.apache.ratis.server.leader.LogAppender;
import org.apache.ratis.server.raftlog.RaftLog;
import org.apache.ratis.server.raftlog.RaftLogIndex;
import org.apache.ratis.util.JavaUtils;
+import org.apache.ratis.util.Preconditions;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@@ -123,8 +124,15 @@ class ReadIndexHeartbeats {
class AppendEntriesListeners {
private final NavigableMap<Long, AppendEntriesListener> sorted = new TreeMap<>();
+ private Exception exception = null;
synchronized AppendEntriesListener add(long commitIndex, Function<Long, AppendEntriesListener> constructor) {
+ if (exception != null) {
+ Preconditions.assertTrue(sorted.isEmpty());
+ final AppendEntriesListener listener = constructor.apply(commitIndex);
+ listener.getFuture().completeExceptionally(exception);
+ return listener;
+ }
return sorted.computeIfAbsent(commitIndex, constructor);
}
@@ -152,6 +160,10 @@ class ReadIndexHeartbeats {
}
synchronized void failAll(Exception e) {
+ if (exception != null) {
+ return;
+ }
+ exception = e;
sorted.forEach((index, listener) -> listener.getFuture().completeExceptionally(e));
sorted.clear();
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2044. Fix ReadIndex loss caused by data race in AppendEntriesListeners [ratis]
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo merged PR #1052:
URL: https://github.com/apache/ratis/pull/1052
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org