client: check ready futures in waitAll and waitCount before waiting

Currently, `waitAll` and `waitCount` unconditionally `wait` instead of
checking response readiness. If there are no more responses, it will cause
them to hang indefinitely. To fix this, let's check the response
readiness first. We should also move the time start to the beginning of the
waiting loop, since the initial response checking overhead should not be
accounted for the waiting time.

Closes #133

Author: CuriousGeorgiy

src/Client/Connector.hpp

@@ -309,16 +309,20 @@ Connector<BUFFER, NetProvider>::waitAll(Connection<BUFFER, NetProvider> &conn,
 					const std::vector<rid_t> &futures,
 					int timeout)
 {
+	size_t last_not_ready = 0;
+	bool finish = false;
+	if (connectionCheckResponsesReadiness(conn, futures, &last_not_ready, &finish) != 0)
+		return -1;
+	if (finish)
+		return 0;
 	Timer timer{timeout};
 	timer.start();
-	size_t last_not_ready = 0;
 	while (!conn.hasError()) {
 		if (m_NetProvider.wait(timer.timeLeft()) != 0) {
 			conn.setError(std::string("Failed to poll: ") +
 				      strerror(errno), errno);
 			return -1;
 		}
-		bool finish = false;
 		if (connectionCheckResponsesReadiness(conn, futures, &last_not_ready, &finish) != 0)
 			return -1;
 		if (finish)
@@ -361,17 +365,21 @@ int
 Connector<BUFFER, NetProvider>::waitCount(Connection<BUFFER, NetProvider> &conn,
 					  size_t future_count, int timeout)
 {
-	Timer timer{timeout};
-	timer.start();
 	size_t ready_futures = conn.getFutureCount();
 	size_t expected_future_count = ready_futures + future_count;
+	bool finish = false;
+	if (connectionCheckCountResponsesReadiness(conn, expected_future_count, &finish) != 0)
+		return -1;
+	if (finish)
+		return 0;
+	Timer timer{timeout};
+	timer.start();
 	while (!conn.hasError()) {
 		if (m_NetProvider.wait(timer.timeLeft()) != 0) {
 			conn.setError(std::string("Failed to poll: ") +
 				      strerror(errno), errno);
 			return -1;
 		}
-		bool finish = false;
 		if (connectionCheckCountResponsesReadiness(conn, expected_future_count, &finish) != 0)
 			return -1;
 		if (finish)

test/ClientTest.cpp

@@ -1324,6 +1324,28 @@ test_wait(Connector<BUFFER, NetProvider> &client)
 	fail_unless(result.header.sync == static_cast<int>(f1));
 	fail_unless(result.header.code == 0);
 
+	TEST_CASE("wait method check future readiness before waiting (gh-133");
+	f = conn.ping();
+	fail_unless(client.wait(conn, f, WAIT_TIMEOUT) == 0);
+	fail_unless(client.wait(conn, f) == 0);
+	conn.getResponse(f);
+	f = conn.ping();
+	fail_unless(client.wait(conn, f, WAIT_TIMEOUT) == 0);
+	fail_unless(client.waitAll(conn, {f}) == 0);
+	conn.getResponse(f);
+	f = conn.ping();
+	fail_unless(client.wait(conn, f, WAIT_TIMEOUT) == 0);
+	/* FIXME(gh-143): test solely that we check future readiness before waiting. */
+	fail_unless(client.waitCount(conn, 0) == 0);
+	conn.getResponse(f);
+	/* FIXME(gh-132): waitAny does not check connections for ready futures. */
+#if 0
+	f = conn.ping();
+	fail_unless(client.wait(conn, f, WAIT_TIMEOUT) == 0);
+	fail_unless(client.waitAny(conn).has_value());
+	conn.getResponse(f);
+#endif
+
 	client.close(conn);
 }