You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mina.apache.org by "trukhinyuri (via GitHub)" <gi...@apache.org> on 2026/10/14 18:25:38 UTC
[PR] GH-929: Refresh per-session heartbeat properties in ClientConnectionService.startHeartBeat() [mina-sshd]
trukhinyuri opened a new pull request, #930:
URL: https://github.com/apache/mina-sshd/pull/930
Fixes #929
`ClientSessionImpl` initializes its services in the constructor before `SshClient.connect(...)` returns the `ClientSession`. Because `ClientConnectionService` previously read `HEARTBEAT_REQUEST`, `HEARTBEAT_INTERVAL`, and `HEARTBEAT_NO_REPLY_MAX` only in its constructor, per-session heartbeat properties configured on `ClientSession` before `session.auth()` were ignored when `startHeartBeat()` ran after authentication.
- Refresh `heartbeatRequest`, `heartbeatInterval`, and `heartbeatMaxNoReply` in `ClientConnectionService.startHeartBeat()`.
- Add `KeepAliveTest#testPerSessionHeartbeatConfiguration`.
--
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-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
Re: [PR] GH-929: Refresh per-session heartbeat properties in ClientConnectionService.startHeartBeat() [mina-sshd]
Posted by "trukhinyuri (via GitHub)" <gi...@apache.org>.
trukhinyuri commented on code in PR #930:
URL: https://github.com/apache/mina-sshd/pull/930#discussion_r4277750135
##########
sshd-core/src/test/java/org/apache/sshd/KeepAliveTest.java:
##########
@@ -249,6 +249,40 @@ public Result process(ConnectionService connectionService, String request, boole
}
}
+ // see GH-929
+ @Test
+ void testPerSessionHeartbeatConfiguration() throws Exception {
+ CoreModuleProperties.IDLE_TIMEOUT.set(sshd, Duration.ofSeconds(30));
+ List<RequestHandler<ConnectionService>> globalHandlers = sshd.getGlobalRequestHandlers();
+ sshd.setGlobalRequestHandlers(Collections.singletonList(new AbstractConnectionServiceRequestHandler() {
+ @Override
+ public Result process(ConnectionService connectionService, String request, boolean wantReply, Buffer buffer)
+ throws Exception {
+ // Never reply;
+ return Result.Replied;
+ }
Review Comment:
Clarified the comment above `return Result.Replied;` in 97389fd1: returning `Result.Replied` without writing a buffer suppresses `AbstractSession.requestSuccess()` so the client receives no reply and exercises the `heartbeatMaxNoReply` disconnect path.
##########
sshd-core/src/main/java/org/apache/sshd/client/session/ClientConnectionService.java:
##########
@@ -47,9 +47,9 @@
public class ClientConnectionService
extends AbstractConnectionService
implements ClientSessionHolder {
- protected final String heartbeatRequest;
- protected final Duration heartbeatInterval;
- protected final int heartbeatMaxNoReply;
+ protected String heartbeatRequest;
+ protected Duration heartbeatInterval;
+ protected int heartbeatMaxNoReply;
Review Comment:
Declared `heartbeatRequest`, `heartbeatInterval`, and `heartbeatMaxNoReply` as `volatile` in 97389fd1 so updates in `startHeartBeat()` are safely published to the heartbeat scheduler thread.
##########
sshd-core/src/main/java/org/apache/sshd/client/session/ClientConnectionService.java:
##########
@@ -122,6 +122,9 @@ public void start() {
@Override
protected synchronized ScheduledFuture<?> startHeartBeat() {
+ heartbeatRequest = CoreModuleProperties.HEARTBEAT_REQUEST.getRequired(this);
+ heartbeatInterval = CoreModuleProperties.HEARTBEAT_INTERVAL.getRequired(this);
+ heartbeatMaxNoReply = configureMaxNoReply();
Review Comment:
Addressed in 97389fd1 by marking all three heartbeat fields `volatile`.
##########
sshd-core/src/test/java/org/apache/sshd/KeepAliveTest.java:
##########
@@ -249,6 +249,40 @@ public Result process(ConnectionService connectionService, String request, boole
}
}
+ // see GH-929
+ @Test
+ void testPerSessionHeartbeatConfiguration() throws Exception {
+ CoreModuleProperties.IDLE_TIMEOUT.set(sshd, Duration.ofSeconds(30));
+ List<RequestHandler<ConnectionService>> globalHandlers = sshd.getGlobalRequestHandlers();
+ sshd.setGlobalRequestHandlers(Collections.singletonList(new AbstractConnectionServiceRequestHandler() {
+ @Override
+ public Result process(ConnectionService connectionService, String request, boolean wantReply, Buffer buffer)
+ throws Exception {
+ // Never reply;
+ return Result.Replied;
+ }
+ }));
+ Duration heartbeat = Duration.ofMillis(500L);
+ try (ClientSession session = client.connect(getCurrentTestName(), TEST_LOCALHOST, port).verify(CONNECT_TIMEOUT)
+ .getSession()) {
+ CoreModuleProperties.HEARTBEAT_INTERVAL.set(session, heartbeat);
+ CoreModuleProperties.HEARTBEAT_NO_REPLY_MAX.set(session, 1);
+ session.addPasswordIdentity(getCurrentTestName());
+ session.auth().verify(AUTH_TIMEOUT);
+
+ try (ClientChannel channel = session.createChannel(Channel.CHANNEL_SHELL)) {
Review Comment:
Added `channel.open().verify(OPEN_TIMEOUT)` before `channel.waitFor(EnumSet.of(ClientChannelEvent.CLOSED), ...)` in 97389fd1.
--
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-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
Re: [PR] GH-929: Refresh per-session heartbeat properties in ClientConnectionService.startHeartBeat() [mina-sshd]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #930:
URL: https://github.com/apache/mina-sshd/pull/930#discussion_r4274837436
##########
sshd-core/src/test/java/org/apache/sshd/KeepAliveTest.java:
##########
@@ -249,6 +249,40 @@ public Result process(ConnectionService connectionService, String request, boole
}
}
+ // see GH-929
+ @Test
+ void testPerSessionHeartbeatConfiguration() throws Exception {
+ CoreModuleProperties.IDLE_TIMEOUT.set(sshd, Duration.ofSeconds(30));
+ List<RequestHandler<ConnectionService>> globalHandlers = sshd.getGlobalRequestHandlers();
+ sshd.setGlobalRequestHandlers(Collections.singletonList(new AbstractConnectionServiceRequestHandler() {
+ @Override
+ public Result process(ConnectionService connectionService, String request, boolean wantReply, Buffer buffer)
+ throws Exception {
+ // Never reply;
+ return Result.Replied;
+ }
Review Comment:
`Result.Replied` indicates the request has been replied to, but this handler does not actually send any reply. This contradicts the intent (“Never reply”) and can invalidate the test by not truly exercising the “no reply” heartbeat path. Return the appropriate status for “handled but no reply sent” (or explicitly send no response when `wantReply` is true), consistent with the request-handler contract in this codebase.
##########
sshd-core/src/main/java/org/apache/sshd/client/session/ClientConnectionService.java:
##########
@@ -47,9 +47,9 @@
public class ClientConnectionService
extends AbstractConnectionService
implements ClientSessionHolder {
- protected final String heartbeatRequest;
- protected final Duration heartbeatInterval;
- protected final int heartbeatMaxNoReply;
+ protected String heartbeatRequest;
+ protected Duration heartbeatInterval;
+ protected int heartbeatMaxNoReply;
Review Comment:
These fields were changed from `final` to mutable and are written in `startHeartBeat()`, but may be read from other threads (e.g., scheduled heartbeat task) without a guaranteed happens-before relationship. This can cause stale reads or inconsistent combinations of values. Consider making them `volatile`, or ensuring all reads/writes occur under the same synchronization, or copying refreshed values into local finals that are captured/used by the scheduled runnable.
##########
sshd-core/src/test/java/org/apache/sshd/KeepAliveTest.java:
##########
@@ -249,6 +249,40 @@ public Result process(ConnectionService connectionService, String request, boole
}
}
+ // see GH-929
+ @Test
+ void testPerSessionHeartbeatConfiguration() throws Exception {
+ CoreModuleProperties.IDLE_TIMEOUT.set(sshd, Duration.ofSeconds(30));
+ List<RequestHandler<ConnectionService>> globalHandlers = sshd.getGlobalRequestHandlers();
+ sshd.setGlobalRequestHandlers(Collections.singletonList(new AbstractConnectionServiceRequestHandler() {
+ @Override
+ public Result process(ConnectionService connectionService, String request, boolean wantReply, Buffer buffer)
+ throws Exception {
+ // Never reply;
+ return Result.Replied;
+ }
+ }));
+ Duration heartbeat = Duration.ofMillis(500L);
+ try (ClientSession session = client.connect(getCurrentTestName(), TEST_LOCALHOST, port).verify(CONNECT_TIMEOUT)
+ .getSession()) {
+ CoreModuleProperties.HEARTBEAT_INTERVAL.set(session, heartbeat);
+ CoreModuleProperties.HEARTBEAT_NO_REPLY_MAX.set(session, 1);
+ session.addPasswordIdentity(getCurrentTestName());
+ session.auth().verify(AUTH_TIMEOUT);
+
+ try (ClientChannel channel = session.createChannel(Channel.CHANNEL_SHELL)) {
Review Comment:
The channel is created but never opened. Waiting for `CLOSED` on a channel that was never opened can lead to false positives/negatives (depending on initial channel state and implementation details) and makes the timing assertion less meaningful. Open the channel (e.g., `channel.open().verify(...)`) before waiting for closure so the test reliably validates heartbeat-driven disconnect behavior.
##########
sshd-core/src/main/java/org/apache/sshd/client/session/ClientConnectionService.java:
##########
@@ -122,6 +122,9 @@ public void start() {
@Override
protected synchronized ScheduledFuture<?> startHeartBeat() {
+ heartbeatRequest = CoreModuleProperties.HEARTBEAT_REQUEST.getRequired(this);
+ heartbeatInterval = CoreModuleProperties.HEARTBEAT_INTERVAL.getRequired(this);
+ heartbeatMaxNoReply = configureMaxNoReply();
Review Comment:
These fields were changed from `final` to mutable and are written in `startHeartBeat()`, but may be read from other threads (e.g., scheduled heartbeat task) without a guaranteed happens-before relationship. This can cause stale reads or inconsistent combinations of values. Consider making them `volatile`, or ensuring all reads/writes occur under the same synchronization, or copying refreshed values into local finals that are captured/used by the scheduled runnable.
--
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-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
Re: [PR] GH-929: Refresh per-session heartbeat properties in ClientConnectionService.startHeartBeat() [mina-sshd]
Posted by "tomaswolf (via GitHub) via dev" <de...@mina.apache.org>.
tomaswolf commented on PR #930:
URL: https://github.com/apache/mina-sshd/pull/930#issuecomment-6535068744
Thank you!
--
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-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
Re: [PR] GH-929: Refresh per-session heartbeat properties in ClientConnectionService.startHeartBeat() [mina-sshd]
Posted by "tomaswolf (via GitHub) via dev" <de...@mina.apache.org>.
tomaswolf merged PR #930:
URL: https://github.com/apache/mina-sshd/pull/930
--
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-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
Re: [PR] GH-929: Refresh per-session heartbeat properties in ClientConnectionService.startHeartBeat() [mina-sshd]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #930:
URL: https://github.com/apache/mina-sshd/pull/930#discussion_r4277795612
##########
sshd-core/src/main/java/org/apache/sshd/client/session/ClientConnectionService.java:
##########
@@ -122,6 +122,9 @@ public void start() {
@Override
protected synchronized ScheduledFuture<?> startHeartBeat() {
+ heartbeatRequest = CoreModuleProperties.HEARTBEAT_REQUEST.getRequired(this);
Review Comment:
Optional: extend `testPerSessionHeartbeatConfiguration` to set a distinct per-session `HEARTBEAT_REQUEST` before authentication and assert that the server receives that request name. The test currently sets only the interval and no-reply limit, and its handler ignores the request name, so it would still pass if this refresh were removed.
--
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-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org
Re: [PR] GH-929: Refresh per-session heartbeat properties in ClientConnectionService.startHeartBeat() [mina-sshd]
Posted by "trukhinyuri (via GitHub)" <gi...@apache.org>.
trukhinyuri commented on code in PR #930:
URL: https://github.com/apache/mina-sshd/pull/930#discussion_r4278089053
##########
sshd-core/src/main/java/org/apache/sshd/client/session/ClientConnectionService.java:
##########
@@ -122,6 +122,9 @@ public void start() {
@Override
protected synchronized ScheduledFuture<?> startHeartBeat() {
+ heartbeatRequest = CoreModuleProperties.HEARTBEAT_REQUEST.getRequired(this);
Review Comment:
Extended `testPerSessionHeartbeatConfiguration` in 12e9fe5 to override `CoreModuleProperties.HEARTBEAT_REQUEST` on the session before authentication and assert that the server receives that custom request name.
--
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-unsubscribe@mina.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@mina.apache.org
For additional commands, e-mail: dev-help@mina.apache.org