RATIS-2162. When closing leaderState, if the logAppender thread sends a snapshot, a deadlock may occur #1154

133tosakarin · 2024-09-25T02:50:58Z

What changes were proposed in this pull request?

Fix deadlock when closing leaderState causes grpc to start a large number of threads and cannot be recycled
Fix concurrency safety issue in LeaderStateImpl.SenderList, which may cause array out of bounds

I'm thinking of replacing the sychronized (server) block in InstallSnapshotRequests with some atomic variables newInstallSnapshotRequest

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/RATIS-2162

This reverts commit b150254.

szetszwo

@133tosakarin , thanks a lot for working on this! It is great that you have idenified the problem that it join() the leader state with the RaftServerImpl lock. Let's change the code to join() without the lock. See https://issues.apache.org/jira/secure/attachment/13071769/1154_review.patch and also the comments inlined.

ratis-server/src/main/java/org/apache/ratis/server/impl/ConfigurationManager.java

szetszwo · 2024-09-25T16:37:59Z

ratis-server/src/main/java/org/apache/ratis/server/impl/LeaderStateImpl.java

      return senders.removeAll(c);
    }

-    CompletableFuture<Void> stopAll() {
+    synchronized CompletableFuture<Void> stopAll() {


Since it is a CopyOnWriteArrayList, we should use iterator() or stream() instead of get(i), i.e.

CompletableFuture<Void> stopAll() { return CompletableFuture.allOf(senders.stream().map(LogAppender::stopAsync).toArray(CompletableFuture[]::new)); }

Then, we don't need synchronized for all the methods in SenderList.

Thanks for the review, I will make changes

szetszwo · 2024-09-25T16:40:44Z

ratis-server/src/main/java/org/apache/ratis/server/impl/ServerState.java

-  private final RaftGroupMemberId memberId;
+  private final AtomicReference<RaftGroupMemberId> memberId = new AtomicReference<>();


Let's don't add AtomicReference since memberId is never changed.

Thanks for the review, I will make changes

ratis-server/src/main/java/org/apache/ratis/server/impl/RoleInfo.java

szetszwo · 2024-09-25T17:42:28Z

ratis-server/src/main/java/org/apache/ratis/server/leader/InstallSnapshotRequests.java

-      synchronized (server) {
-        final SnapshotChunkProto.Builder b = LeaderProtoUtils.toSnapshotChunkProtoBuilder(
-            requestId, requestIndex++, snapshot.getTermIndex(), chunk, totalSize, done);
-        return LeaderProtoUtils.toInstallSnapshotRequestProto(server, followerId, b);
-      }
+      final SnapshotChunkProto.Builder b = LeaderProtoUtils.toSnapshotChunkProtoBuilder(
+          requestId, requestIndex++, snapshot.getTermIndex(), chunk, totalSize, done);
+      return LeaderProtoUtils.toInstallSnapshotRequestProto(server, followerId, b);


Let's keep synchronized (server). Otherwise, it could create an inconsistent request.

Thanks for the review, I will make changes

szetszwo · 2024-09-25T17:42:39Z

ratis-server/src/main/java/org/apache/ratis/server/leader/LogAppenderBase.java

@@ -128,7 +128,7 @@ public void start() {

  @Override
  public boolean isRunning() {
-    return daemon.isWorking();
+    return daemon.isWorking() && server.getInfo().isLeader();


This is a good change!

szetszwo

@133tosakarin , thanks for the update! Please see the comments inlined.

szetszwo · 2024-09-26T03:41:27Z

ratis-server/src/main/java/org/apache/ratis/server/impl/ConfigurationManager.java

+  private RaftConfigurationImpl currentConf;
  /** Cache the peer corresponding to {@link #id}. */
  @SuppressWarnings({"squid:S3077"}) // Suppress volatile for generic type
-  private volatile RaftPeer currentPeer;
+  private RaftPeer currentPeer;


Please remove also the @SuppressWarnings({"squid:S3077"}) for both fields. It was for the volatile false positive.

szetszwo · 2024-09-26T03:43:18Z

ratis-server/src/main/java/org/apache/ratis/server/impl/LeaderStateImpl.java

@@ -219,16 +219,13 @@ void addAll(Collection<LogAppender> newSenders) {
      Preconditions.assertTrue(changed);
    }

-    boolean removeAll(Collection<LogAppender> c) {
+    synchronized boolean removeAll(Collection<LogAppender> c) {


We don't need synchronized for both addAll and removeAll since it uses a CopyOnWriteArrayList.

ratis-server/src/main/java/org/apache/ratis/server/impl/RoleInfo.java

szetszwo

+1 the change looks good.

133tosakarin · 2024-09-26T07:05:25Z

+1 the change looks good.

There are some other deadlock problem, I need to check

133tosakarin · 2024-09-27T04:04:26Z

@szetszwo It seems that there are also cases where the unit (misc) test fails in the master branch

szetszwo · 2024-09-27T18:16:11Z

... It seems that there are also cases where the unit (misc) test fails in the master branch

@133tosakarin , the test failures in misc may be related to this change. In #1152 , the misc tests did not fail.

133tosakarin · 2024-09-28T05:55:21Z

... It seems that there are also cases where the unit (misc) test fails in the master branch

@133tosakarin , the test failures in misc may be related to this change. In #1152 , the misc tests did not fail.

I see. I will further check

… a snapshot, a deadlock may occur. (apache#1154)

szetszwo · 2024-09-29T17:50:49Z

Let's try testing it over release-3.1.1; see #1157

szetszwo · 2024-09-30T06:43:21Z

#1157 has passed over 3.1.1.

… a snapshot, a deadlock may occur. (#1154)

turboFei · 2024-12-20T19:26:11Z

Hi @133tosakarin
Thanks for fixing this.
I am using ratis-3.1.2, but continue to meet deadlock issue.

Here is the full jstack: https://issues.apache.org/jira/secure/attachment/13073619/13073619_celeborn_master_jstack.txt

And here is the ticket in celeborn community: https://issues.apache.org/jira/browse/CELEBORN-1795?focusedCommentId=17907470&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-17907470

BTW: The ratis rpc type is netty.

133tosakarin · 2024-12-21T10:07:23Z

Hi @133tosakarin Thanks for fixing this. I am using ratis-3.1.2, but continue to meet deadlock issue.

Here is the full jstack: https://issues.apache.org/jira/secure/attachment/13073619/13073619_celeborn_master_jstack.txt

And here is the ticket in celeborn community: https://issues.apache.org/jira/browse/CELEBORN-1795?focusedCommentId=17907470&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-17907470

BTW: The ratis rpc type is netty.

Thank you for providing the question. I hope you can post the specific information to Jira, and we will keep following up.

turboFei · 2024-12-21T21:19:45Z

Thank you for providing the question. I hope you can post the specific information to Jira, and we will keep following up.

Thanks for the reminder, I have update the info in RATIS-2162.

I can create a new RATIS ticket if necessary. Thank you.

@133tosakarin

133tosakarin added 5 commits September 25, 2024 10:42

fix deadlock and concurrent problem in LeaderStateImpl.SenderList

ca65521

change role to atomic

b150254

Revert "change role to atomic"

6463f47

This reverts commit b150254.

change role to atomic

1288a71

remove synchronized in newInstallSnapshotRequests

9937a05

szetszwo reviewed Sep 25, 2024

View reviewed changes

133tosakarin added 2 commits September 26, 2024 10:01

add changeToFollowerAsync

3bda524

format

a6c970a

szetszwo reviewed Sep 26, 2024

View reviewed changes

133tosakarin added 2 commits September 26, 2024 11:48

remove @SuppressWarnings

2308597

remove sync in SenderList

e0f488b

szetszwo approved these changes Sep 26, 2024

View reviewed changes

fix set stoppedFuture's value to null

0bc1783

szetszwo added a commit to szetszwo/ratis that referenced this pull request Sep 29, 2024

RATIS-2162. When closing leaderState, if the logAppender thread sends…

d3aa6d4

… a snapshot, a deadlock may occur. (apache#1154)

szetszwo mentioned this pull request Sep 29, 2024

Testing RATIS-2162 #1154 over release-3.1.1. #1157

Open

szetszwo merged commit e3d6736 into apache:master Sep 30, 2024
10 of 12 checks passed

OneSizeFitsQuorum pushed a commit that referenced this pull request Oct 9, 2024

RATIS-2162. When closing leaderState, if the logAppender thread sends…

d8482f1

… a snapshot, a deadlock may occur. (#1154)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RATIS-2162. When closing leaderState, if the logAppender thread sends a snapshot, a deadlock may occur #1154

RATIS-2162. When closing leaderState, if the logAppender thread sends a snapshot, a deadlock may occur #1154

133tosakarin commented Sep 25, 2024 •

edited

Loading

szetszwo left a comment

szetszwo Sep 25, 2024

133tosakarin Sep 26, 2024

szetszwo Sep 25, 2024

133tosakarin Sep 26, 2024

szetszwo Sep 25, 2024

133tosakarin Sep 26, 2024

szetszwo Sep 25, 2024

szetszwo left a comment

szetszwo Sep 26, 2024

133tosakarin Sep 26, 2024

szetszwo Sep 26, 2024

szetszwo left a comment

133tosakarin commented Sep 26, 2024

133tosakarin commented Sep 27, 2024

szetszwo commented Sep 27, 2024

133tosakarin commented Sep 28, 2024

szetszwo commented Sep 29, 2024

szetszwo commented Sep 30, 2024

turboFei commented Dec 20, 2024 •

edited

Loading

133tosakarin commented Dec 21, 2024

turboFei commented Dec 21, 2024

		private final RaftGroupMemberId memberId;
		private final AtomicReference<RaftGroupMemberId> memberId = new AtomicReference<>();

RATIS-2162. When closing leaderState, if the logAppender thread sends a snapshot, a deadlock may occur #1154

RATIS-2162. When closing leaderState, if the logAppender thread sends a snapshot, a deadlock may occur #1154

Conversation

133tosakarin commented Sep 25, 2024 • edited Loading

What changes were proposed in this pull request?

What is the link to the Apache JIRA

szetszwo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

szetszwo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

szetszwo left a comment

Choose a reason for hiding this comment

133tosakarin commented Sep 26, 2024

133tosakarin commented Sep 27, 2024

szetszwo commented Sep 27, 2024

133tosakarin commented Sep 28, 2024

szetszwo commented Sep 29, 2024

szetszwo commented Sep 30, 2024

turboFei commented Dec 20, 2024 • edited Loading

133tosakarin commented Dec 21, 2024

turboFei commented Dec 21, 2024

133tosakarin commented Sep 25, 2024 •

edited

Loading

turboFei commented Dec 20, 2024 •

edited

Loading