PROTON-2804 Send half the idle timeout value in ConnectionOptions

The ConnectionOptions API docs state the idle timeout value will be halved when sent in the open frame but this is not being done currently. Fix and test this to ensure it is handled and check the range as the value is a UInt ranged setting.
apache · Mar 14, 2024 · 3593a3f · 3593a3f
1 parent 226dede
commit 3593a3f
Show file tree

Hide file tree

Showing 4 changed files with 50 additions and 2 deletions.
diff --git a/protonj2-client/src/main/java/org/apache/qpid/protonj2/client/ConnectionOptions.java b/protonj2-client/src/main/java/org/apache/qpid/protonj2/client/ConnectionOptions.java
@@ -26,6 +26,7 @@
 
 import org.apache.qpid.protonj2.client.exceptions.ClientOperationTimedOutException;
 import org.apache.qpid.protonj2.client.exceptions.ClientSendTimedOutException;
+import org.apache.qpid.protonj2.types.UnsignedInteger;
 import org.apache.qpid.protonj2.types.transport.Open;
 
 /**
@@ -388,7 +389,7 @@ public ConnectionOptions idleTimeout(long idleTimeout) {
      * @return this {@link ConnectionOptions} instance.
      */
     public ConnectionOptions idleTimeout(long timeout, TimeUnit units) {
-        this.idleTimeout = units.toMillis(timeout);
+        this.idleTimeout = UnsignedInteger.valueOf(units.toMillis(timeout)).longValue();;
         return this;
     }
 

diff --git a/protonj2-client/src/main/java/org/apache/qpid/protonj2/client/impl/ClientConnection.java b/protonj2-client/src/main/java/org/apache/qpid/protonj2/client/impl/ClientConnection.java
@@ -895,7 +895,7 @@ private void initializeProtonResources(ReconnectLocation location) throws Client
         protonConnection.setLinkedResource(this);
         protonConnection.setChannelMax(options.channelMax());
         protonConnection.setMaxFrameSize(options.maxFrameSize());
-        protonConnection.setIdleTimeout((int) options.idleTimeout());
+        protonConnection.setIdleTimeout(options.idleTimeout() / 2);
         protonConnection.setOfferedCapabilities(ClientConversionSupport.toSymbolArray(options.offeredCapabilities()));
         protonConnection.setDesiredCapabilities(ClientConversionSupport.toSymbolArray(options.desiredCapabilities()));
         protonConnection.setProperties(ClientConversionSupport.toSymbolKeyedMap(options.properties()));

diff --git a/protonj2-client/src/test/java/org/apache/qpid/protonj2/client/ConnectionOptionsTest.java b/protonj2-client/src/test/java/org/apache/qpid/protonj2/client/ConnectionOptionsTest.java
@@ -20,10 +20,12 @@
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertNotSame;
 import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.junit.jupiter.api.Assertions.assertThrows;
 
 import java.util.HashMap;
 import java.util.Map;
 
+import org.apache.qpid.protonj2.types.UnsignedInteger;
 import org.junit.jupiter.api.Test;
 
 public class ConnectionOptionsTest {
@@ -37,6 +39,13 @@ public void testCreate() {
         assertNull(options.virtualHost());
     }
 
+    @Test
+    public void testSetIdleTimeoutValidesUIntRange() {
+        ConnectionOptions options = new ConnectionOptions();
+
+        assertThrows(NumberFormatException.class, () -> options.idleTimeout(UnsignedInteger.MAX_VALUE.longValue() + 1));
+    }
+
     @Test
     public void testCreateDefaultsTimeouts() {
         ConnectionOptions options = new ConnectionOptions();

diff --git a/protonj2-client/src/test/java/org/apache/qpid/protonj2/client/impl/ConnectionTest.java b/protonj2-client/src/test/java/org/apache/qpid/protonj2/client/impl/ConnectionTest.java
@@ -58,6 +58,7 @@
 import org.apache.qpid.protonj2.test.driver.codec.messaging.TerminusExpiryPolicy;
 import org.apache.qpid.protonj2.test.driver.codec.security.SaslCode;
 import org.apache.qpid.protonj2.test.driver.matchers.messaging.SourceMatcher;
+import org.apache.qpid.protonj2.types.UnsignedInteger;
 import org.apache.qpid.protonj2.types.messaging.AmqpValue;
 import org.apache.qpid.protonj2.types.transport.AMQPHeader;
 import org.apache.qpid.protonj2.types.transport.AmqpError;
@@ -1827,6 +1828,43 @@ public void testCreateConnectionWithEmptyVHostSendsNullValueInOpen() throws Exce
         }
     }
 
+    @Test
+    public void testCreateConnectionWithNoIdleTimeout() throws Exception {
+        doTestCreateConnectionWithIdleTimeoutSendsExpectedValue(0, 0);
+    }
+
+    @Test
+    public void testCreateConnectionWithHalfIdleTimeout() throws Exception {
+        doTestCreateConnectionWithIdleTimeoutSendsExpectedValue(10_000, 5_000);
+    }
+
+    @Test
+    public void testCreateConnectionWithHalfLargeIdleTimeout() throws Exception {
+        doTestCreateConnectionWithIdleTimeoutSendsExpectedValue(UnsignedInteger.MAX_VALUE.longValue(), UnsignedInteger.MAX_VALUE.longValue() / 2);
+    }
+
+    private void doTestCreateConnectionWithIdleTimeoutSendsExpectedValue(long setValue, long expectedValue) throws Exception {
+        try (ProtonTestServer peer = new ProtonTestServer(testServerOptions())) {
+            peer.expectSASLAnonymousConnect();
+            peer.expectOpen().withIdleTimeOut(expectedValue).respond();
+            peer.expectClose().respond();
+            peer.start();
+
+            URI remoteURI = peer.getServerURI();
+
+            LOG.info("Connect test started, peer listening on: {}", remoteURI);
+
+            Client container = Client.create();
+            ConnectionOptions options = connectionOptions().idleTimeout(setValue);
+            Connection connection = container.connect(remoteURI.getHost(), remoteURI.getPort(), options);
+
+            connection.openFuture().get(10, TimeUnit.SECONDS);
+            connection.closeAsync().get(10, TimeUnit.SECONDS);
+
+            peer.waitForScriptToComplete(5, TimeUnit.SECONDS);
+        }
+    }
+
     @Disabled("Disabled due to requirement of hard coded port")
     @Test
     public void testLocalPortOption() throws Exception {