Introduce readTimeout and connectTimeout to Jira Define explicit timeouts for connection and read from Jira backend. Allows to prevent incoming git-receive-pack threads to become blocked because of a slowdown of connectivity or execution times on the Jira side. Bug: Issue 15702 Change-Id: Id94892a50363d5be3fee4007c1357cf64927ce41
diff --git a/src/main/java/com/googlesource/gerrit/plugins/its/jira/InitJira.java b/src/main/java/com/googlesource/gerrit/plugins/its/jira/InitJira.java index a5c16ec..ccab69c 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/its/jira/InitJira.java +++ b/src/main/java/com/googlesource/gerrit/plugins/its/jira/InitJira.java
@@ -21,6 +21,7 @@ import com.google.gerrit.pgm.init.api.ConsoleUI; import com.google.gerrit.pgm.init.api.InitFlags; import com.google.gerrit.pgm.init.api.Section; +import com.google.gerrit.server.config.ConfigUtil; import com.google.inject.Inject; import com.google.inject.Singleton; import com.googlesource.gerrit.plugins.its.base.its.InitIts; @@ -30,7 +31,9 @@ import com.googlesource.gerrit.plugins.its.jira.restapi.JiraURL; import java.io.IOException; import java.net.MalformedURLException; +import java.time.Duration; import java.util.Arrays; +import java.util.concurrent.TimeUnit; import org.eclipse.jgit.errors.ConfigInvalidException; /** Initialize the GitRepositoryManager configuration section. */ @@ -43,6 +46,8 @@ private JiraURL jiraUrl; private String jiraUsername; private String jiraPassword; + private String jiraConnectionTimeout; + private String jiraReadTimeout; @Inject InitJira( @@ -123,9 +128,24 @@ public void enterJiraConnectivity() throws MalformedURLException { String jiraUrlString = jira.string("Jira URL (empty to skip)", "url", null); if (jiraUrlString != null) { - jiraUrl = new JiraURL(jiraUrlString); jiraUsername = jira.string("Jira username", "username", ""); jiraPassword = jira.password("username", "password"); + jiraConnectionTimeout = + jira.string( + "Connection timeout", + JiraConfig.GERRIT_CONFIG_CONNECT_TIMEOUT, + JiraConfig.GERRIT_CONFIG_CONNECT_TIMEOUT_DEFAULT.toMillis() + " ms"); + jiraReadTimeout = + jira.string( + "Read timeout", + JiraConfig.GERRIT_CONFIG_READ_TIMEOUT, + JiraConfig.GERRIT_CONFIG_READ_TIMEOUT_DEFAULT.toMillis() + " ms"); + jiraUrl = + new JiraURL( + jiraUrlString, + Duration.ofMillis( + ConfigUtil.getTimeUnit(jiraConnectionTimeout, 0, TimeUnit.MILLISECONDS)), + Duration.ofMillis(ConfigUtil.getTimeUnit(jiraReadTimeout, 0, TimeUnit.MILLISECONDS))); } }
diff --git a/src/main/java/com/googlesource/gerrit/plugins/its/jira/JiraConfig.java b/src/main/java/com/googlesource/gerrit/plugins/its/jira/JiraConfig.java index 339d16c..b0e8bcb 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/its/jira/JiraConfig.java +++ b/src/main/java/com/googlesource/gerrit/plugins/its/jira/JiraConfig.java
@@ -21,6 +21,7 @@ import com.google.gerrit.entities.StoredCommentLinkInfo; import com.google.gerrit.extensions.annotations.PluginName; import com.google.gerrit.server.GerritPersonIdent; +import com.google.gerrit.server.config.ConfigUtil; import com.google.gerrit.server.config.GerritServerConfig; import com.google.gerrit.server.config.PluginConfig; import com.google.gerrit.server.config.PluginConfigFactory; @@ -35,6 +36,8 @@ import com.googlesource.gerrit.plugins.its.jira.restapi.JiraURL; import com.googlesource.gerrit.plugins.its.jira.restapi.JiraVisibilityType; import java.io.IOException; +import java.time.Duration; +import java.util.concurrent.TimeUnit; import org.eclipse.jgit.errors.ConfigInvalidException; import org.eclipse.jgit.lib.Config; import org.eclipse.jgit.lib.PersonIdent; @@ -58,6 +61,10 @@ private static final String GERRIT_CONFIG_PASSWORD = "password"; private static final String GERRIT_CONFIG_COMMENT_VISIBILITY_TYPE = "visibilityType"; private static final String GERRIT_CONFIG_COMMENT_VISIBILITY_VALUE = "visibilityValue"; + public static final String GERRIT_CONFIG_CONNECT_TIMEOUT = "connectTimeout"; + public static final Duration GERRIT_CONFIG_CONNECT_TIMEOUT_DEFAULT = Duration.ofMinutes(2); + public static final String GERRIT_CONFIG_READ_TIMEOUT = "readTimeout"; + public static final Duration GERRIT_CONFIG_READ_TIMEOUT_DEFAULT = Duration.ofSeconds(30); private final String pluginName; private final PluginConfigFactory cfgFactory; @@ -96,9 +103,33 @@ gerritConfig.getEnum( pluginName, null, GERRIT_CONFIG_COMMENT_VISIBILITY_TYPE, JiraVisibilityType.NOTSET), gerritConfig.getString(pluginName, null, GERRIT_CONFIG_COMMENT_VISIBILITY_VALUE)) + .connectTimeout( + getDurationFromConfig( + gerritConfig, + pluginName, + GERRIT_CONFIG_CONNECT_TIMEOUT, + GERRIT_CONFIG_CONNECT_TIMEOUT_DEFAULT)) + .readTimeout( + getDurationFromConfig( + gerritConfig, + pluginName, + GERRIT_CONFIG_READ_TIMEOUT, + GERRIT_CONFIG_READ_TIMEOUT_DEFAULT)) .build(); } + private static Duration getDurationFromConfig( + Config gerritConfig, String pluginName, String setting, Duration defaultDuration) { + return Duration.ofMillis( + ConfigUtil.getTimeUnit( + gerritConfig, + pluginName, + null, + setting, + defaultDuration.toMillis(), + TimeUnit.MILLISECONDS)); + } + JiraItsServerInfo getDefaultServerInfo() { return defaultJiraServerInfo; } @@ -119,9 +150,24 @@ PROJECT_CONFIG_COMMENT_VISIBILITY_TYPE, JiraVisibilityType.NOTSET), pluginConfig.getString(PROJECT_CONFIG_COMMENT_VISIBILITY_VALUE, null)) + .connectTimeout( + getDurationFromConfig( + pluginConfig, GERRIT_CONFIG_CONNECT_TIMEOUT, GERRIT_CONFIG_CONNECT_TIMEOUT_DEFAULT)) + .readTimeout( + getDurationFromConfig( + pluginConfig, GERRIT_CONFIG_READ_TIMEOUT, GERRIT_CONFIG_READ_TIMEOUT_DEFAULT)) .build(); } + private static Duration getDurationFromConfig( + PluginConfig pluginConfig, String setting, Duration defaultDuration) { + return Duration.ofMillis( + ConfigUtil.getTimeUnit( + pluginConfig.getString(setting, ""), + defaultDuration.toMillis(), + TimeUnit.MILLISECONDS)); + } + void addCommentLinksSection(Project.NameKey projectName, JiraItsServerInfo jiraItsServerInfo) { try (Repository git = repoManager.openRepository(projectName); MetaDataUpdate md = new MetaDataUpdate(GitReferenceUpdated.DISABLED, projectName, git)) {
diff --git a/src/main/java/com/googlesource/gerrit/plugins/its/jira/JiraItsServerInfo.java b/src/main/java/com/googlesource/gerrit/plugins/its/jira/JiraItsServerInfo.java index 4ffb72d..5a92d84 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/its/jira/JiraItsServerInfo.java +++ b/src/main/java/com/googlesource/gerrit/plugins/its/jira/JiraItsServerInfo.java
@@ -15,25 +15,27 @@ package com.googlesource.gerrit.plugins.its.jira; +import static com.google.common.base.Preconditions.checkNotNull; + import com.googlesource.gerrit.plugins.its.jira.restapi.JiraURL; import com.googlesource.gerrit.plugins.its.jira.restapi.JiraVisibility; import com.googlesource.gerrit.plugins.its.jira.restapi.JiraVisibilityType; import java.net.MalformedURLException; +import java.time.Duration; public class JiraItsServerInfo { public static class Builder { private JiraItsServerInfo instance = new JiraItsServerInfo(); + private String projectUrl; + private Duration connectTimeout; + private Duration readTimeout; private Builder() {} public Builder url(String projectUrl) { - try { - instance.url = projectUrl != null ? new JiraURL(projectUrl) : null; - return this; - } catch (MalformedURLException e) { - throw new IllegalArgumentException("Unable to resolve URL", e); - } + this.projectUrl = projectUrl; + return this; } public Builder username(String username) { @@ -55,8 +57,26 @@ return this; } + public Builder connectTimeout(Duration timeout) { + this.connectTimeout = timeout; + return this; + } + + public Builder readTimeout(Duration timeout) { + this.readTimeout = timeout; + return this; + } + public JiraItsServerInfo build() { - return instance; + try { + checkNotNull(connectTimeout, "Missing connection timeout"); + checkNotNull(readTimeout, "Missing read timeout"); + instance.url = + projectUrl != null ? new JiraURL(projectUrl, connectTimeout, readTimeout) : null; + return instance; + } catch (MalformedURLException e) { + throw new IllegalArgumentException("Unable to resolve URL", e); + } } }
diff --git a/src/main/java/com/googlesource/gerrit/plugins/its/jira/JiraUrlProjectConfigEntry.java b/src/main/java/com/googlesource/gerrit/plugins/its/jira/JiraUrlProjectConfigEntry.java index 40c248c..247720e 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/its/jira/JiraUrlProjectConfigEntry.java +++ b/src/main/java/com/googlesource/gerrit/plugins/its/jira/JiraUrlProjectConfigEntry.java
@@ -37,7 +37,7 @@ public ConfigValue preUpdate(ConfigValue configValue) { if (configValue.value != null && !configValue.value.isEmpty()) { try { - new JiraURL(configValue.value); + JiraURL.validateUrl(configValue.value); } catch (MalformedURLException e) { configValue.value = INVALID_URL_MSG; }
diff --git a/src/main/java/com/googlesource/gerrit/plugins/its/jira/restapi/JiraRestApi.java b/src/main/java/com/googlesource/gerrit/plugins/its/jira/restapi/JiraRestApi.java index ceec13d..ef4c43f 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/its/jira/restapi/JiraRestApi.java +++ b/src/main/java/com/googlesource/gerrit/plugins/its/jira/restapi/JiraRestApi.java
@@ -70,7 +70,7 @@ * @throws IOException generated if unexpected failCode is returned */ public T doGet(String spec, int passCode, int[] failCodes) throws IOException { - HttpURLConnection conn = prepHttpConnection(spec, "GET", false); + HttpURLConnection conn = openConnection(spec, "GET", false); try { if (validateResponse(conn, passCode, failCodes)) { readIncomingData(conn); @@ -99,18 +99,8 @@ return sendPayload("PUT", spec, jsonInput, passCode); } - private boolean sendPayload(String method, String spec, String jsonInput, int passCode) - throws IOException { - HttpURLConnection conn = prepHttpConnection(spec, method, true); - try { - writeBodyData(jsonInput, conn); - return validateResponse(conn, passCode, null); - } finally { - conn.disconnect(); - } - } - - private HttpURLConnection prepHttpConnection(String spec, String method, boolean withPayload) + /** Open an HTTP connection with the Jira's endpoint URL. */ + public HttpURLConnection openConnection(String spec, String method, boolean withPayload) throws IOException { JiraURL url = baseUrl.withSpec(spec); ProxySelector proxySelector = ProxySelector.getDefault(); @@ -125,6 +115,17 @@ return conn; } + private boolean sendPayload(String method, String spec, String jsonInput, int passCode) + throws IOException { + HttpURLConnection conn = openConnection(spec, method, true); + try { + writeBodyData(jsonInput, conn); + return validateResponse(conn, passCode, null); + } finally { + conn.disconnect(); + } + } + /** Write the data to the HTTP connection. */ private void writeBodyData(String data, HttpURLConnection conn) throws IOException { if (data != null) {
diff --git a/src/main/java/com/googlesource/gerrit/plugins/its/jira/restapi/JiraURL.java b/src/main/java/com/googlesource/gerrit/plugins/its/jira/restapi/JiraURL.java index 0de96d8..6f31f39 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/its/jira/restapi/JiraURL.java +++ b/src/main/java/com/googlesource/gerrit/plugins/its/jira/restapi/JiraURL.java
@@ -23,6 +23,7 @@ import java.net.Proxy; import java.net.ProxySelector; import java.net.URL; +import java.time.Duration; import java.util.Objects; import org.eclipse.jgit.util.HttpSupport; import org.slf4j.Logger; @@ -36,18 +37,31 @@ private final URL url; - public JiraURL(String spec) throws MalformedURLException { - this.url = new URL(CharMatcher.is('/').trimFrom(spec) + "/"); + private final int connectionTimeoutMs; + + private final int readTimeoutMs; + + public static URL validateUrl(String spec) throws MalformedURLException { + return new URL(CharMatcher.is('/').trimFrom(spec) + "/"); } - private JiraURL(URL url) { + public JiraURL(String spec, Duration connectionTimeout, Duration readTimeout) + throws MalformedURLException { + this.url = validateUrl(spec); + this.connectionTimeoutMs = (int) connectionTimeout.toMillis(); + this.readTimeoutMs = (int) readTimeout.toMillis(); + } + + private JiraURL(URL url, int connectionTimeoutMs, int readTimeoutMs) { this.url = requireNonNull(url); + this.connectionTimeoutMs = connectionTimeoutMs; + this.readTimeoutMs = readTimeoutMs; } public JiraURL resolveUrl(String... paths) { String relativePath = String.join("", paths); try { - return new JiraURL(new URL(url, relativePath)); + return new JiraURL(new URL(url, relativePath), connectionTimeoutMs, readTimeoutMs); } catch (MalformedURLException e) { log.error("Unexpected exception while composing URL {} with path {}", url, relativePath, e); throw new IllegalArgumentException(e); @@ -63,12 +77,15 @@ } public JiraURL withSpec(String spec) throws MalformedURLException { - return new JiraURL(new URL(url, spec)); + return new JiraURL(new URL(url, spec), connectionTimeoutMs, readTimeoutMs); } public HttpURLConnection openConnection(ProxySelector proxySelector) throws IOException { Proxy proxy = HttpSupport.proxyFor(proxySelector, url); - return (HttpURLConnection) url.openConnection(proxy); + HttpURLConnection conn = (HttpURLConnection) url.openConnection(proxy); + conn.setConnectTimeout(connectionTimeoutMs); + conn.setReadTimeout(readTimeoutMs); + return conn; } public String getPath() {
diff --git a/src/main/resources/Documentation/config.md b/src/main/resources/Documentation/config.md index a86b80c..4827dfd 100644 --- a/src/main/resources/Documentation/config.md +++ b/src/main/resources/Documentation/config.md
@@ -101,6 +101,24 @@ Jira credentials and connectivity details are asked and verified during the Gerrit init. +Timeouts +-------- + +The HTTP connection and read timeouts are configurable in every place where the Jira URL is specified, +either in the `gerrit.config` or `project.config`, whichever has the precedence. + +`connectTimeout` +: Timeout period for making a new HTTP connection to the Jira URL. + The value is in the usual time-unit format like "1 s", "100 ms", etc. + By default `2 minutes`. + +`readTimeout` +: The read timeout for the HTTP operation to complete. + The value is in the usual time-unit format like "1 s", "100 ms", etc. + A timeout can be used to avoid blocking all of the incoming Git receive-packs threads in case + Jira server becomes slow. + By default `30 seconds`. + Gerrit init integration -----------------------
diff --git a/src/test/java/com/googlesource/gerrit/plugins/its/jira/restapi/JiraRestApiTest.java b/src/test/java/com/googlesource/gerrit/plugins/its/jira/restapi/JiraRestApiTest.java index d3b4812..27c5fc7 100644 --- a/src/test/java/com/googlesource/gerrit/plugins/its/jira/restapi/JiraRestApiTest.java +++ b/src/test/java/com/googlesource/gerrit/plugins/its/jira/restapi/JiraRestApiTest.java
@@ -22,6 +22,7 @@ import java.io.ByteArrayOutputStream; import java.net.HttpURLConnection; import java.net.MalformedURLException; +import java.time.Duration; import org.junit.Test; import org.junit.runner.RunWith; import org.mockito.junit.MockitoJUnitRunner; @@ -33,17 +34,20 @@ private static final String JSON_PAYLOAD = "{}"; private static final String USERNAME = "user"; private static final String PASSWORD = "pass"; + private static final Duration CONNECTION_TIMEOUT = Duration.ofSeconds(30); + private static final Duration READ_TIMEOUT = Duration.ofSeconds(30); private JiraURL url; private JiraRestApi restApi; - private void setURL(String jiraUrl) throws MalformedURLException { - url = new JiraURL(jiraUrl); + private void setURL(String jiraUrl, Duration connectionTimeout, Duration readTimeout) + throws MalformedURLException { + url = new JiraURL(jiraUrl, connectionTimeout, readTimeout); } @Test public void testJiraServerInfoForNonRootJiraUrl() throws Exception { - setURL("http://jira.mycompany.com/myroot/"); + setURL("http://jira.mycompany.com/myroot/", CONNECTION_TIMEOUT, READ_TIMEOUT); restApi = new JiraRestApi(url, USERNAME, PASSWORD, JiraIssue.class, ISSUE_CLASS_PREFIX); String jiraApiUrl = restApi.getBaseUrl().toString(); assertThat(jiraApiUrl).startsWith(url.toString()); @@ -51,7 +55,7 @@ @Test public void testJiraServerInfoForNonRootJiraUrlNotEndingWithSlash() throws Exception { - setURL("http://jira.mycompany.com/myroot"); + setURL("http://jira.mycompany.com/myroot", CONNECTION_TIMEOUT, READ_TIMEOUT); restApi = new JiraRestApi(url, USERNAME, PASSWORD, JiraIssue.class, ISSUE_CLASS_PREFIX); String jiraApiUrl = restApi.getBaseUrl().toString(); assertThat(jiraApiUrl).startsWith(url.toString()); @@ -59,13 +63,24 @@ @Test public void testJiraServerInfoForRootJiraUrl() throws Exception { - setURL("http://jira.mycompany.com/myroot"); + setURL("http://jira.mycompany.com/myroot", CONNECTION_TIMEOUT, READ_TIMEOUT); restApi = new JiraRestApi(url, USERNAME, PASSWORD, JiraIssue.class, ISSUE_CLASS_PREFIX); String jiraApiUrl = restApi.getBaseUrl().toString(); assertThat(jiraApiUrl).startsWith(url.toString()); } @Test + public void testJiraServerInfoWithTimeouts() throws Exception { + Duration connectionTimeout = Duration.ofSeconds(500L); + Duration readTimeout = Duration.ofMillis(10L); + setURL("http://jira.mycompany.com/myroot", connectionTimeout, readTimeout); + restApi = new JiraRestApi(url, USERNAME, PASSWORD, JiraIssue.class, ISSUE_CLASS_PREFIX); + HttpURLConnection httpConnection = restApi.openConnection("/", "GET", false); + assertThat(httpConnection.getConnectTimeout()).isEqualTo(connectionTimeout.toMillis()); + assertThat(httpConnection.getReadTimeout()).isEqualTo(readTimeout.toMillis()); + } + + @Test public void testDoPut() throws Exception { JiraURL url = mock(JiraURL.class); when(url.resolveUrl(any())).thenReturn(url);