mroeppis commented on code in PR #13791:
URL: https://github.com/apache/ignite/pull/13791#discussion_r4504684254
##########
modules/core/src/main/java/org/apache/ignite/spi/communication/tcp/internal/ConnectionClientPool.java:
##########
@@ -355,33 +401,67 @@ public GridCommunicationClient reserveClient(ClusterNode
node, int connIdx) thro
else
fut = oldFut;
- long clientReserveWaitTimeout = registry != null ?
registry.getSystemWorkerBlockedTimeout() / 3
- : cfg.connectionTimeout() / 3;
+ if (ownClient != null)
+ client = ownClient;
+
+ if (client == null) {
+ long perWaitTimeout = registry != null ?
+ registry.getSystemWorkerBlockedTimeout() / 3 :
cfg.connectionTimeout() / 3;
- long currTimeout = System.currentTimeMillis();
+ long currTimeout = System.currentTimeMillis();
- // This cycle will eventually quit when future is
completed by concurrent thread reserving client.
- while (true) {
- try {
- client = fut.get(clientReserveWaitTimeout,
TimeUnit.MILLISECONDS);
+ // This cycle will eventually quit when future is
completed by concurrent thread reserving client.
+ while (true) {
+ // A concurrent thread may have established the
client while this thread was
+ // waiting: return it instead of waiting further
or giving up, even if the
+ // reserve budget has already elapsed — the budget
bounds waiting for a
+ // connection, not the availability of an existing
client.
+ GridCommunicationClient[] curClients0 =
clients.get(nodeId);
- break;
- }
- catch (IgniteFutureTimeoutCheckedException ignored) {
- currTimeout += clientReserveWaitTimeout;
-
- if (log.isDebugEnabled()) {
- log.debug(
- "Still waiting for reestablishing
connection to node " +
- "[nodeId=" + node.id() + ",
waitingTime=" + currTimeout + "ms]"
- );
+ GridCommunicationClient curClient = curClients0 !=
null && connIdx < curClients0.length ?
+ curClients0[connIdx] : null;
+
+ if (curClient != null) {
+ client = curClient;
+
+ break;
+ }
+
+ // Recompute the remaining budget on every
iteration and cap the individual
+ // wait by it, so that the total reserve budget is
enforced even when a single
+ // wait would overshoot it. When the per-wait
timeout is not configured (0),
+ // e.g. because the system worker blocked timeout
is set to a non-positive
+ // value, wait the full remaining budget instead
of aborting before it has
+ // elapsed.
+ long remainingReserveBudget = totalReserveTimeout
- reserveElapsedMillis(reserveStartNanos);
+
+ long clientReserveWaitTimeout = perWaitTimeout > 0
?
+ Math.min(perWaitTimeout,
remainingReserveBudget) : remainingReserveBudget;
+
+ if (clientReserveWaitTimeout <= 0)
+ throw reserveTimeoutException(node,
totalReserveTimeout);
Review Comment:
My original reservation logic has a total time budget. Threads waiting for
another thread to establish a connection are supposed to stop waiting once that
budget expires.
However, the timeout handling could report a generic reservation timeout
even when the shared connection future has already completed with a more useful
error.
Consider this sequence:
1. Thread A starts establishing a connection.
2. The connection attempt fails.
3. The shared future is completed exceptionally with the actual connection
error.
4. The reservation budget expires at approximately the same time.
5. Thread A reaches the timeout check before retrieving the completed
future's result.
6. The caller receives a generic reservation-timeout exception instead of
the original connection error.
The distinction matters operationally.
A reservation timeout says: "The caller waited too long for another thread
to establish a connection."
The actual connection error might say: "The connection to the remote node
failed."
Those are different failure conditions. The latter provides the information
needed to investigate the underlying problem.
**Why the finding is valid**: If the future is already completed when the
reservation budget expires, its result should take precedence over the generic
timeout. Otherwise, the implementation discards useful diagnostic information.
My revised implementation checks `fut.isDone()` before throwing the
reservation-timeout exception and retrieves the completed future's result.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
