ZOOKEEPER-2466: Fix C client skipping servers when reconnecting

zookeeper-client/zookeeper-client-c/include/zookeeper.h

@@ -843,9 +843,9 @@ ZOOAPI int zoo_set_servers_resolution_delay(zhandle_t *zh, int delay_ms);
  *
  * This method allows a client to cycle through the list of servers in it's
  * connection pool to be used on the next connection attempt. This function does
- * not actually trigger a connection or state change in any way. Its purpose is
- * to allow testing changing servers on the fly and the probabilistic load
- * balancing algorithm.
+ * not actually trigger a connection, but it updates the handle with the address
+ * of the next server to connect to. Its purpose is to allow testing changing
+ * servers on the fly and the probabilistic load balancing algorithm.
  */
 ZOOAPI void zoo_cycle_next_server(zhandle_t *zh);
 

zookeeper-client/zookeeper-client-c/src/zookeeper.c

@@ -1604,6 +1604,8 @@ void zoo_cycle_next_server(zhandle_t *zh)
         zh->reconfig = 0;
     }
 
+    // Delay the next attempt when this selection wraps the server list.
+    zh->delay = addrvec_atend(&zh->addrs);
     addrvec_next(&zh->addrs, &zh->addr_cur);
 
     unlock_reconfig(zh);
@@ -2030,11 +2032,6 @@ static void cleanup(zhandle_t *zh,int rc)
 static void handle_error(zhandle_t *zh,int rc)
 {
     cleanup(zh, rc);
-    // NOTE: If we're at the end of the list of addresses to connect to, then
-    // we want to delay the next connection attempt to avoid spinning.
-    // Then increment what host we'll connect to since we failed to connect to current
-    zh->delay = addrvec_atend(&zh->addrs);
-    addrvec_next(&zh->addrs, &zh->addr_cur);
 }
 
 static int handle_socket_error_msg(zhandle_t *zh, int line, const char *func, int rc,
@@ -2694,7 +2691,11 @@ int zookeeper_interest(zhandle_t *zh, socket_t *fd, int *interest,
                              format_endpoint_info(&addr));
                     cleanup(zh, ZOK);
                 } else {
-                    addrvec_next(&zh->addrs, NULL);
+                    lock_reconfig(zh);
+                    if (!zh->reconfig) {
+                        addrvec_next(&zh->addrs, NULL);
+                    }
+                    unlock_reconfig(zh);
                 }
             }
             send_to = min(send_to, zh->ping_rw_timeout - idle_ping_rw);

zookeeper-client/zookeeper-client-c/tests/TestClient.cc

@@ -345,11 +345,26 @@ class Zookeeper_simpleSystem : public CPPUNIT_NS::TestFixture
     /** ZOOKEEPER-1057 This checks that the client connects to the second server when the first is not reachable **/
     void testFirstServerDown() {
         watchctx_t ctx;
+        const char *hosts = "127.0.0.1:22182,127.0.0.1:22181";
+        const char *server = "127.0.0.1:22181";
+        char cmd[1024];
 
         zoo_deterministic_conn_order(true);
 
-        zhandle_t* zk = createClient("127.0.0.1:22182,127.0.0.1:22181", &ctx);
-        CPPUNIT_ASSERT(zk != 0);
+        // ZOOKEEPER-2466: exhaust the server list before making the second
+        // server available, so reconnecting must not skip it.
+        snprintf(cmd, sizeof(cmd), "%s stop %s", ZKSERVER_CMD, server);
+        CPPUNIT_ASSERT(system(cmd) == 0);
+
+        zhandle_t* zk = createClient(hosts, &ctx);
+        bool clientCreated = zk != 0;
+        bool connectedWhileStopped = clientCreated && ctx.waitForConnected(zk);
+
+        snprintf(cmd, sizeof(cmd), "%s startClean %s", ZKSERVER_CMD, server);
+        CPPUNIT_ASSERT(system(cmd) == 0);
+
+        CPPUNIT_ASSERT(clientCreated);
+        CPPUNIT_ASSERT(!connectedWhileStopped);
         CPPUNIT_ASSERT(ctx.waitForConnected(zk));
     }