tomaswolf commented on code in PR #507: URL: https://github.com/apache/mina-sshd/pull/507#discussion_r1618319934
########## CHANGES.md: ########## @@ -58,7 +59,39 @@ NTRU Prime sntrup761 and X25519 with SHA-512: sntrup761x25519-sha512](https://ww ## Behavioral changes and enhancements -* [GH-468](https://github.com/apache/mina-sshd/issues/468) SFTP: validate length of data received: must not be more than requested +### [GH-461](https://github.com/apache/mina-sshd/issues/461) Fix heartbeats with `wantReply=true` + +The client-side heartbeat mechanism has been updated. Such heartbeats are configured via the +`CoreModuleProperties.HEARTBEAT_INTERVAL` property. If this interval is > 0, heartbeats are sent to +the server. + +Previously these heartbeats could also be configured with a `CoreModuleProperties.HEARTBEAT_REPLY_WAIT` +timeout. If the timeout was <= 0, the client would just send heartbeat requests without expecting any +answers. If the timeout was > 0, the client would send requests with a flag indicating that the server +should reply. The client would then wait for the specified duration for the reply and would terminate +the connection if none was received. + +This old mechanism could cause trouble if the timeout was fairly long and the server was slow to respond. Review Comment: Done. ########## sshd-core/src/main/java/org/apache/sshd/client/session/ClientConnectionService.java: ########## @@ -55,7 +61,41 @@ public ClientConnectionService(AbstractClientSession s) throws SshException { heartbeatRequest = CoreModuleProperties.HEARTBEAT_REQUEST.getRequired(this); heartbeatInterval = CoreModuleProperties.HEARTBEAT_INTERVAL.getRequired(this); - heartbeatReplyMaxWait = CoreModuleProperties.HEARTBEAT_REPLY_WAIT.getRequired(this); + heartbeatMaxNoReply = configureMaxNoReply(); + } + + protected int configureMaxNoReply() { + @SuppressWarnings("deprecation") + Duration timeout = CoreModuleProperties.HEARTBEAT_REPLY_WAIT.getOrNull(this); + if (timeout == null || GenericUtils.isNegativeOrNull(heartbeatInterval) || GenericUtils.isEmpty(heartbeatRequest)) { + return CoreModuleProperties.HEARTBEAT_NO_REPLY_MAX.getRequired(this).intValue(); + } + // The deprecated timeout is configured explicitly. If the new no-reply-max is _not_ explicitly configured, + // set it from the timeout. + Integer noReplyValue = CoreModuleProperties.HEARTBEAT_NO_REPLY_MAX.getOrNull(this); + if (noReplyValue != null) { + return noReplyValue.intValue(); + } + if (timeout.compareTo(heartbeatInterval) >= 0) { + // Timeout is longer than the interval. With the previous system, that would have killed the session when + // the Review Comment: Done. -- 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: dev-unsubscr...@mina.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@mina.apache.org For additional commands, e-mail: dev-h...@mina.apache.org
