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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
