You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@zookeeper.apache.org by GitBox <gi...@apache.org> on 2021/02/06 21:54:14 UTC

[GitHub] [zookeeper] functioner opened a new pull request #1596: ZOOKEEPER-4203: Leader swallows the ZooKeeperServer.State.ERROR from Leader.LearnerCnxAcceptor in some concurrency condition

functioner opened a new pull request #1596:
URL: https://github.com/apache/zookeeper/pull/1596


   The fix of [ZOOKEEPER-4203](https://issues.apache.org/jira/browse/ZOOKEEPER-4203) is implemented. In my local machine, it is able to pass all test cases.


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [zookeeper] functioner commented on pull request #1596: ZOOKEEPER-4203: Leader swallows the ZooKeeperServer.State.ERROR from Leader.LearnerCnxAcceptor in some concurrency condition

Posted by GitBox <gi...@apache.org>.
functioner commented on pull request #1596:
URL: https://github.com/apache/zookeeper/pull/1596#issuecomment-978799722


   I've added `LOG.warn` when this case is happening. You can take a look @eolivelli @ztzg 
   @lanicc Thanks for the test case you provide! It inspires me a lot.
   I implemented a test case without using Byteman. It's basically overriding lots of methods; thanks  @ztzg  for the idea.
   Anybody can approve the CI workflow?


-- 
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: notifications-unsubscribe@zookeeper.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [zookeeper] functioner commented on pull request #1596: ZOOKEEPER-4203: Leader swallows the ZooKeeperServer.State.ERROR from Leader.LearnerCnxAcceptor in some concurrency condition

Posted by GitBox <gi...@apache.org>.
functioner commented on pull request #1596:
URL: https://github.com/apache/zookeeper/pull/1596#issuecomment-982299722


   @maoling Sorry to bother you. But the other guys might be busy recently. Can you help review my patch & test case? And approve the CI workflow? Thank you!


-- 
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: notifications-unsubscribe@zookeeper.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [zookeeper] functioner commented on a change in pull request #1596: ZOOKEEPER-4203: Leader swallows the ZooKeeperServer.State.ERROR from Leader.LearnerCnxAcceptor in some concurrency condition

Posted by GitBox <gi...@apache.org>.
functioner commented on a change in pull request #1596:
URL: https://github.com/apache/zookeeper/pull/1596#discussion_r582098731



##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/QuorumZooKeeperServer.java
##########
@@ -189,9 +189,18 @@ public void dumpConf(PrintWriter pwriter) {
         pwriter.print(self.getQuorumVerifier().toString());
     }
 
+    private final Object stateChangeMutex = new Object();
+
     @Override
     protected void setState(State state) {
-        this.state = state;
+        synchronized (stateChangeMutex) {
+            if (this.state == State.ERROR) {
+                if (state == State.RUNNING || state == State.INITIAL) {
+                    return;

Review comment:
       To add the test, do we need to consider using a fault injection tool? See [ZOOKEEPER-3601](https://issues.apache.org/jira/browse/ZOOKEEPER-3601) and https://github.com/apache/zookeeper/pull/1135




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [zookeeper] functioner edited a comment on pull request #1596: ZOOKEEPER-4203: Leader swallows the ZooKeeperServer.State.ERROR from Leader.LearnerCnxAcceptor in some concurrency condition

Posted by GitBox <gi...@apache.org>.
functioner edited a comment on pull request #1596:
URL: https://github.com/apache/zookeeper/pull/1596#issuecomment-791988099


   > One thing I am wondering (which was not introduced by your code) is why we catch and continue on `SaslException`s, but "crash" on other `IOException`s? Not a major point as the server will now recover, but perhaps we could just accept that `accept` can fail? @eolivelli, @symat?
   
   @ztzg I think it can be explained with:
   https://github.com/apache/zookeeper/blob/0c98d1d3252e645a5c25bfecaba5ca1f5cd3258e/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/Leader.java#L523-L536
   In the case of `SaslException`, the exception is not thrown again, then it will not be caught by:
   https://github.com/apache/zookeeper/blob/0c98d1d3252e645a5c25bfecaba5ca1f5cd3258e/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/Leader.java#L498-L504
   In this case, `ZooKeeperServer.State.ERROR` is not set, everything works well temporally, and the critical code
   https://github.com/apache/zookeeper/blob/0c98d1d3252e645a5c25bfecaba5ca1f5cd3258e/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/Leader.java#L688
   finishes, so, if there is any error later, it can be detect by:
   https://github.com/apache/zookeeper/blob/0c98d1d3252e645a5c25bfecaba5ca1f5cd3258e/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/Leader.java#L754
   Then, the quorum can handle it well.
   
   However, if the IOException is not `SaslException`, it will be thrown again by (line 528 or 535):
   https://github.com/apache/zookeeper/blob/0c98d1d3252e645a5c25bfecaba5ca1f5cd3258e/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/Leader.java#L523-L536
   In this case, the `ZooKeeperServer.State.ERROR` may be set before the critical code:
   https://github.com/apache/zookeeper/blob/0c98d1d3252e645a5c25bfecaba5ca1f5cd3258e/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/Leader.java#L688
   Then this error state is covered by running state. The symptom described by this issue occurs.


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [zookeeper] functioner commented on pull request #1596: ZOOKEEPER-4203: Leader swallows the ZooKeeperServer.State.ERROR from Leader.LearnerCnxAcceptor in some concurrency condition

Posted by GitBox <gi...@apache.org>.
functioner commented on pull request #1596:
URL: https://github.com/apache/zookeeper/pull/1596#issuecomment-984209856


   @eolivelli The state check & modification within `setState` method should be atomic as a whole, so `AtomicReference` is not enough (including the `updateAndGet` methods and so on). Now I use `synchronized` for the methods read/write `state`.


-- 
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: notifications-unsubscribe@zookeeper.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [zookeeper] TisonKun commented on pull request #1596: ZOOKEEPER-4203: Leader swallows the ZooKeeperServer.State.ERROR from Leader.LearnerCnxAcceptor in some concurrency condition

Posted by GitBox <gi...@apache.org>.
TisonKun commented on pull request #1596:
URL: https://github.com/apache/zookeeper/pull/1596#issuecomment-785066590


   retest please


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [zookeeper] functioner commented on pull request #1596: ZOOKEEPER-4203: Leader swallows the ZooKeeperServer.State.ERROR from Leader.LearnerCnxAcceptor in some concurrency condition

Posted by GitBox <gi...@apache.org>.
functioner commented on pull request #1596:
URL: https://github.com/apache/zookeeper/pull/1596#issuecomment-791988099


   > One thing I am wondering (which was not introduced by your code) is why we catch and continue on `SaslException`s, but "crash" on other `IOException`s? Not a major point as the server will now recover, but perhaps we could just accept that `accept` can fail? @eolivelli, @symat?
   
   @ztzg I think it can be explained with:
   https://github.com/apache/zookeeper/blob/0c98d1d3252e645a5c25bfecaba5ca1f5cd3258e/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/Leader.java#L523-L536
   In the case of `SaslException`, the exception is not thrown again, then it will not be caught by:
   https://github.com/apache/zookeeper/blob/0c98d1d3252e645a5c25bfecaba5ca1f5cd3258e/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/Leader.java#L498-L504
   In this case, `ZooKeeperServer.State.ERROR` is not set, everything works well temporally, and the critical code
   https://github.com/apache/zookeeper/blob/0c98d1d3252e645a5c25bfecaba5ca1f5cd3258e/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/Leader.java#L688
   finishes, so, if there is any error later, it can be detect by:
   https://github.com/apache/zookeeper/blob/0c98d1d3252e645a5c25bfecaba5ca1f5cd3258e/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/Leader.java#L754
   
   However, if the IOException is not `SaslException`, it will be thrown again by:
   https://github.com/apache/zookeeper/blob/0c98d1d3252e645a5c25bfecaba5ca1f5cd3258e/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/Leader.java#L523-L536
   In this case, the `ZooKeeperServer.State.ERROR` may be set before the critical code:
   https://github.com/apache/zookeeper/blob/0c98d1d3252e645a5c25bfecaba5ca1f5cd3258e/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/Leader.java#L688
   Then this error state is covered by running state.


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [zookeeper] sonatype-lift[bot] commented on a change in pull request #1596: ZOOKEEPER-4203: Leader swallows the ZooKeeperServer.State.ERROR from Leader.LearnerCnxAcceptor in some concurrency condition

Posted by GitBox <gi...@apache.org>.
sonatype-lift[bot] commented on a change in pull request #1596:
URL: https://github.com/apache/zookeeper/pull/1596#discussion_r760708722



##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/Leader.java
##########
@@ -457,7 +461,7 @@ public void run() {
                 CountDownLatch latch = new CountDownLatch(serverSockets.size());
 
                 serverSockets.forEach(serverSocket ->
-                        executor.submit(new LearnerCnxAcceptorHandler(serverSocket, latch)));
+                        executor.submit(createLearnerCnxAcceptorHandler(serverSocket, latch)));

Review comment:
       I've recorded this as ignored for this pull request. If you change your mind, just comment `@sonatype-lift unignore`.




-- 
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: notifications-unsubscribe@zookeeper.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [zookeeper] lanicc commented on pull request #1596: ZOOKEEPER-4203: Leader swallows the ZooKeeperServer.State.ERROR from Leader.LearnerCnxAcceptor in some concurrency condition

Posted by GitBox <gi...@apache.org>.
lanicc commented on pull request #1596:
URL: https://github.com/apache/zookeeper/pull/1596#issuecomment-803547449


   I think the exception in Leader.LearnerCnxAcceptor should not be handled by zkServer, but should be handled internally by Leader.Because Acceptor and zkServer are not closely related, both Acceptor and zkServer are used as internal components of Leader, and Leader combines them to complete the function of leader.
   When an exception occurs in Leader.LearnerCnxAcceptor,the error status should be fed back to Leader, that is, Leader.isRunning can find this error status.
   My solution:
   - Leader.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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [zookeeper] TisonKun commented on a change in pull request #1596: ZOOKEEPER-4203: Leader swallows the ZooKeeperServer.State.ERROR from Leader.LearnerCnxAcceptor in some concurrency condition

Posted by GitBox <gi...@apache.org>.
TisonKun commented on a change in pull request #1596:
URL: https://github.com/apache/zookeeper/pull/1596#discussion_r581989933



##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/QuorumZooKeeperServer.java
##########
@@ -189,9 +189,18 @@ public void dumpConf(PrintWriter pwriter) {
         pwriter.print(self.getQuorumVerifier().toString());
     }
 
+    private final Object stateChangeMutex = new Object();
+
     @Override
     protected void setState(State state) {

Review comment:
       After a closer look I find that setState can be a single implementation in `ZooKeeperServer`. Just as is there. And you can apply this check as well as logging to verify as @eolivelli said.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [zookeeper] functioner commented on a change in pull request #1596: ZOOKEEPER-4203: Leader swallows the ZooKeeperServer.State.ERROR from Leader.LearnerCnxAcceptor in some concurrency condition

Posted by GitBox <gi...@apache.org>.
functioner commented on a change in pull request #1596:
URL: https://github.com/apache/zookeeper/pull/1596#discussion_r588902426



##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/QuorumZooKeeperServer.java
##########
@@ -189,9 +189,18 @@ public void dumpConf(PrintWriter pwriter) {
         pwriter.print(self.getQuorumVerifier().toString());
     }
 
+    private final Object stateChangeMutex = new Object();
+
     @Override
     protected void setState(State state) {

Review comment:
       > But @Tison's point still holds; it seems to me that this could be simplified by making `setState()` a `synchronized` method. Do you have a specific reason of using a separate object?
   > 
   > (The parent `setState()` does not _need_ to be `synchronized` because the field is `volatile`.)
   
   @ztzg The rationale for not making `setState()` a synchronized method is that if a thread is running another synchronized method of this ZooKeeperServer object for some time, it may block the `setState()` invoked by another thread, and this invocation may be critical.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [zookeeper] functioner commented on pull request #1596: ZOOKEEPER-4203: Leader swallows the ZooKeeperServer.State.ERROR from Leader.LearnerCnxAcceptor in some concurrency condition

Posted by GitBox <gi...@apache.org>.
functioner commented on pull request #1596:
URL: https://github.com/apache/zookeeper/pull/1596#issuecomment-806259802


   > Follower can do the same.
   
   @lanicc, IMO, the root problem this issue exposes is that the exception caught by `ZooKeeperCriticalThread` is translated into the state change in zkServer. Even if you deal with Acceptor separately in Leader. There could be other similar issues in `ZooKeeperCriticalThread`, affecting Leader or Follower. Either way, the logic of state change needs improvement, either in zkServer, or in Leader/Follower as you proposed. Maybe the fix you provided here is reasonable, but I think it should be a separate PR, dealing with Acceptor. The current PR should focus on improving the state change logic.
   
   What do you think? @eolivelli @ztzg 


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [zookeeper] lanicc removed a comment on pull request #1596: ZOOKEEPER-4203: Leader swallows the ZooKeeperServer.State.ERROR from Leader.LearnerCnxAcceptor in some concurrency condition

Posted by GitBox <gi...@apache.org>.
lanicc removed a comment on pull request #1596:
URL: https://github.com/apache/zookeeper/pull/1596#issuecomment-803547449


   I think the exception in Leader.LearnerCnxAcceptor should not be handled by zkServer, but should be handled internally by Leader.Because Acceptor and zkServer are not closely related, both Acceptor and zkServer are used as internal components of Leader, and Leader combines them to complete the function of leader.
   When an exception occurs in Leader.LearnerCnxAcceptor,the error status should be fed back to Leader, that is, Leader.isRunning can find this error status.
   My solution:
   - Leader.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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [zookeeper] eolivelli commented on pull request #1596: ZOOKEEPER-4203: Leader swallows the ZooKeeperServer.State.ERROR from Leader.LearnerCnxAcceptor in some concurrency condition

Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #1596:
URL: https://github.com/apache/zookeeper/pull/1596#issuecomment-785067488


   @TisonKun I am sorry but the magic word does not work anymore.
   Let me force a rerun manually


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [zookeeper] functioner commented on pull request #1596: ZOOKEEPER-4203: Leader swallows the ZooKeeperServer.State.ERROR from Leader.LearnerCnxAcceptor in some concurrency condition

Posted by GitBox <gi...@apache.org>.
functioner commented on pull request #1596:
URL: https://github.com/apache/zookeeper/pull/1596#issuecomment-774555959


   I believe that in the CI server, multiple test cases fail due to the network connection binding issue, and the QuorumMainTest fails because the ZK snapshot is compromised by some test cases that have failed. Thus, I think this patch can be merge into master if the reviewers agree with my design.


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [zookeeper] functioner commented on a change in pull request #1596: ZOOKEEPER-4203: Leader swallows the ZooKeeperServer.State.ERROR from Leader.LearnerCnxAcceptor in some concurrency condition

Posted by GitBox <gi...@apache.org>.
functioner commented on a change in pull request #1596:
URL: https://github.com/apache/zookeeper/pull/1596#discussion_r582098731



##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/QuorumZooKeeperServer.java
##########
@@ -189,9 +189,18 @@ public void dumpConf(PrintWriter pwriter) {
         pwriter.print(self.getQuorumVerifier().toString());
     }
 
+    private final Object stateChangeMutex = new Object();
+
     @Override
     protected void setState(State state) {
-        this.state = state;
+        synchronized (stateChangeMutex) {
+            if (this.state == State.ERROR) {
+                if (state == State.RUNNING || state == State.INITIAL) {
+                    return;

Review comment:
       To add the test, do we need to consider using a fault injection tool? See [ZOOKEEPER-3601](https://issues.apache.org/jira/browse/ZOOKEEPER-3601) and https://github.com/apache/zookeeper/pull/1135. I have provided Byteman injection script in [ZOOKEEPER-4203](https://issues.apache.org/jira/browse/ZOOKEEPER-4203) so it can be somehow translated into a test.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [zookeeper] functioner edited a comment on pull request #1596: ZOOKEEPER-4203: Leader swallows the ZooKeeperServer.State.ERROR from Leader.LearnerCnxAcceptor in some concurrency condition

Posted by GitBox <gi...@apache.org>.
functioner edited a comment on pull request #1596:
URL: https://github.com/apache/zookeeper/pull/1596#issuecomment-984209856


   @eolivelli The state check & modification within `setState` method should be atomic as a whole, so `AtomicReference` is not enough (including the `updateAndGet` methods and so on). Now I use `synchronized` for the methods which read/write `state`.


-- 
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: notifications-unsubscribe@zookeeper.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [zookeeper] sonatype-lift[bot] commented on a change in pull request #1596: ZOOKEEPER-4203: Leader swallows the ZooKeeperServer.State.ERROR from Leader.LearnerCnxAcceptor in some concurrency condition

Posted by GitBox <gi...@apache.org>.
sonatype-lift[bot] commented on a change in pull request #1596:
URL: https://github.com/apache/zookeeper/pull/1596#discussion_r760706130



##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/Leader.java
##########
@@ -457,7 +461,7 @@ public void run() {
                 CountDownLatch latch = new CountDownLatch(serverSockets.size());
 
                 serverSockets.forEach(serverSocket ->
-                        executor.submit(new LearnerCnxAcceptorHandler(serverSocket, latch)));
+                        executor.submit(createLearnerCnxAcceptorHandler(serverSocket, latch)));

Review comment:
       *FutureReturnValueIgnored:*  Return value of methods returning Future must be checked. Ignoring returned Futures suppresses exceptions thrown from the code that completes the Future. [(details)](https://errorprone.info/bugpattern/FutureReturnValueIgnored)
   (at-me [in a reply](https://help.sonatype.com/lift/talking-to-lift) with `help` or `ignore`)




-- 
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: notifications-unsubscribe@zookeeper.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [zookeeper] functioner commented on a change in pull request #1596: ZOOKEEPER-4203: Leader swallows the ZooKeeperServer.State.ERROR from Leader.LearnerCnxAcceptor in some concurrency condition

Posted by GitBox <gi...@apache.org>.
functioner commented on a change in pull request #1596:
URL: https://github.com/apache/zookeeper/pull/1596#discussion_r760708695



##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/Leader.java
##########
@@ -457,7 +461,7 @@ public void run() {
                 CountDownLatch latch = new CountDownLatch(serverSockets.size());
 
                 serverSockets.forEach(serverSocket ->
-                        executor.submit(new LearnerCnxAcceptorHandler(serverSocket, latch)));
+                        executor.submit(createLearnerCnxAcceptorHandler(serverSocket, latch)));

Review comment:
       @sonatype-lift ignore




-- 
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: notifications-unsubscribe@zookeeper.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [zookeeper] eolivelli commented on a change in pull request #1596: ZOOKEEPER-4203: Leader swallows the ZooKeeperServer.State.ERROR from Leader.LearnerCnxAcceptor in some concurrency condition

Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #1596:
URL: https://github.com/apache/zookeeper/pull/1596#discussion_r759023451



##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/QuorumZooKeeperServer.java
##########
@@ -189,9 +189,18 @@ public void dumpConf(PrintWriter pwriter) {
         pwriter.print(self.getQuorumVerifier().toString());
     }
 
+    private final Object stateChangeMutex = new Object();
+
     @Override
     protected void setState(State state) {
-        this.state = state;
+        synchronized (stateChangeMutex) {
+            if (this.state == State.ERROR) {
+                if (state == State.RUNNING || state == State.INITIAL) {
+                    return;

Review comment:
       if you want to use `stateChangeMutex` to handle concurrent access to `state` you have to use it at every read/write access.
   
   Using AtomicReference is better and easier to understand.
   I suggest to switch to `AtomicReference` and drop `stateChangeMutex`




-- 
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: notifications-unsubscribe@zookeeper.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [zookeeper] functioner edited a comment on pull request #1596: ZOOKEEPER-4203: Leader swallows the ZooKeeperServer.State.ERROR from Leader.LearnerCnxAcceptor in some concurrency condition

Posted by GitBox <gi...@apache.org>.
functioner edited a comment on pull request #1596:
URL: https://github.com/apache/zookeeper/pull/1596#issuecomment-791988099


   > One thing I am wondering (which was not introduced by your code) is why we catch and continue on `SaslException`s, but "crash" on other `IOException`s? Not a major point as the server will now recover, but perhaps we could just accept that `accept` can fail? @eolivelli, @symat?
   
   @ztzg I think it can be explained with:
   https://github.com/apache/zookeeper/blob/0c98d1d3252e645a5c25bfecaba5ca1f5cd3258e/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/Leader.java#L523-L536
   In the case of `SaslException`, the exception is not thrown again, then it will not be caught by:
   https://github.com/apache/zookeeper/blob/0c98d1d3252e645a5c25bfecaba5ca1f5cd3258e/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/Leader.java#L498-L504
   In this case, `ZooKeeperServer.State.ERROR` is not set, everything works well temporally, and the critical code
   https://github.com/apache/zookeeper/blob/0c98d1d3252e645a5c25bfecaba5ca1f5cd3258e/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/Leader.java#L688
   finishes, so, if there is any error later, it can be detect by:
   https://github.com/apache/zookeeper/blob/0c98d1d3252e645a5c25bfecaba5ca1f5cd3258e/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/Leader.java#L754
   Then, the quorum can handle it well.
   
   However, if the IOException is not `SaslException`, it will be thrown again by (line 528 or 535):
   https://github.com/apache/zookeeper/blob/0c98d1d3252e645a5c25bfecaba5ca1f5cd3258e/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/Leader.java#L523-L536
   In this case, the `ZooKeeperServer.State.ERROR` may be set before the critical code:
   https://github.com/apache/zookeeper/blob/0c98d1d3252e645a5c25bfecaba5ca1f5cd3258e/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/Leader.java#L688
   Then this error state is covered by the running state. The symptom described by this issue occurs.


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [zookeeper] functioner edited a comment on pull request #1596: ZOOKEEPER-4203: Leader swallows the ZooKeeperServer.State.ERROR from Leader.LearnerCnxAcceptor in some concurrency condition

Posted by GitBox <gi...@apache.org>.
functioner edited a comment on pull request #1596:
URL: https://github.com/apache/zookeeper/pull/1596#issuecomment-791988099


   > One thing I am wondering (which was not introduced by your code) is why we catch and continue on `SaslException`s, but "crash" on other `IOException`s? Not a major point as the server will now recover, but perhaps we could just accept that `accept` can fail? @eolivelli, @symat?
   
   @ztzg I think it can be explained with:
   https://github.com/apache/zookeeper/blob/0c98d1d3252e645a5c25bfecaba5ca1f5cd3258e/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/Leader.java#L523-L536
   In the case of `SaslException`, the exception is not thrown again, then it will not be caught by:
   https://github.com/apache/zookeeper/blob/0c98d1d3252e645a5c25bfecaba5ca1f5cd3258e/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/Leader.java#L498-L504
   In this case, `ZooKeeperServer.State.ERROR` is not set, everything works well temporally, and the critical code
   https://github.com/apache/zookeeper/blob/0c98d1d3252e645a5c25bfecaba5ca1f5cd3258e/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/Leader.java#L688
   finishes, so, if there is any error later, it can be detect by:
   https://github.com/apache/zookeeper/blob/0c98d1d3252e645a5c25bfecaba5ca1f5cd3258e/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/Leader.java#L754
   Then, the quorum can handle it well.
   
   However, if the IOException is not `SaslException`, it will be thrown again by:
   https://github.com/apache/zookeeper/blob/0c98d1d3252e645a5c25bfecaba5ca1f5cd3258e/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/Leader.java#L523-L536
   In this case, the `ZooKeeperServer.State.ERROR` may be set before the critical code:
   https://github.com/apache/zookeeper/blob/0c98d1d3252e645a5c25bfecaba5ca1f5cd3258e/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/Leader.java#L688
   Then this error state is covered by running state. The symptom described by this issue occurs.


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [zookeeper] functioner commented on a change in pull request #1596: ZOOKEEPER-4203: Leader swallows the ZooKeeperServer.State.ERROR from Leader.LearnerCnxAcceptor in some concurrency condition

Posted by GitBox <gi...@apache.org>.
functioner commented on a change in pull request #1596:
URL: https://github.com/apache/zookeeper/pull/1596#discussion_r588907627



##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/QuorumZooKeeperServer.java
##########
@@ -189,9 +189,18 @@ public void dumpConf(PrintWriter pwriter) {
         pwriter.print(self.getQuorumVerifier().toString());
     }
 
+    private final Object stateChangeMutex = new Object();
+
     @Override
     protected void setState(State state) {
-        this.state = state;
+        synchronized (stateChangeMutex) {
+            if (this.state == State.ERROR) {
+                if (state == State.RUNNING || state == State.INITIAL) {
+                    return;

Review comment:
       > 100% agree with @eolivelli on adding a `LOG.warn` (if not `LOG.error`).
   
   Okay, I will add the log later.
   
   > For the test, I would suggest mocking and/or overriding some methods to explicitly generate the fault, if possible. I haven't tried doing so, but perhaps `LearnerTest.java` (which subclasses `LearnerZooKeeperServer`) can serve as inspiration?
   
   Okay, I will take a try.
   BTW, I think introducing a fault injection tool like https://github.com/apache/zookeeper/pull/1135 may make it easier to write a test for it, and is helpful for the community. Another bug I propose (https://github.com/apache/zookeeper/pull/1582) also require such tool to reproduce/test. I commented at https://github.com/apache/zookeeper/pull/1135 but haven't got response. Do you have any suggestion or opinion? Thank you.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [zookeeper] eolivelli commented on pull request #1596: ZOOKEEPER-4203: Leader swallows the ZooKeeperServer.State.ERROR from Leader.LearnerCnxAcceptor in some concurrency condition

Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #1596:
URL: https://github.com/apache/zookeeper/pull/1596#issuecomment-785068071


   I have restarted the github actions job, but the Jenkins job is too old.
   I guess you have to rebase/merge with current master and push


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [zookeeper] ztzg commented on a change in pull request #1596: ZOOKEEPER-4203: Leader swallows the ZooKeeperServer.State.ERROR from Leader.LearnerCnxAcceptor in some concurrency condition

Posted by GitBox <gi...@apache.org>.
ztzg commented on a change in pull request #1596:
URL: https://github.com/apache/zookeeper/pull/1596#discussion_r588897003



##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/QuorumZooKeeperServer.java
##########
@@ -189,9 +189,18 @@ public void dumpConf(PrintWriter pwriter) {
         pwriter.print(self.getQuorumVerifier().toString());
     }
 
+    private final Object stateChangeMutex = new Object();
+
     @Override
     protected void setState(State state) {
-        this.state = state;
+        synchronized (stateChangeMutex) {
+            if (this.state == State.ERROR) {
+                if (state == State.RUNNING || state == State.INITIAL) {
+                    return;

Review comment:
       100% agree with @eolivelli on adding a `LOG.warn` (if not `LOG.error`).
   
   For the test, I would suggest mocking and/or overriding some methods to explicitly generate the fault, if possible.  I haven't tried doing so, but perhaps `LearnerTest.java` (which subclasses `LearnerZooKeeperServer`) can serve as inspiration?

##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/QuorumZooKeeperServer.java
##########
@@ -189,9 +189,18 @@ public void dumpConf(PrintWriter pwriter) {
         pwriter.print(self.getQuorumVerifier().toString());
     }
 
+    private final Object stateChangeMutex = new Object();
+
     @Override
     protected void setState(State state) {

Review comment:
       I agree with @eolivelli that `setState()` has to be "careful," because `ZooKeeperServerListenerImpl.notifyStopping` calls it from "random" threads.
   
   But @tison's point still holds; it seems to me that this could be simplified by making `setState()` a `synchronized` method.  Do you have a specific reason of using a separate object?
   
   (The parent `setState()` does not *need* to be `synchronized` because the field is `volatile`.)




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [zookeeper] eolivelli commented on a change in pull request #1596: ZOOKEEPER-4203: Leader swallows the ZooKeeperServer.State.ERROR from Leader.LearnerCnxAcceptor in some concurrency condition

Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #1596:
URL: https://github.com/apache/zookeeper/pull/1596#discussion_r581952952



##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/QuorumZooKeeperServer.java
##########
@@ -189,9 +189,18 @@ public void dumpConf(PrintWriter pwriter) {
         pwriter.print(self.getQuorumVerifier().toString());
     }
 
+    private final Object stateChangeMutex = new Object();
+
     @Override
     protected void setState(State state) {
-        this.state = state;
+        synchronized (stateChangeMutex) {
+            if (this.state == State.ERROR) {
+                if (state == State.RUNNING || state == State.INITIAL) {
+                    return;

Review comment:
       we should add a relevant log in order to see if this case is happening.
   
   Creating a test that reproduces the problem and demonstrates that this fix actually resolves the problem would be better
   
   




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [zookeeper] functioner commented on a change in pull request #1596: ZOOKEEPER-4203: Leader swallows the ZooKeeperServer.State.ERROR from Leader.LearnerCnxAcceptor in some concurrency condition

Posted by GitBox <gi...@apache.org>.
functioner commented on a change in pull request #1596:
URL: https://github.com/apache/zookeeper/pull/1596#discussion_r588906560



##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/QuorumZooKeeperServer.java
##########
@@ -189,9 +189,18 @@ public void dumpConf(PrintWriter pwriter) {
         pwriter.print(self.getQuorumVerifier().toString());
     }
 
+    private final Object stateChangeMutex = new Object();
+
     @Override
     protected void setState(State state) {

Review comment:
       > I agree with @eolivelli that `setState()` has to be "careful," because `ZooKeeperServerListenerImpl.notifyStopping` calls it from "random" threads.
   
   Actually, the single-node ZooKeeperServer can handle this issue with the handler:
   https://github.com/apache/zookeeper/blob/0c98d1d3252e645a5c25bfecaba5ca1f5cd3258e/zookeeper-server/src/main/java/org/apache/zookeeper/server/ZooKeeperServer.java#L779-L789
   because it is registered at the very beginning:
   https://github.com/apache/zookeeper/blob/0c98d1d3252e645a5c25bfecaba5ca1f5cd3258e/zookeeper-server/src/main/java/org/apache/zookeeper/server/ZooKeeperServerMain.java#L150
   This handling logic is:
   https://github.com/apache/zookeeper/blob/0c98d1d3252e645a5c25bfecaba5ca1f5cd3258e/zookeeper-server/src/main/java/org/apache/zookeeper/server/ZooKeeperServerShutdownHandler.java#L42-L46
   and then
   https://github.com/apache/zookeeper/blob/0c98d1d3252e645a5c25bfecaba5ca1f5cd3258e/zookeeper-server/src/main/java/org/apache/zookeeper/server/ZooKeeperServerMain.java#L181-L185
   
   However, in the case of `QuorumZooKeeperServer`, including Leader, Follower, etc., the aforementioned logic is not used.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [zookeeper] lanicc commented on a change in pull request #1596: ZOOKEEPER-4203: Leader swallows the ZooKeeperServer.State.ERROR from Leader.LearnerCnxAcceptor in some concurrency condition

Posted by GitBox <gi...@apache.org>.
lanicc commented on a change in pull request #1596:
URL: https://github.com/apache/zookeeper/pull/1596#discussion_r594356625



##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/QuorumZooKeeperServer.java
##########
@@ -189,9 +189,18 @@ public void dumpConf(PrintWriter pwriter) {
         pwriter.print(self.getQuorumVerifier().toString());
     }
 
+    private final Object stateChangeMutex = new Object();
+
     @Override
     protected void setState(State state) {
-        this.state = state;
+        synchronized (stateChangeMutex) {
+            if (this.state == State.ERROR) {
+                if (state == State.RUNNING || state == State.INITIAL) {
+                    return;

Review comment:
       Hi @functioner, I am very interested in this problem, I have written a test case, I don't konw if it can be used to test this problem.Maybe you can use it as a reference.
   [TestCase](https://github.com/lanicc/zookeeper/blob/state_byteman/zookeeper-server/src/test/java/org/apache/zookeeper/server/quorum/LeaderLeadingStateTest.java)
   [Byteman btm script](https://github.com/lanicc/zookeeper/blob/state_byteman/zookeeper-server/src/test/resources/test-leader-leading-state.btm)




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [zookeeper] functioner edited a comment on pull request #1596: ZOOKEEPER-4203: Leader swallows the ZooKeeperServer.State.ERROR from Leader.LearnerCnxAcceptor in some concurrency condition

Posted by GitBox <gi...@apache.org>.
functioner edited a comment on pull request #1596:
URL: https://github.com/apache/zookeeper/pull/1596#issuecomment-791988099


   > One thing I am wondering (which was not introduced by your code) is why we catch and continue on `SaslException`s, but "crash" on other `IOException`s? Not a major point as the server will now recover, but perhaps we could just accept that `accept` can fail? @eolivelli, @symat?
   
   @ztzg I think it can be explained with:
   https://github.com/apache/zookeeper/blob/0c98d1d3252e645a5c25bfecaba5ca1f5cd3258e/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/Leader.java#L523-L536
   In the case of `SaslException`, the exception is not thrown again, then it will not be caught by:
   https://github.com/apache/zookeeper/blob/0c98d1d3252e645a5c25bfecaba5ca1f5cd3258e/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/Leader.java#L498-L504
   In this case, `ZooKeeperServer.State.ERROR` is not set, everything works well temporally, and the critical code
   https://github.com/apache/zookeeper/blob/0c98d1d3252e645a5c25bfecaba5ca1f5cd3258e/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/Leader.java#L688
   finishes, so, if there is any error later, it can be detect by:
   https://github.com/apache/zookeeper/blob/0c98d1d3252e645a5c25bfecaba5ca1f5cd3258e/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/Leader.java#L754
   Then, the quorum can handle it well.
   
   However, if the IOException is not `SaslException`, it will be thrown again by (line 582 or 535):
   https://github.com/apache/zookeeper/blob/0c98d1d3252e645a5c25bfecaba5ca1f5cd3258e/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/Leader.java#L523-L536
   In this case, the `ZooKeeperServer.State.ERROR` may be set before the critical code:
   https://github.com/apache/zookeeper/blob/0c98d1d3252e645a5c25bfecaba5ca1f5cd3258e/zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/Leader.java#L688
   Then this error state is covered by running state. The symptom described by this issue occurs.


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [zookeeper] TisonKun commented on pull request #1596: ZOOKEEPER-4203: Leader swallows the ZooKeeperServer.State.ERROR from Leader.LearnerCnxAcceptor in some concurrency condition

Posted by GitBox <gi...@apache.org>.
TisonKun commented on pull request #1596:
URL: https://github.com/apache/zookeeper/pull/1596#issuecomment-785066768


   retest please


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [zookeeper] lanicc commented on pull request #1596: ZOOKEEPER-4203: Leader swallows the ZooKeeperServer.State.ERROR from Leader.LearnerCnxAcceptor in some concurrency condition

Posted by GitBox <gi...@apache.org>.
lanicc commented on pull request #1596:
URL: https://github.com/apache/zookeeper/pull/1596#issuecomment-803549126


   I think the exception in Leader.LearnerCnxAcceptor should not be handled by zkServer, but should be handled internally by Leader.Because Acceptor and zkServer are not closely related, both Acceptor and zkServer are used as internal components of Leader, and Leader combines them to complete the function of leader.
   When an exception occurs in Leader.LearnerCnxAcceptor,the error status should be fed back to Leader, that is, Leader.isRunning can find this error status.
   My solution:
   - Leader.isRunning
   ![image](https://user-images.githubusercontent.com/48583080/111901210-84ddd480-8a71-11eb-91b2-cfa8751afe87.png)
   - Make Leader.LearnerCnxAcceptor extends ZooKeeperThread
   ![image](https://user-images.githubusercontent.com/48583080/111901247-cff7e780-8a71-11eb-85dc-86ee0169d667.png)
   
   Follower can do the same.
   


-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org