gnodet commented on code in PR #507:
URL: https://github.com/apache/mina-sshd/pull/507#discussion_r1617742237
##########
sshd-core/src/main/java/org/apache/sshd/client/session/ClientConnectionService.java:
##########
@@ -55,7 +61,40 @@ 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 old system, that
would have killed the session when the
+ // timeout was reached. A slow server that managed to return the
reply just before the timeout expired would
+ // have delayed subsequent heartbeats. The new system will keep
sending heartbeats with the given interval.
+ // Thus we can have timeout / interval heartbeats without reply if
we want to approximate the old system.
+ double timeoutSec = timeout.getSeconds() + (timeout.getNano() /
1_000_000_000.0);
+ double intervalSec = heartbeatInterval.getSeconds() +
(heartbeatInterval.getNano() / 1_000_000_000.0);
+ double multiple = timeoutSec / intervalSec;
+ if (multiple >= Integer.MAX_VALUE - 1) {
+ return Integer.MAX_VALUE;
+ } else {
+ return (int) multiple + 1;
+ }
+ }
+ // Timeout is smaller than the interval. We want to have every
heartbeat replied to.
+ return 1;
+ // This is an approximation. If no reply is forthcoming, the session
will newly be killed after the interval. In
Review Comment:
newly -> immediately ?
##########
sshd-core/src/main/java/org/apache/sshd/client/session/ClientConnectionService.java:
##########
@@ -55,7 +61,40 @@ 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,
Review Comment:
"no reply max" -> "no-reply-max" so that it's easier to see we're referring
to the value
##########
sshd-core/src/main/java/org/apache/sshd/core/CoreModuleProperties.java:
##########
@@ -162,10 +162,21 @@ public final class CoreModuleProperties {
/**
* Key used to indicate that the heartbeat request is also expecting a
reply - time in <U>milliseconds</U> to wait
* for the reply. If non-positive then no reply is expected (nor
requested).
+ *
+ * @deprecated since 2.13.0, use {@link #HEARTBEAT_NO_REPLY_MAX} instead
*/
+ @Deprecated
public static final Property<Duration> HEARTBEAT_REPLY_WAIT
= Property.durationSec("heartbeat-reply-wait",
Duration.ofMinutes(5));
+ /**
+ * Key to set the maximum number of heartbeat messages to send without
having received a reply. If > 0, heartbeat
+ * messages are sent with a flag that requires the peer to reply. The
session will be killed if
+ * {@code HEARTBEAT_NO_REPLY_MAX} heartbeats have been sent without having
received a reply. If <= 0, heartbeat
+ * messages will be sent, but no reply is requested or expected, and the
client will not kill the session.
+ */
Review Comment:
`@since 2.13.0`
##########
sshd-core/src/main/java/org/apache/sshd/core/CoreModuleProperties.java:
##########
@@ -162,10 +162,21 @@ public final class CoreModuleProperties {
/**
* Key used to indicate that the heartbeat request is also expecting a
reply - time in <U>milliseconds</U> to wait
* for the reply. If non-positive then no reply is expected (nor
requested).
+ *
+ * @deprecated since 2.13.0, use {@link #HEARTBEAT_NO_REPLY_MAX} instead
*/
+ @Deprecated
Review Comment:
`@Deprecated(since = "2.13.0")` ?
##########
sshd-core/src/main/java/org/apache/sshd/client/session/ClientConnectionService.java:
##########
@@ -55,7 +61,40 @@ 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 old system, that
would have killed the session when the
Review Comment:
old -> previous
##########
sshd-core/src/main/java/org/apache/sshd/client/session/ClientConnectionService.java:
##########
@@ -55,7 +61,40 @@ 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 old system, that
would have killed the session when the
+ // timeout was reached. A slow server that managed to return the
reply just before the timeout expired would
+ // have delayed subsequent heartbeats. The new system will keep
sending heartbeats with the given interval.
+ // Thus we can have timeout / interval heartbeats without reply if
we want to approximate the old system.
+ double timeoutSec = timeout.getSeconds() + (timeout.getNano() /
1_000_000_000.0);
+ double intervalSec = heartbeatInterval.getSeconds() +
(heartbeatInterval.getNano() / 1_000_000_000.0);
+ double multiple = timeoutSec / intervalSec;
+ if (multiple >= Integer.MAX_VALUE - 1) {
+ return Integer.MAX_VALUE;
+ } else {
+ return (int) multiple + 1;
+ }
+ }
+ // Timeout is smaller than the interval. We want to have every
heartbeat replied to.
+ return 1;
+ // This is an approximation. If no reply is forthcoming, the session
will newly be killed after the interval. In
+ // the old system it would have been killed after the timeout. We
_could_ code something to schedule a task that
Review Comment:
old -> previous
missing comma after "the previous system"
##########
CHANGES.md:
##########
@@ -58,7 +59,34 @@ 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 changed. Such heartbeats are
configured with an interval
+(`CoreModuleProperties.HEARTBEAT_INTERVAL`) > 0. Previously they could also be
configured with a timeout
+(`CoreModuleProperties.HEARTBEAT_REPLY_WAIT`). If the timeout was <= 0, the
client would just send a
+heartbeat request, but not expect any answer. However, with a timeout > 0, it
would send the request
+with a flag telling the server to send back a reply, and then wait for the
given duration for the
+reply to arrive, and terminate the connection if no reply was forthcoming.
+
+That could cause trouble if the timeout was fairly long, and the server was
slow to respond. If the
+timeout was longer than the interval it could also delay subsequent heartbeats.
+
+Newly, `CoreModuleProperties.HEARTBEAT_REPLY_WAIT` is _deprecated_. There is a
new configuration property
+`CoreModuleProperties.HEARTBEAT_NO_REPLY_MAX` instead. It defines a limit for
how many heartbeats without
+reply a session survives. If the value is <= 0, the client still sends
heartbeats without expecting
+any answer. If > 0, it will request a reply from the server for each heartbeat
message, and it will
+terminate the connection once there are
`CoreModuleProperties.HEARTBEAT_NO_REPLY_MAX` previous heartbeats
+for which no reply was received yet.
+
+This new way to configure heartbeats corresponds to the configuration in
OpenSSH via the
+`ServerAliveInterval` and `ServerAliveCountMax` options.
+
+For compatibility with old configurations that do define
`CoreModuleProperties.HEARTBEAT_REPLY_WAIT` > 0,
+the new code maps this to the new configuration (but only if
`CoreModuleProperties.HEARTBEAT_INTERVAL` > 0
+and the new property `CoreModuleProperties.HEARTBEAT_NO_REPLY_MAX` has _not_
been set) by setting
+`CoreModuleProperties.HEARTBEAT_NO_REPLY_MAX =
(CoreModuleProperties.HEARTBEAT_REPLY_WAIT /
CoreModuleProperties.HEARTBEAT_INTERVAL) + 1`.
Review Comment:
With AI help, I have the following reworded doc
The client-side heartbeat mechanism has been updated.
Heartbeats are primarily configured using the
CoreModuleProperties.HEARTBEAT_INTERVAL interval. If this value is > 0,
heartbeats will be sent to the server.
Previously, heartbeats could also be configured with the
CoreModuleProperties.HEARTBEAT_REPLY_WAIT timeout. If the timeout was <= 0, the
client would send a heartbeat request without expecting a reply. If the timeout
was > 0, the client would send the request with a flag indicating that the
server should reply. The client would then wait for the specified duration for
the reply and terminate the connection if no reply was received.
This old mechanism could cause issues if the timeout was long and the server
was slow to respond. A timeout longer than the interval could also delay
subsequent heartbeats.
The CoreModuleProperties.HEARTBEAT_REPLY_WAIT property is now deprecated.
It has been replaced by a new configuration property,
CoreModuleProperties.HEARTBEAT_NO_REPLY_MAX. This property defines the limit
for the number of heartbeats sent without receiving a reply before a session is
terminated. If the value is <= 0, the client sends heartbeats without expecting
any reply. If the value is > 0, the client requests a reply from the server for
each heartbeat message and terminates the connection if the number of
unanswered heartbeats reaches CoreModuleProperties.HEARTBEAT_NO_REPLY_MAX.
This new configuration method aligns with the OpenSSH configuration options
ServerAliveInterval and ServerAliveCountMax.
For compatibility with older configurations where
CoreModuleProperties.HEARTBEAT_REPLY_WAIT > 0, the new code maps this to the
new configuration (only if CoreModuleProperties.HEARTBEAT_INTERVAL > 0 and
CoreModuleProperties.HEARTBEAT_NO_REPLY_MAX is not set) by setting
CoreModuleProperties.HEARTBEAT_NO_REPLY_MAX to
(CoreModuleProperties.HEARTBEAT_REPLY_WAIT /
CoreModuleProperties.HEARTBEAT_INTERVAL) + 1.
--
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
