From ba12141710aea4ec2b316fae248c17511dc4c89d Mon Sep 17 00:00:00 2001 From: osandamaleesha Date: Fri, 13 Sep 2024 13:10:00 +0530 Subject: [PATCH] Address reviewed changes --- .../action/execution/util/APIClient.java | 8 ++-- .../execution/util/ActionExecutorConfig.java | 44 ++++++++++++++----- .../util/ActionExecutorConfigTest.java | 39 +++++++++++++--- .../resources/identity.xml.j2 | 1 + ....identity.core.server.feature.default.json | 1 + 5 files changed, 72 insertions(+), 21 deletions(-) diff --git a/components/action-mgt/org.wso2.carbon.identity.action.execution/src/main/java/org/wso2/carbon/identity/action/execution/util/APIClient.java b/components/action-mgt/org.wso2.carbon.identity.action.execution/src/main/java/org/wso2/carbon/identity/action/execution/util/APIClient.java index e75c5dc046d..b1cf59fbc06 100644 --- a/components/action-mgt/org.wso2.carbon.identity.action.execution/src/main/java/org/wso2/carbon/identity/action/execution/util/APIClient.java +++ b/components/action-mgt/org.wso2.carbon.identity.action.execution/src/main/java/org/wso2/carbon/identity/action/execution/util/APIClient.java @@ -54,9 +54,9 @@ public APIClient() { // todo: read connection configurations related to the http client of actions from the server configuration. // Initialize the http client. Set connection time out to 2s and read time out to 5s. - int readTimeout = ActionExecutorConfig.getInstance().getHttpReadTimeout(); - int connectionRequestTimeout = ActionExecutorConfig.getInstance().getHttpConnectionRequestTimeout(); - int connectionTimeout = ActionExecutorConfig.getInstance().getHttpConnectionTimeout(); + int readTimeout = ActionExecutorConfig.getInstance().getHttpReadTimeoutInMillis(); + int connectionRequestTimeout = ActionExecutorConfig.getInstance().getHttpConnectionRequestTimeoutInMillis(); + int connectionTimeout = ActionExecutorConfig.getInstance().getHttpConnectionTimeoutInMillis(); RequestConfig config = RequestConfig.custom() .setConnectTimeout(connectionTimeout) @@ -66,7 +66,7 @@ public APIClient() { .setRelativeRedirectsAllowed(false) .build(); PoolingHttpClientConnectionManager connectionManager = new PoolingHttpClientConnectionManager(); - connectionManager.setMaxTotal(20); + connectionManager.setMaxTotal(ActionExecutorConfig.getInstance().getHttpClientConnectionPoolSize()); httpClient = HttpClientBuilder.create().setDefaultRequestConfig(config).setConnectionManager(connectionManager) .build(); } diff --git a/components/action-mgt/org.wso2.carbon.identity.action.execution/src/main/java/org/wso2/carbon/identity/action/execution/util/ActionExecutorConfig.java b/components/action-mgt/org.wso2.carbon.identity.action.execution/src/main/java/org/wso2/carbon/identity/action/execution/util/ActionExecutorConfig.java index cabbeb84a3c..189d7440be2 100644 --- a/components/action-mgt/org.wso2.carbon.identity.action.execution/src/main/java/org/wso2/carbon/identity/action/execution/util/ActionExecutorConfig.java +++ b/components/action-mgt/org.wso2.carbon.identity.action.execution/src/main/java/org/wso2/carbon/identity/action/execution/util/ActionExecutorConfig.java @@ -48,9 +48,11 @@ public class ActionExecutorConfig { private static final String HTTP_CONNECTION_REQUEST_TIMEOUT_PROPERTY = "Actions.HTTPConnections.HTTPConnectionRequestTimeout"; private static final String HTTP_CONNECTION_TIMEOUT_PROPERTY = "Actions.HTTPConnections.HTTPConnectionTimeout"; - private static final int DEFAULT_HTTP_READ_TIMEOUT = 5000; - private static final int DEFAULT_HTTP_CONNECTION_REQUEST_TIMEOUT = 2000; - private static final int DEFAULT_HTTP_CONNECTION_TIMEOUT = 2000; + private static final String HTTP_CLIENT_CONNECTION_POOL_SIZE_PROPERTY = "Actions.HTTPClientConnectionPoolSize"; + private static final int DEFAULT_HTTP_CLIENT_CONNECTION_POOL_SIZE_PROPERTY = 20; + private static final int DEFAULT_HTTP_READ_TIMEOUT_IN_MILLIS = 5000; + private static final int DEFAULT_HTTP_CONNECTION_REQUEST_TIMEOUT_MILLIS = 2000; + private static final int DEFAULT_HTTP_CONNECTION_TIMEOUT_MILLIS = 2000; private ActionExecutorConfig() { @@ -80,15 +82,34 @@ public boolean isExecutionForActionTypeEnabled(ActionType actionType) { } } + public int getHttpClientConnectionPoolSize() { + + int poolSizePropertyValue = DEFAULT_HTTP_CLIENT_CONNECTION_POOL_SIZE_PROPERTY; + String poolSizeValue = (String) IdentityConfigParser.getInstance().getConfiguration(). + get(HTTP_CLIENT_CONNECTION_POOL_SIZE_PROPERTY); + if (StringUtils.isNotBlank(poolSizeValue)) { + try { + poolSizePropertyValue = Integer.parseInt(poolSizeValue); + } catch (NumberFormatException e) { + if (LOG.isDebugEnabled()) { + LOG.debug("Failed to read Http client connection pool size property in identity.xml." + + " Expects a number. Using the default value: " + + DEFAULT_HTTP_CLIENT_CONNECTION_POOL_SIZE_PROPERTY, e); + } + } + } + return poolSizePropertyValue; + } + /** * Retrieves the HTTP read timeout configuration. * If the configuration value is invalid or missing, the default timeout value is parsed. * * @return The HTTP read timeout int value in milliseconds. */ - public int getHttpReadTimeout() { + public int getHttpReadTimeoutInMillis() { - return parseTimeoutConfig(HTTP_READ_TIMEOUT_PROPERTY, DEFAULT_HTTP_READ_TIMEOUT); + return parseTimeoutConfig(HTTP_READ_TIMEOUT_PROPERTY, DEFAULT_HTTP_READ_TIMEOUT_IN_MILLIS); } /** @@ -97,9 +118,10 @@ public int getHttpReadTimeout() { * * @return The HTTP connection request timeout int value in milliseconds. */ - public int getHttpConnectionRequestTimeout() { + public int getHttpConnectionRequestTimeoutInMillis() { - return parseTimeoutConfig(HTTP_CONNECTION_REQUEST_TIMEOUT_PROPERTY, DEFAULT_HTTP_CONNECTION_REQUEST_TIMEOUT); + return parseTimeoutConfig(HTTP_CONNECTION_REQUEST_TIMEOUT_PROPERTY, + DEFAULT_HTTP_CONNECTION_REQUEST_TIMEOUT_MILLIS); } /** @@ -108,9 +130,9 @@ public int getHttpConnectionRequestTimeout() { * * @return The HTTP connection timeout int value in milliseconds. */ - public int getHttpConnectionTimeout() { + public int getHttpConnectionTimeoutInMillis() { - return parseTimeoutConfig(HTTP_CONNECTION_TIMEOUT_PROPERTY, DEFAULT_HTTP_CONNECTION_TIMEOUT); + return parseTimeoutConfig(HTTP_CONNECTION_TIMEOUT_PROPERTY, DEFAULT_HTTP_CONNECTION_TIMEOUT_MILLIS); } private int parseTimeoutConfig(String timeoutTypeName, int defaultTimeout) { @@ -122,8 +144,8 @@ private int parseTimeoutConfig(String timeoutTypeName, int defaultTimeout) { timeoutPropertyValue = Integer.parseInt(timeoutValue); } catch (NumberFormatException e) { if (LOG.isDebugEnabled()) { - LOG.debug(" " + timeoutTypeName + " property value must be an integer in identity.xml. " + - "Default property value is parsed ", e); + LOG.debug("Failed to read " + timeoutTypeName + " property in identity.xml." + + " Expects a number. Using the default value: " + defaultTimeout, e); } } } diff --git a/components/action-mgt/org.wso2.carbon.identity.action.execution/src/test/java/org/wso2/carbon/identity/action/execution/util/ActionExecutorConfigTest.java b/components/action-mgt/org.wso2.carbon.identity.action.execution/src/test/java/org/wso2/carbon/identity/action/execution/util/ActionExecutorConfigTest.java index 1b7749ab3d4..4f913662424 100644 --- a/components/action-mgt/org.wso2.carbon.identity.action.execution/src/test/java/org/wso2/carbon/identity/action/execution/util/ActionExecutorConfigTest.java +++ b/components/action-mgt/org.wso2.carbon.identity.action.execution/src/test/java/org/wso2/carbon/identity/action/execution/util/ActionExecutorConfigTest.java @@ -305,29 +305,56 @@ public void testGetExcludedParamsInActionRequestForEmptyConfigForAllAndDefinedTy } @Test - public void testGetHttpReadTimeout() { + public void testGetHttpReadTimeoutInMillis() { Map configMap = new HashMap<>(); configMap.put("Actions.HTTPConnections.HTTPReadTimeout", "5000"); when(mockIdentityConfigParser.getConfiguration()).thenReturn(configMap); - Assert.assertEquals(5000, actionExecutorConfig.getHttpReadTimeout()); + Assert.assertEquals(5000, actionExecutorConfig.getHttpReadTimeoutInMillis()); } @Test - public void testGetHttpConnectionRequestTimeout() { + public void testGetHttpReadTimeoutInMillisWithoutNumber(){ + + Map configMap = new HashMap<>(); + configMap.put("Actions.HTTPConnections.HTTPReadTimeout", "value"); + when(mockIdentityConfigParser.getConfiguration()).thenReturn(configMap); + Assert.assertEquals(5000, actionExecutorConfig.getHttpReadTimeoutInMillis()); + } + + @Test + public void testGetHttpConnectionRequestTimeoutInMillis() { Map configMap = new HashMap<>(); configMap.put("Actions.HTTPConnections.HTTPConnectionRequestTimeout", "2000"); when(mockIdentityConfigParser.getConfiguration()).thenReturn(configMap); - Assert.assertEquals(2000, actionExecutorConfig.getHttpConnectionRequestTimeout()); + Assert.assertEquals(2000, actionExecutorConfig.getHttpConnectionRequestTimeoutInMillis()); } @Test - public void testGetHttpConnectionTimeout() { + public void testGetHttpConnectionTimeoutInMillis() { Map configMap = new HashMap<>(); configMap.put("Actions.HTTPConnections.HTTPConnectionTimeout", "2000"); when(mockIdentityConfigParser.getConfiguration()).thenReturn(configMap); - Assert.assertEquals(2000, actionExecutorConfig.getHttpConnectionTimeout()); + Assert.assertEquals(2000, actionExecutorConfig.getHttpConnectionTimeoutInMillis()); + } + + @Test + public void testGetHttpClientConnectionPoolSize() { + + Map configMap = new HashMap<>(); + configMap.put("Actions.HTTPClientConnectionPoolSize", "20"); + when(mockIdentityConfigParser.getConfiguration()).thenReturn(configMap); + Assert.assertEquals(20, actionExecutorConfig.getHttpClientConnectionPoolSize()); + } + + @Test + public void testGetHttpClientConnectionPoolSizeWithoutNumberFormat() { + + Map configMap = new HashMap<>(); + configMap.put("Actions.HTTPClientConnectionPoolSize", "value"); + when(mockIdentityConfigParser.getConfiguration()).thenReturn(configMap); + Assert.assertEquals(20, actionExecutorConfig.getHttpClientConnectionPoolSize()); } } diff --git a/features/identity-core/org.wso2.carbon.identity.core.server.feature/resources/identity.xml.j2 b/features/identity-core/org.wso2.carbon.identity.core.server.feature/resources/identity.xml.j2 index fccd75b1d0f..fd9ec6dfc37 100644 --- a/features/identity-core/org.wso2.carbon.identity.core.server.feature/resources/identity.xml.j2 +++ b/features/identity-core/org.wso2.carbon.identity.core.server.feature/resources/identity.xml.j2 @@ -2025,6 +2025,7 @@ {{actions.http_connections.read_timeout}} {{actions.http_connections.request_timeout}} + {{actions.http_client_connection_pool_size}} {{actions.maximum_actions_per_action_type}} diff --git a/features/identity-core/org.wso2.carbon.identity.core.server.feature/resources/org.wso2.carbon.identity.core.server.feature.default.json b/features/identity-core/org.wso2.carbon.identity.core.server.feature/resources/org.wso2.carbon.identity.core.server.feature.default.json index 2823abb06cf..c3701720b56 100644 --- a/features/identity-core/org.wso2.carbon.identity.core.server.feature/resources/org.wso2.carbon.identity.core.server.feature.default.json +++ b/features/identity-core/org.wso2.carbon.identity.core.server.feature/resources/org.wso2.carbon.identity.core.server.feature.default.json @@ -1574,6 +1574,7 @@ "actions.http_connections.read_timeout": 5000, "actions.http_connections.request_timeout": 2000, "actions.http_connections.connection_timeout": 2000, + "actions.http_client_connection_pool_size": 20, "actions.maximum_actions_per_action_type": 1, "actions.action_request.excluded_headers": [ "authorization",