From 33c12f4650fb3cbc31be0e82b195e305405e276f Mon Sep 17 00:00:00 2001 From: Kushagra Thapar Date: Thu, 9 Jul 2020 11:43:41 -0700 Subject: [PATCH] Added idle tcp connection timeout bug where gateway connection overrides direct connection config --- .../com/azure/cosmos/CosmosClientBuilder.java | 4 +- .../implementation/ConnectionPolicy.java | 54 +++++++++++++++---- .../implementation/RxDocumentClientImpl.java | 2 +- .../RntbdTransportClient.java | 4 +- .../azure/cosmos/ConnectionConfigTest.java | 49 +++++++++-------- .../models/CosmosPartitionKeyTests.java | 2 +- 6 files changed, 74 insertions(+), 41 deletions(-) diff --git a/sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/CosmosClientBuilder.java b/sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/CosmosClientBuilder.java index ed30bac228434..13892748126c6 100644 --- a/sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/CosmosClientBuilder.java +++ b/sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/CosmosClientBuilder.java @@ -668,8 +668,10 @@ private void buildConnectionPolicy() { // Check if the user passed additional gateway connection configuration if (this.gatewayConnectionConfig != null) { this.connectionPolicy.setMaxConnectionPoolSize(this.gatewayConnectionConfig.getMaxConnectionPoolSize()); + // TODO(kuthapar): potential bug - when we expose requestTimeout from direct and gateway connection config, + // as gateway connection config will overwrite direct connection config settings this.connectionPolicy.setRequestTimeout(this.gatewayConnectionConfig.getRequestTimeout()); - this.connectionPolicy.setIdleConnectionTimeout(this.gatewayConnectionConfig.getIdleConnectionTimeout()); + this.connectionPolicy.setIdleHttpConnectionTimeout(this.gatewayConnectionConfig.getIdleConnectionTimeout()); } } else if (gatewayConnectionConfig != null) { this.connectionPolicy = new ConnectionPolicy(gatewayConnectionConfig); diff --git a/sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/implementation/ConnectionPolicy.java b/sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/implementation/ConnectionPolicy.java index 9362d784712e0..a9433289dcc9c 100644 --- a/sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/implementation/ConnectionPolicy.java +++ b/sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/implementation/ConnectionPolicy.java @@ -27,7 +27,6 @@ public final class ConnectionPolicy { private ConnectionMode connectionMode; private boolean endpointDiscoveryEnabled; - private Duration idleConnectionTimeout; private boolean multipleWriteRegionsEnabled; private List preferredRegions; private boolean readRequestsFallbackEnabled; @@ -38,19 +37,21 @@ public final class ConnectionPolicy { private int maxConnectionPoolSize; private Duration requestTimeout; private ProxyOptions proxy; + private Duration idleHttpConnectionTimeout; // Direct connection config properties private Duration connectTimeout; private Duration idleEndpointTimeout; private int maxConnectionsPerEndpoint; private int maxRequestsPerConnection; + private Duration idleTcpConnectionTimeout; /** * Constructor. */ public ConnectionPolicy(GatewayConnectionConfig gatewayConnectionConfig) { this(ConnectionMode.GATEWAY); - this.idleConnectionTimeout = gatewayConnectionConfig.getIdleConnectionTimeout(); + this.idleHttpConnectionTimeout = gatewayConnectionConfig.getIdleConnectionTimeout(); this.maxConnectionPoolSize = gatewayConnectionConfig.getMaxConnectionPoolSize(); this.requestTimeout = BridgeInternal.getRequestTimeoutFromGatewayConnectionConfig(gatewayConnectionConfig); this.proxy = gatewayConnectionConfig.getProxy(); @@ -59,7 +60,7 @@ public ConnectionPolicy(GatewayConnectionConfig gatewayConnectionConfig) { public ConnectionPolicy(DirectConnectionConfig directConnectionConfig) { this(ConnectionMode.DIRECT); this.connectTimeout = directConnectionConfig.getConnectTimeout(); - this.idleConnectionTimeout = directConnectionConfig.getIdleConnectionTimeout(); + this.idleTcpConnectionTimeout = directConnectionConfig.getIdleConnectionTimeout(); this.idleEndpointTimeout = directConnectionConfig.getIdleEndpointTimeout(); this.maxConnectionsPerEndpoint = directConnectionConfig.getMaxConnectionsPerEndpoint(); this.maxRequestsPerConnection = directConnectionConfig.getMaxRequestsPerConnection(); @@ -149,24 +150,54 @@ public ConnectionPolicy setMaxConnectionPoolSize(int maxConnectionPoolSize) { } /** - * Gets the value of the timeout for an idle connection, the default is 60 + * Gets the value of the timeout for an idle http connection, the default is 60 * seconds. * * @return Idle connection timeout duration. */ - public Duration getIdleConnectionTimeout() { - return this.idleConnectionTimeout; + public Duration getIdleHttpConnectionTimeout() { + return this.idleHttpConnectionTimeout; } /** - * sets the value of the timeout for an idle connection. After that time, + * sets the value of the timeout for an idle http connection. After that time, * the connection will be automatically closed. * - * @param idleConnectionTimeout the duration for an idle connection. + * @param idleHttpConnectionTimeout the duration for an idle connection. * @return the ConnectionPolicy. */ - public ConnectionPolicy setIdleConnectionTimeout(Duration idleConnectionTimeout) { - this.idleConnectionTimeout = idleConnectionTimeout; + public ConnectionPolicy setIdleHttpConnectionTimeout(Duration idleHttpConnectionTimeout) { + this.idleHttpConnectionTimeout = idleHttpConnectionTimeout; + return this; + } + + /** + * Gets the idle tcp connection timeout for direct client + * + * Default value is {@link Duration#ZERO} + * + * Direct client doesn't close a single connection to an endpoint + * by default unless specified. + * + * @return idle tcp connection timeout + */ + public Duration getIdleTcpConnectionTimeout() { + return idleTcpConnectionTimeout; + } + + /** + * Sets the idle tcp connection timeout + * + * Default value is {@link Duration#ZERO} + * + * Direct client doesn't close a single connection to an endpoint + * by default unless specified. + * + * @param idleTcpConnectionTimeout idle connection timeout + * @return the {@link ConnectionPolicy} + */ + public ConnectionPolicy setIdleTcpConnectionTimeout(Duration idleTcpConnectionTimeout) { + this.idleTcpConnectionTimeout = idleTcpConnectionTimeout; return this; } @@ -451,7 +482,8 @@ public String toString() { "requestTimeout=" + requestTimeout + ", connectionMode=" + connectionMode + ", maxConnectionPoolSize=" + maxConnectionPoolSize + - ", idleConnectionTimeout=" + idleConnectionTimeout + + ", idleHttpConnectionTimeout=" + idleHttpConnectionTimeout + + ", idleTcpConnectionTimeout=" + idleTcpConnectionTimeout + ", userAgentSuffix='" + userAgentSuffix + '\'' + ", throttlingRetryOptions=" + throttlingRetryOptions + ", endpointDiscoveryEnabled=" + endpointDiscoveryEnabled + diff --git a/sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/implementation/RxDocumentClientImpl.java b/sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/implementation/RxDocumentClientImpl.java index a004f7ab68bc5..120f0efd21bc6 100644 --- a/sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/implementation/RxDocumentClientImpl.java +++ b/sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/implementation/RxDocumentClientImpl.java @@ -357,7 +357,7 @@ RxGatewayStoreModel createRxGatewayProxy(ISessionContainer sessionContainer, private HttpClient httpClient() { HttpClientConfig httpClientConfig = new HttpClientConfig(this.configs) - .withMaxIdleConnectionTimeout(this.connectionPolicy.getIdleConnectionTimeout()) + .withMaxIdleConnectionTimeout(this.connectionPolicy.getIdleHttpConnectionTimeout()) .withPoolSize(this.connectionPolicy.getMaxConnectionPoolSize()) .withProxy(this.connectionPolicy.getProxy()) .withRequestTimeout(this.connectionPolicy.getRequestTimeout()); diff --git a/sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/implementation/directconnectivity/RntbdTransportClient.java b/sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/implementation/directconnectivity/RntbdTransportClient.java index 71682cdbd2846..837163ab5725e 100644 --- a/sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/implementation/directconnectivity/RntbdTransportClient.java +++ b/sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/implementation/directconnectivity/RntbdTransportClient.java @@ -254,7 +254,7 @@ private Options(final ConnectionPolicy connectionPolicy) { this.bufferPageSize = 8192; this.connectionAcquisitionTimeout = Duration.ZERO; this.connectTimeout = connectionPolicy.getConnectTimeout(); - this.idleChannelTimeout = connectionPolicy.getIdleConnectionTimeout(); + this.idleChannelTimeout = connectionPolicy.getIdleTcpConnectionTimeout(); this.idleEndpointTimeout = Duration.ofSeconds(70L); this.maxBufferCapacity = 8192 << 10; this.maxChannelsPerEndpoint = connectionPolicy.getMaxConnectionsPerEndpoint(); @@ -484,7 +484,7 @@ public Builder(ConnectionPolicy connectionPolicy) { this.bufferPageSize = DEFAULT_OPTIONS.bufferPageSize; this.connectionAcquisitionTimeout = DEFAULT_OPTIONS.connectionAcquisitionTimeout; this.connectTimeout = connectionPolicy.getConnectTimeout(); - this.idleChannelTimeout = connectionPolicy.getIdleConnectionTimeout(); + this.idleChannelTimeout = connectionPolicy.getIdleTcpConnectionTimeout(); this.idleEndpointTimeout = DEFAULT_OPTIONS.idleEndpointTimeout; this.maxBufferCapacity = DEFAULT_OPTIONS.maxBufferCapacity; this.maxChannelsPerEndpoint = connectionPolicy.getMaxConnectionsPerEndpoint(); diff --git a/sdk/cosmos/azure-cosmos/src/test/java/com/azure/cosmos/ConnectionConfigTest.java b/sdk/cosmos/azure-cosmos/src/test/java/com/azure/cosmos/ConnectionConfigTest.java index cf0dc55b925a0..a7a01d1f68eea 100644 --- a/sdk/cosmos/azure-cosmos/src/test/java/com/azure/cosmos/ConnectionConfigTest.java +++ b/sdk/cosmos/azure-cosmos/src/test/java/com/azure/cosmos/ConnectionConfigTest.java @@ -12,7 +12,6 @@ import java.time.Duration; import java.util.ArrayList; import java.util.List; -import java.util.Objects; import static org.assertj.core.api.Assertions.assertThat; @@ -37,7 +36,7 @@ public void buildClient_withDefaultGatewayConnectionConfig() { AsyncDocumentClient asyncDocumentClient = CosmosBridgeInternal.getAsyncDocumentClient(cosmosClient); ConnectionPolicy connectionPolicy = asyncDocumentClient.getConnectionPolicy(); - assertThat(connectionPolicy.getConnectionMode().equals(ConnectionMode.GATEWAY)); + assertThat(connectionPolicy.getConnectionMode()).isEqualTo(ConnectionMode.GATEWAY); validateGatewayConnectionConfig(connectionPolicy, cosmosClientBuilder, gatewayConnectionConfig); safeCloseSyncClient(cosmosClient); } @@ -66,7 +65,7 @@ public void buildClient_withCustomGatewayConnectionConfig() { AsyncDocumentClient asyncDocumentClient = CosmosBridgeInternal.getAsyncDocumentClient(cosmosClient); ConnectionPolicy connectionPolicy = asyncDocumentClient.getConnectionPolicy(); - assertThat(connectionPolicy.getConnectionMode().equals(ConnectionMode.GATEWAY)); + assertThat(connectionPolicy.getConnectionMode()).isEqualTo(ConnectionMode.GATEWAY); validateGatewayConnectionConfig(connectionPolicy, cosmosClientBuilder, gatewayConnectionConfig); safeCloseSyncClient(cosmosClient); } @@ -84,7 +83,7 @@ public void buildClient_withDefaultDirectConnectionConfig() { AsyncDocumentClient asyncDocumentClient = CosmosBridgeInternal.getAsyncDocumentClient(cosmosClient); ConnectionPolicy connectionPolicy = asyncDocumentClient.getConnectionPolicy(); - assertThat(connectionPolicy.getConnectionMode().equals(ConnectionMode.DIRECT)); + assertThat(connectionPolicy.getConnectionMode()).isEqualTo(ConnectionMode.DIRECT); validateDirectConnectionConfig(connectionPolicy, cosmosClientBuilder, directConnectionConfig); safeCloseSyncClient(cosmosClient); } @@ -113,7 +112,7 @@ public void buildClient_withCustomDirectConnectionConfig() { AsyncDocumentClient asyncDocumentClient = CosmosBridgeInternal.getAsyncDocumentClient(cosmosClient); ConnectionPolicy connectionPolicy = asyncDocumentClient.getConnectionPolicy(); - assertThat(connectionPolicy.getConnectionMode().equals(ConnectionMode.DIRECT)); + assertThat(connectionPolicy.getConnectionMode()).isEqualTo(ConnectionMode.DIRECT); validateDirectConnectionConfig(connectionPolicy, cosmosClientBuilder, directConnectionConfig); safeCloseSyncClient(cosmosClient); } @@ -133,7 +132,7 @@ public void buildClient_withDirectAndGatewayConnectionConfig() { AsyncDocumentClient asyncDocumentClient = CosmosBridgeInternal.getAsyncDocumentClient(cosmosClient); ConnectionPolicy connectionPolicy = asyncDocumentClient.getConnectionPolicy(); - assertThat(connectionPolicy.getConnectionMode().equals(ConnectionMode.DIRECT)); + assertThat(connectionPolicy.getConnectionMode()).isEqualTo(ConnectionMode.DIRECT); validateDirectAndGatewayConnectionConfig(connectionPolicy, cosmosClientBuilder, directConnectionConfig, gatewayConnectionConfig); safeCloseSyncClient(cosmosClient); } @@ -149,7 +148,7 @@ public void buildClient_withNoConnectionConfig() { AsyncDocumentClient asyncDocumentClient = CosmosBridgeInternal.getAsyncDocumentClient(cosmosClient); ConnectionPolicy connectionPolicy = asyncDocumentClient.getConnectionPolicy(); - assertThat(connectionPolicy.getConnectionMode().equals(ConnectionMode.DIRECT)); + assertThat(connectionPolicy.getConnectionMode()).isEqualTo(ConnectionMode.DIRECT); validateDirectConnectionConfig(connectionPolicy, cosmosClientBuilder, DirectConnectionConfig.getDefaultConfig()); safeCloseSyncClient(cosmosClient); } @@ -157,44 +156,44 @@ public void buildClient_withNoConnectionConfig() { private void validateDirectAndGatewayConnectionConfig(ConnectionPolicy connectionPolicy, CosmosClientBuilder cosmosClientBuilder, DirectConnectionConfig directConnectionConfig, GatewayConnectionConfig gatewayConnectionConfig) { validateCommonConnectionConfig(connectionPolicy, cosmosClientBuilder); - assertThat(Objects.equals(connectionPolicy.getConnectionMode(), ConnectionMode.DIRECT)); + assertThat(connectionPolicy.getConnectionMode()).isEqualTo(ConnectionMode.DIRECT); validateDirectConfig(connectionPolicy, directConnectionConfig); validateGatewayConfig(connectionPolicy, gatewayConnectionConfig); } private void validateGatewayConnectionConfig(ConnectionPolicy connectionPolicy, CosmosClientBuilder cosmosClientBuilder, GatewayConnectionConfig gatewayConnectionConfig) { validateCommonConnectionConfig(connectionPolicy, cosmosClientBuilder); - assertThat(Objects.equals(connectionPolicy.getConnectionMode(), ConnectionMode.GATEWAY)); + assertThat(connectionPolicy.getConnectionMode()).isEqualTo(ConnectionMode.GATEWAY); validateGatewayConfig(connectionPolicy, gatewayConnectionConfig); } private void validateDirectConnectionConfig(ConnectionPolicy connectionPolicy, CosmosClientBuilder cosmosClientBuilder, DirectConnectionConfig directConnectionConfig) { validateCommonConnectionConfig(connectionPolicy, cosmosClientBuilder); - assertThat(Objects.equals(connectionPolicy.getConnectionMode(), ConnectionMode.DIRECT)); + assertThat(connectionPolicy.getConnectionMode()).isEqualTo(ConnectionMode.DIRECT); validateDirectConfig(connectionPolicy, directConnectionConfig); } private void validateCommonConnectionConfig(ConnectionPolicy connectionPolicy, CosmosClientBuilder cosmosClientBuilder) { - assertThat(Objects.equals(connectionPolicy.isMultipleWriteRegionsEnabled(), cosmosClientBuilder.isMultipleWriteRegionsEnabled())); - assertThat(Objects.equals(connectionPolicy.isEndpointDiscoveryEnabled(), cosmosClientBuilder.isEndpointDiscoveryEnabled())); - assertThat(Objects.equals(connectionPolicy.isReadRequestsFallbackEnabled(), cosmosClientBuilder.isReadRequestsFallbackEnabled())); - assertThat(Objects.equals(connectionPolicy.getPreferredRegions(), cosmosClientBuilder.getPreferredRegions())); - assertThat(Objects.equals(connectionPolicy.getThrottlingRetryOptions(), cosmosClientBuilder.getThrottlingRetryOptions())); - assertThat(Objects.equals(connectionPolicy.getUserAgentSuffix(), cosmosClientBuilder.getUserAgentSuffix())); + assertThat(connectionPolicy.isMultipleWriteRegionsEnabled()).isEqualTo(cosmosClientBuilder.isMultipleWriteRegionsEnabled()); + assertThat(connectionPolicy.isEndpointDiscoveryEnabled()).isEqualTo(cosmosClientBuilder.isEndpointDiscoveryEnabled()); + assertThat(connectionPolicy.isReadRequestsFallbackEnabled()).isEqualTo(cosmosClientBuilder.isReadRequestsFallbackEnabled()); + assertThat(connectionPolicy.getPreferredRegions()).isEqualTo(cosmosClientBuilder.getPreferredRegions()); + assertThat(connectionPolicy.getThrottlingRetryOptions()).isEqualTo(cosmosClientBuilder.getThrottlingRetryOptions()); + assertThat(connectionPolicy.getUserAgentSuffix()).isEqualTo(cosmosClientBuilder.getUserAgentSuffix()); } private void validateGatewayConfig(ConnectionPolicy connectionPolicy, GatewayConnectionConfig gatewayConnectionConfig) { - assertThat(Objects.equals(connectionPolicy.getIdleConnectionTimeout(), gatewayConnectionConfig.getIdleConnectionTimeout())); - assertThat(Objects.equals(connectionPolicy.getMaxConnectionPoolSize(), gatewayConnectionConfig.getMaxConnectionPoolSize())); - assertThat(Objects.equals(connectionPolicy.getRequestTimeout(), gatewayConnectionConfig.getRequestTimeout())); - assertThat(Objects.equals(connectionPolicy.getProxy(), gatewayConnectionConfig.getProxy())); + assertThat(connectionPolicy.getIdleHttpConnectionTimeout()).isEqualTo(gatewayConnectionConfig.getIdleConnectionTimeout()); + assertThat(connectionPolicy.getMaxConnectionPoolSize()).isEqualTo(gatewayConnectionConfig.getMaxConnectionPoolSize()); + assertThat(connectionPolicy.getRequestTimeout()).isEqualTo(gatewayConnectionConfig.getRequestTimeout()); + assertThat(connectionPolicy.getProxy()).isEqualTo(gatewayConnectionConfig.getProxy()); } private void validateDirectConfig(ConnectionPolicy connectionPolicy, DirectConnectionConfig directConnectionConfig) { - assertThat(Objects.equals(connectionPolicy.getConnectTimeout(), directConnectionConfig.getConnectTimeout())); - assertThat(Objects.equals(connectionPolicy.getIdleConnectionTimeout(), directConnectionConfig.getIdleConnectionTimeout())); - assertThat(Objects.equals(connectionPolicy.getIdleEndpointTimeout(), directConnectionConfig.getIdleEndpointTimeout())); - assertThat(Objects.equals(connectionPolicy.getMaxConnectionsPerEndpoint(), directConnectionConfig.getMaxConnectionsPerEndpoint())); - assertThat(Objects.equals(connectionPolicy.getMaxRequestsPerConnection(), directConnectionConfig.getMaxRequestsPerConnection())); + assertThat(connectionPolicy.getConnectTimeout()).isEqualTo(directConnectionConfig.getConnectTimeout()); + assertThat(connectionPolicy.getIdleTcpConnectionTimeout()).isEqualTo(directConnectionConfig.getIdleConnectionTimeout()); + assertThat(connectionPolicy.getIdleEndpointTimeout()).isEqualTo(directConnectionConfig.getIdleEndpointTimeout()); + assertThat(connectionPolicy.getMaxConnectionsPerEndpoint()).isEqualTo(directConnectionConfig.getMaxConnectionsPerEndpoint()); + assertThat(connectionPolicy.getMaxRequestsPerConnection()).isEqualTo(directConnectionConfig.getMaxRequestsPerConnection()); } } diff --git a/sdk/cosmos/azure-cosmos/src/test/java/com/azure/cosmos/models/CosmosPartitionKeyTests.java b/sdk/cosmos/azure-cosmos/src/test/java/com/azure/cosmos/models/CosmosPartitionKeyTests.java index 58f2ea67d7fe8..87c6c943a1fec 100644 --- a/sdk/cosmos/azure-cosmos/src/test/java/com/azure/cosmos/models/CosmosPartitionKeyTests.java +++ b/sdk/cosmos/azure-cosmos/src/test/java/com/azure/cosmos/models/CosmosPartitionKeyTests.java @@ -78,7 +78,7 @@ public void afterClass() { private void createContainerWithoutPk() throws URISyntaxException, IOException { ConnectionPolicy connectionPolicy = new ConnectionPolicy(DirectConnectionConfig.getDefaultConfig()); HttpClientConfig httpClientConfig = new HttpClientConfig(new Configs()) - .withMaxIdleConnectionTimeout(connectionPolicy.getIdleConnectionTimeout()) + .withMaxIdleConnectionTimeout(connectionPolicy.getIdleHttpConnectionTimeout()) .withPoolSize(connectionPolicy.getMaxConnectionPoolSize()) .withProxy(connectionPolicy.getProxy()) .withRequestTimeout(connectionPolicy.getRequestTimeout());