mroeppis commented on code in PR #13791:
URL: https://github.com/apache/ignite/pull/13791#discussion_r4504668170
##########
modules/core/src/main/java/org/apache/ignite/spi/communication/tcp/internal/GridNioServerWrapper.java:
##########
@@ -489,50 +484,82 @@ public GridNioSession createNioSession(ClusterNode node,
int connIdx) throws Ign
throw new IgniteCheckedException("Local node has
not been started or " +
"fully initialized [isStopping=" +
stateProvider.isStopping() + ']');
+ // The handshake window, re-verified against the
strategy's total timeout (the call
+ // does not advance the backoff state, so the
per-attempt backoff progression is
+ // unchanged).
timeout = connTimeoutStgy.nextTimeout(timeout);
- rcvCnt = safeTcpHandshake(ch,
- node.id(),
- timeout,
- sslMeta,
- new HandshakeMessage(locNode.id(),
- recoveryDesc.incrementConnectCount(),
- recoveryDesc.received(),
- connIdx));
-
- if (rcvCnt == ALREADY_CONNECTED)
- return null;
- else if (rcvCnt == NODE_STOPPING) {
- // Safe to remap on remote node stopping.
- throw new ClusterTopologyCheckedException("Remote
node started stop procedure: " + node.id());
+ long handshakeTimeout = timeout;
+
+ // The deadline covers the handshake and the NIO
session registration: when it
+ // elapses, the channel is closed, which aborts the
in-progress handshake and makes
+ // the registration fail with a ClosedChannelException
- the latter only once the
+ // responsible NIO worker processes the queued
registration operation, so the
+ // registration wait is bounded only under that
precondition. It is armed after the
+ // socket connect, which is bounded by its own connect
timeout, so the connect does
+ // not consume the deadline and the handshake keeps
the full window the strategy
+ // allocated to it.
+ HandshakeTimeoutObject timeoutObj = new
HandshakeTimeoutObject(ch);
+
+ handshakeTimeoutExecutorService.schedule(timeoutObj,
handshakeTimeout, TimeUnit.MILLISECONDS);
+
+ boolean deadlineCancelled = false;
+
+ try {
+ rcvCnt = safeTcpHandshake(ch,
+ node.id(),
+ sslMeta,
+ new HandshakeMessage(locNode.id(),
+ recoveryDesc.incrementConnectCount(),
+ recoveryDesc.received(),
+ connIdx));
+
+ if (rcvCnt == ALREADY_CONNECTED)
+ return null;
+ else if (rcvCnt == NODE_STOPPING) {
+ // Safe to remap on remote node stopping.
+ throw new
ClusterTopologyCheckedException("Remote node started stop procedure: " +
node.id());
+ }
+ else if (rcvCnt == UNKNOWN_NODE)
+ throw new IgniteCheckedException("Remote node
does not observe current node " +
+ "in topology : " + node.id());
+ else if (rcvCnt == NEED_WAIT) {
+ // Should wait for discovery lag without
applying connTimeoutStgy, otherwise cache operations
+ // may hang on waiting inexistent topology,
see IgniteClientConnectTest for test
+ // scenarios with delayed client node join.
+ if (log.isDebugEnabled())
+ log.debug("NEED_WAIT received, handshake
after delay [node = "
+ + node + ", outOfTopologyDelay = " +
DFLT_NEED_WAIT_DELAY + "ms]");
+
+ // The peer-requested discovery lag delay is
not a step of the connection
+ // establishment: end the deadline before the
wait, the next attempt arms a
+ // fresh one.
+ deadlineCancelled = timeoutObj.cancel();
+
+ U.sleep(DFLT_NEED_WAIT_DELAY);
+
+ continue;
+ }
+ else if (rcvCnt < 0)
+ throw new IgniteCheckedException("Unsupported
negative receivedCount [rcvCnt=" + rcvCnt +
+ ", senderNode=" + node + ']');
+
+ recoveryDesc.onHandshake(rcvCnt);
+
+ meta.put(CONSISTENT_ID_META, node.consistentId());
+ meta.put(CONN_IDX_META, connKey);
+ meta.put(GridNioServer.RECOVERY_DESC_META_KEY,
recoveryDesc);
+ meta.put(MSG_SER_CTX.ordinal(),
buildForPeers(stateProvider.ignite(), node));
+
+ ses = nioSrv.createSession(ch, meta, false,
null).get();
Review Comment:
The call to `get()` has no timeout.
The connection establishment process already has a deadline, and a scheduled
timeout task closes the underlying channel when that deadline expires.
However, closing the channel does not guarantee that the future returned by
`createSession()` completes immediately.
Session registration is processed by a NIO worker. If that worker is blocked
or otherwise unable to process its queued registration operation, the following
can happen:
1. The connection thread submits the session registration.
2. The NIO worker does not process the registration because it is stalled.
3. The connection establishment deadline expires.
4. The timeout task closes the channel.
5. The connection thread remains blocked in `sesFut.get()` because the
registration future has not completed.
The timeout has closed the socket, but it has not necessarily released the
thread waiting for the registration result.
This distinction is essential: closing a channel and completing an
asynchronous operation are not the same thing.
My original implementation therefore does not actually guarantee that
connection establishment would terminate within its configured deadline.
**Why the finding is valid**: An unbounded wait on a future controlled by
another worker defeats the connection deadline. The waiting thread needs its
own bounded wait, based on the remaining deadline.
My revised implementation does exactly that and also closes a session if
registration completes after the timeout.
--
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]
