client: fix `result` argument of `wait` #131

drewdzzz · 2025-04-23T14:40:45Z

The argument was completely broken - the commit fixes it.
Along the way, fixed a crash when a decoded future appeared in m_ReadyToDecode. See the commits for details.

Closes #112
Closes #124

Connector uses a special helper for decoding ready responses and then it erases them from `m_ReadyToDecode` set by himself. That's not a good design and it can lead to various bugs (there is already a bug, actually) because it's easy to forgot to earse the decoded future. So let's move the erasure right to the `connectionDecodeResponses` helper. Note that the helper must be turned into a method of `Connector` then since it needs to use its fields. The above-mentioned bug happens because we decode responses in `Connector::wait` right before the main loop but we forgot to remove the connection from `m_ReadyToDecode` set there. Closes tarantool#124

Method `wait` of `Connector` allows to obtain result directly with `result` argument - it can be useful when the user cares about performance and don't want to perform an unneeded insertion to a map of ready futures. However, we check that the future is ready by looking to the map of ready futures, and the future is not inserted there when the argument is used. The commit fixes the problem by checking the argument as well. Also, the commit handles a situation when user waits for already decoded future using `result` argument. Before the commit, `wait` would return zero return code (success) but the `result` wouldn't be set - user still had to check if it's not in the map of futures. After the commit, already decoded future is moved to `result` as well, so if the function returns successfully, `result` is guaranteed to be set. Part of tarantool#112

Somewhy we cast `nullptr` to the needed class sometimes and it looks ugly. Anyway, `nullptr` is designed to be casted to any pointer automatically, so let's just stop doing it.

Currently, the argument returns any decoded future - that is inconvenient and completely unusable. Let's return only the requested future, or nothing, if it's not ready. Along the way, reformat argument list of modified functions to make them conform clang-format. Closes tarantool#112

drewdzzz · 2025-04-24T08:55:37Z

Ignoring clang-format suggestions in order not to mess with the diff.
(I hope we'll do something about it sooner or later...)

Suggestions

diff --git a/src/Client/Connection.hpp b/src/Client/Connection.hpp
index d9626f0..30547e1 100644
--- a/src/Client/Connection.hpp
+++ b/src/Client/Connection.hpp
@@ -228,9 +228,8 @@ public:
 	friend
 	bool hasDataToDecode(Connection<B, N> &conn);
 
-	template<class B, class N>
-	friend
-	enum DecodeStatus processResponse(Connection<B, N> &conn, int req_sync, Response<B> *result);
+	template <class B, class N>
+	friend enum DecodeStatus processResponse(Connection<B, N> &conn, int req_sync, Response<B> *result);
 
 	template<class B, class N>
 	friend
@@ -527,7 +526,7 @@ inputBufGC(Connection<BUFFER, NetProvider> &conn)
 	}
 }
 
-template<class BUFFER, class NetProvider>
+template <class BUFFER, class NetProvider>
 DecodeStatus
 processResponse(Connection<BUFFER, NetProvider> &conn, int req_sync, Response<BUFFER> *result)
 {
diff --git a/src/Client/Connector.hpp b/src/Client/Connector.hpp
index 863bd28..c0db86e 100644
--- a/src/Client/Connector.hpp
+++ b/src/Client/Connector.hpp
@@ -171,7 +171,7 @@ Connector<BUFFER, NetProvider>::close(ConnectionImpl<BUFFER, NetProvider> &conn)
 	m_NetProvider.close(conn.strm);
 }
 
-template<class BUFFER, class NetProvider>
+template <class BUFFER, class NetProvider>
 int
 Connector<BUFFER, NetProvider>::connectionDecodeResponses(Connection<BUFFER, NetProvider> &conn, int req_sync,
 							  Response<BUFFER> *result)

mkostoevr · 2025-04-24T12:02:00Z

src/Client/Connector.hpp

Typo in commit comment: "the helper must be turned into a method of `Connector` then since it".

Nit: please reread your commit message. In this patch you are erasing decoded connections, not futures or responses.

CuriousGeorgiy · 2025-04-25T09:40:13Z

src/Client/Connector.hpp

-		DecodeStatus rc = processResponse(conn,  result);
-		if (rc == DECODE_ERR)
-			return -1;
+		DecodeStatus status = processResponse(conn,  result);


Nit: excess whitespace after processResponse(conn,.

CuriousGeorgiy · 2025-04-25T09:47:45Z

src/Client/Connector.hpp

 	if (connectionDecodeResponses(conn, result) != 0)
 		return -1;
+	if (result != NULL && result->header.sync != INVALID_SYNC) {
+		LOG_DEBUG("Future ", future, " is ready and decoded");


Can we assert that result->header.sync == future here and below? I think we should refactor this branch into a separate helper since it repeats more than once.

CuriousGeorgiy · 2025-04-25T09:59:10Z

test/ClientTest.cpp

+	fail_unless(conn.futureIsReady(f));
+	fail_unless(client.wait(conn, f, WAIT_TIMEOUT, &result) == 0);
+	/* The result was consumed, so the future is not ready. */
+	fail_unless(!conn.futureIsReady(f));


I think the future semantics are kind of weird right now. If a user tries to wait on this future, he will never get it, right?

Could we track the current sync in the connection and return an error in wait methods for this? And add something like a valid method or return 3 possible values from futureIsReady (false, true, invalid)? Just open a ticket if you agree.

CuriousGeorgiy · 2025-04-25T10:09:49Z

src/Client/Connector.hpp

@@ -213,9 +214,10 @@ Connector<BUFFER, NetProvider>::wait(Connection<BUFFER, NetProvider> &conn,
 	Timer timer{timeout};
 	timer.start();
 	static constexpr int INVALID_SYNC = -1;
+	int req_sync = static_cast<int>(future);


Why don't you just use the rid_t type everywhere? Would be nice to use integer types consistently across the code base. If you think it's more convenient to pass around an int right now, but you agree that this looks inconsistent to maintain, please open a ticket.

CuriousGeorgiy · 2025-04-25T10:11:36Z

src/Client/Connector.hpp

@@ -266,7 +268,7 @@ Connector<BUFFER, NetProvider>::waitAll(Connection<BUFFER, NetProvider> &conn,
 				      strerror(errno), errno);
 			return -1;
 		}
-		if (connectionDecodeResponses(conn, nullptr) != 0)
+		if (connectionDecodeResponses(conn, 0, nullptr) != 0)


Nit: isn't it better to pass the INVALID_SYNC constant you introduced previously to be consistent?

I would also introduce an overload for connectionDecodeResponses that accepts only a connection, since we are in C++ world. But feel free to ignore/defer.

CuriousGeorgiy · 2025-04-25T10:18:43Z

src/Client/Connector.hpp

Nit: please reread your commit message. In this patch you are erasing decoded connections, not futures or responses.

drewdzzz force-pushed the wait_with_response branch 4 times, most recently from d7df387 to abbe0c2 Compare April 24, 2025 08:39

drewdzzz changed the title ~~Wait with response~~ client: fix result argument Apr 24, 2025

drewdzzz changed the title ~~client: fix result argument~~ client: fix result argument of wait Apr 24, 2025

drewdzzz force-pushed the wait_with_response branch 2 times, most recently from 3c7f774 to 2d9d6cc Compare April 24, 2025 08:49

drewdzzz added 4 commits April 24, 2025 11:50

client: do not cast nullptr

aeb43ee

Somewhy we cast `nullptr` to the needed class sometimes and it looks ugly. Anyway, `nullptr` is designed to be casted to any pointer automatically, so let's just stop doing it.

drewdzzz force-pushed the wait_with_response branch from 2d9d6cc to 995d5ec Compare April 24, 2025 08:51

drewdzzz marked this pull request as ready for review April 24, 2025 11:05

drewdzzz requested review from CuriousGeorgiy and mkostoevr April 24, 2025 11:05

drewdzzz assigned CuriousGeorgiy and mkostoevr Apr 24, 2025

mkostoevr approved these changes Apr 24, 2025

View reviewed changes

CuriousGeorgiy reviewed Apr 25, 2025

View reviewed changes

CuriousGeorgiy removed their assignment Apr 25, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

client: fix `result` argument of `wait` #131

client: fix `result` argument of `wait` #131

drewdzzz commented Apr 23, 2025 •

edited

Loading

drewdzzz commented Apr 24, 2025

mkostoevr Apr 24, 2025

CuriousGeorgiy Apr 25, 2025

CuriousGeorgiy Apr 25, 2025

CuriousGeorgiy Apr 25, 2025

CuriousGeorgiy Apr 25, 2025 •

edited

Loading

CuriousGeorgiy Apr 25, 2025

CuriousGeorgiy Apr 25, 2025

CuriousGeorgiy Apr 25, 2025

client: fix result argument of wait #131

Are you sure you want to change the base?

client: fix result argument of wait #131

Conversation

drewdzzz commented Apr 23, 2025 • edited Loading

drewdzzz commented Apr 24, 2025

mkostoevr Apr 24, 2025

Choose a reason for hiding this comment

CuriousGeorgiy Apr 25, 2025

Choose a reason for hiding this comment

CuriousGeorgiy Apr 25, 2025

Choose a reason for hiding this comment

CuriousGeorgiy Apr 25, 2025

Choose a reason for hiding this comment

CuriousGeorgiy Apr 25, 2025 • edited Loading

Choose a reason for hiding this comment

CuriousGeorgiy Apr 25, 2025

Choose a reason for hiding this comment

CuriousGeorgiy Apr 25, 2025

Choose a reason for hiding this comment

CuriousGeorgiy Apr 25, 2025

Choose a reason for hiding this comment

client: fix `result` argument of `wait` #131

client: fix `result` argument of `wait` #131

drewdzzz commented Apr 23, 2025 •

edited

Loading

CuriousGeorgiy Apr 25, 2025 •

edited

Loading