You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ratis.apache.org by "chungen0126 (via GitHub)" <gi...@apache.org> on 2024/09/09 13:40:51 UTC
[PR] RATIS-2152. The `done` should also lock in `AwaitForSignal` [ratis]
chungen0126 opened a new pull request, #1146:
URL: https://github.com/apache/ratis/pull/1146
## What changes were proposed in this pull request?
In `GrpcLogAppender`, it waits for signal at the end of `notifyInstallSnapshot` as following.
https://github.com/apache/ratis/blob/master/ratis-grpc/src/main/java/org/apache/ratis/grpc/server/GrpcLogAppender.java#L825-L831
However, checking whether the `InstallSnapshotResponseHandler` is done and the call `AwaitForSignal.await()` are not atomic. This creates a potential race condition where InstallSnapshotResponseHandler.close() could finish after the check but before the wait, causing that `GrpcLogAppender` is still waiting even though `InstallSnapshotResponseHandler` has already completed, leading to timeout.
Things change in the patch:
Move the done check inside the lock in AwaitForSignal to ensure atomicity.
## What is the link to the Apache JIRA
[RATIS-2152](https://issues.apache.org/jira/browse/RATIS-2152)
Please replace this section with the link to the Apache JIRA)
## How was this patch tested?
CI:
https://github.com/chungen0126/ratis/actions/runs/10771583301
--
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-2152. GrpcLogAppender stucks while sending an installSnapshot notification request [ratis]
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on PR #1146:
URL: https://github.com/apache/ratis/pull/1146#issuecomment-2344762073
> I think the while loop in the new diff might also become infinite ...
Good catch!
We actually should use `CompletableFuture` in this case; see below:
```java
diff --git a/ratis-grpc/src/main/java/org/apache/ratis/grpc/server/GrpcLogAppender.java b/ratis-grpc/src/main/java/org/apache/ratis/grpc/server/GrpcLogAppender.java
index 1da7bb3b47..355adc2f8b 100644
--- a/ratis-grpc/src/main/java/org/apache/ratis/grpc/server/GrpcLogAppender.java
+++ b/ratis-grpc/src/main/java/org/apache/ratis/grpc/server/GrpcLogAppender.java
@@ -54,8 +54,9 @@ import java.io.InterruptedIOException;
import java.util.*;
import java.util.concurrent.CompletableFuture;
import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.ExecutionException;
import java.util.concurrent.TimeUnit;
-import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.TimeoutException;
import java.util.concurrent.atomic.AtomicLong;
/**
@@ -576,7 +577,7 @@ public class GrpcLogAppender extends LogAppenderBase {
private class InstallSnapshotResponseHandler implements StreamObserver<InstallSnapshotReplyProto> {
private final String name = getFollower().getName() + "-" + JavaUtils.getClassSimpleName(getClass());
private final Queue<Integer> pending;
- private final AtomicBoolean done = new AtomicBoolean(false);
+ private final CompletableFuture<Void> done = new CompletableFuture<>();
private final boolean isNotificationOnly;
InstallSnapshotResponseHandler() {
@@ -637,12 +638,19 @@ public class GrpcLogAppender extends LogAppenderBase {
getServer().getStateMachine().event().notifySnapshotInstalled(result, snapshotIndex, getFollower().getPeer());
}
- boolean isDone() {
- return done.get();
+ boolean isRunning() {
+ try {
+ done.get(100, TimeUnit.MILLISECONDS);
+ return false;
+ } catch (InterruptedException | TimeoutException e) {
+ return true;
+ } catch (ExecutionException e) {
+ throw new IllegalStateException("Failed to complete " + name, e);
+ }
}
void close() {
- done.set(true);
+ done.complete(null);
notifyLogAppender();
}
@@ -718,7 +726,7 @@ public class GrpcLogAppender extends LogAppenderBase {
@Override
public void onError(Throwable t) {
- if (!isRunning()) {
+ if (!GrpcLogAppender.this.isRunning()) {
LOG.info("{} is stopped", GrpcLogAppender.this);
return;
}
@@ -777,13 +785,8 @@ public class GrpcLogAppender extends LogAppenderBase {
return;
}
- while (isRunning() && !responseHandler.isDone()) {
- try {
- getEventAwaitForSignal().await(getWaitTimeMs(), TimeUnit.MILLISECONDS);
- } catch (InterruptedException ignored) {
- Thread.currentThread().interrupt();
- }
- }
+ // wait for responseHandler to complete
+ while (isRunning() && responseHandler.isRunning());
if (responseHandler.hasAllResponse()) {
getFollower().setSnapshotIndex(snapshot.getTermIndex().getIndex());
@@ -822,13 +825,8 @@ public class GrpcLogAppender extends LogAppenderBase {
return;
}
- while (isRunning() && !responseHandler.isDone()) {
- try {
- getEventAwaitForSignal().await(getWaitTimeMs(), TimeUnit.MILLISECONDS);
- } catch (InterruptedException ignored) {
- Thread.currentThread().interrupt();
- }
- }
+ // wait for responseHandler to complete
+ while (isRunning() && responseHandler.isRunning());
}
/**
```
--
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-2152. GrpcLogAppender stucks while sending an installSnapshot notification request [ratis]
Posted by "chungen0126 (via GitHub)" <gi...@apache.org>.
chungen0126 commented on PR #1146:
URL: https://github.com/apache/ratis/pull/1146#issuecomment-2344615720
@szetszwo , I think the while loop in the new diff might also become infinite if the InstallSnapshotResponseHandler closes before the loop starts.
--
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-2152. GrpcLogAppender stucks while sending an installSnapshot notification request [ratis]
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on PR #1146:
URL: https://github.com/apache/ratis/pull/1146#issuecomment-2344539912
@chungen0126 , sorry, copied a wrong diff. Updated my previous comment.
--
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-2152. GrpcLogAppender stucks while sending an installSnapshot notification request [ratis]
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo merged PR #1146:
URL: https://github.com/apache/ratis/pull/1146
--
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-2152. GrpcLogAppender stucks while sending an installSnapshot notification request [ratis]
Posted by "chungen0126 (via GitHub)" <gi...@apache.org>.
chungen0126 commented on PR #1146:
URL: https://github.com/apache/ratis/pull/1146#issuecomment-2344454009
>It seems that we may remove done as below. What do you think?
Is there something miss in the code? The while loop might end only when it is closed.
The another reason why I think the current implement is better because It will become a busy loop in a normal node.
--
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