Reschedule unsuccessful requests instead of sleeping
Prior to this change the retry logic was implemented at the http client
level and it would simply sleep and retry in a loop. This was suboptimal
as it keeps a thread allocated all the time.
When a web hook can not be executed, reschedule the task to be executed
again after the configured retry interval. This is the same approach
as we have in the replication plugin.
This change also adds dependency to the mockito and provides tests
for the retry logic.
Change-Id: I53486cb8752990991763acf9dd83d075b2aa7273
diff --git a/BUCK b/BUCK
index aa5ea84..f78d670 100644
--- a/BUCK
+++ b/BUCK
@@ -7,6 +7,7 @@
TEST_DEPS = GERRIT_PLUGIN_API + GERRIT_TESTS + [
':webhooks__plugin',
+ ':mockito',
]
gerrit_plugin(
@@ -40,3 +41,30 @@
labels = ['webhooks'],
deps = TEST_DEPS,
)
+
+maven_jar(
+ name = 'mockito',
+ id = 'org.mockito:mockito-core:2.7.21',
+ sha1 = '23e9f7bfb9717e849a05b84c29ee3ac723f1a653',
+ license = 'DO_NOT_DISTRIBUTE',
+ deps = [
+ ':byte-buddy',
+ ':objenesis',
+ ],
+)
+
+maven_jar(
+ name = 'byte-buddy',
+ id = 'net.bytebuddy:byte-buddy:1.6.11',
+ sha1 = '8a8f9409e27f1d62c909c7eef2aa7b3a580b4901',
+ license = 'DO_NOT_DISTRIBUTE',
+ attach_source = False,
+)
+
+maven_jar(
+ name = 'objenesis',
+ id = 'org.objenesis:objenesis:2.5',
+ sha1 = '612ecb799912ccf77cba9b3ed8c813da086076e9',
+ license = 'DO_NOT_DISTRIBUTE',
+ attach_source = False,
+)
diff --git a/src/main/java/com/googlesource/gerrit/plugins/webhooks/HttpClientProvider.java b/src/main/java/com/googlesource/gerrit/plugins/webhooks/HttpClientProvider.java
index 047105e..d045fd5 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/webhooks/HttpClientProvider.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/webhooks/HttpClientProvider.java
@@ -14,18 +14,15 @@
package com.googlesource.gerrit.plugins.webhooks;
-import java.io.IOException;
import java.security.KeyManagementException;
import java.security.NoSuchAlgorithmException;
import java.security.cert.X509Certificate;
import javax.net.ssl.SSLContext;
-import javax.net.ssl.SSLException;
import javax.net.ssl.TrustManager;
import javax.net.ssl.X509TrustManager;
import org.apache.http.HttpResponse;
-import org.apache.http.client.HttpRequestRetryHandler;
import org.apache.http.client.ServiceUnavailableRetryStrategy;
import org.apache.http.client.config.RequestConfig;
import org.apache.http.config.Registry;
@@ -71,7 +68,6 @@
return HttpClients.custom().setSSLSocketFactory(sslSocketFactory)
.setConnectionManager(customConnectionManager())
.setDefaultRequestConfig(customRequestConfig())
- .setRetryHandler(customRetryHandler())
.setServiceUnavailableRetryStrategy(customServiceUnavailRetryStrategy())
.build();
}
@@ -83,28 +79,6 @@
.build();
}
- private HttpRequestRetryHandler customRetryHandler() {
- return new HttpRequestRetryHandler() {
-
- @Override
- public boolean retryRequest(IOException exception, int executionCount,
- HttpContext context) {
- if (executionCount > cfg.getMaxTries()
- || exception instanceof SSLException) {
- return false;
- }
- logRetry(exception.getMessage(), context);
- try {
- Thread.sleep(cfg.getRetryInterval());
- } catch (InterruptedException e) {
- Thread.currentThread().interrupt();
- return false;
- }
- return true;
- }
- };
- }
-
private ServiceUnavailableRetryStrategy customServiceUnavailRetryStrategy() {
return new ServiceUnavailableRetryStrategy() {
@Override
diff --git a/src/main/java/com/googlesource/gerrit/plugins/webhooks/Module.java b/src/main/java/com/googlesource/gerrit/plugins/webhooks/Module.java
index 128bfa5..accb0a2 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/webhooks/Module.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/webhooks/Module.java
@@ -14,7 +14,7 @@
package com.googlesource.gerrit.plugins.webhooks;
-import java.util.concurrent.Executor;
+import java.util.concurrent.ScheduledThreadPoolExecutor;
import org.apache.http.impl.client.CloseableHttpClient;
@@ -27,7 +27,7 @@
@Override
protected void configure() {
- bind(Executor.class)
+ bind(ScheduledThreadPoolExecutor.class)
.annotatedWith(WebHooksExecutor.class)
.toProvider(ExecutorProvider.class);
bind(Configuration.class).in(Scopes.SINGLETON);
diff --git a/src/main/java/com/googlesource/gerrit/plugins/webhooks/PostTask.java b/src/main/java/com/googlesource/gerrit/plugins/webhooks/PostTask.java
index 39e48d4..9311531 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/webhooks/PostTask.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/webhooks/PostTask.java
@@ -15,13 +15,17 @@
package com.googlesource.gerrit.plugins.webhooks;
import java.io.IOException;
-import java.util.concurrent.Executor;
+import java.util.concurrent.ScheduledThreadPoolExecutor;
+import java.util.concurrent.TimeUnit;
+
+import javax.net.ssl.SSLException;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import com.google.inject.assistedinject.Assisted;
import com.google.inject.assistedinject.AssistedInject;
+import com.googlesource.gerrit.plugins.webhooks.HttpResponseHandler.HttpResult;
class PostTask implements Runnable {
private static final Logger log = LoggerFactory
@@ -31,18 +35,22 @@
PostTask create(@Assisted("url") String url, @Assisted("body") String body);
}
- private final Executor executor;
+ private final ScheduledThreadPoolExecutor executor;
private final HttpSession session;
+ private final Configuration cfg;
private final String url;
private final String body;
+ private int execCnt;
@AssistedInject
- public PostTask(@WebHooksExecutor Executor executor,
+ public PostTask(@WebHooksExecutor ScheduledThreadPoolExecutor executor,
HttpSession session,
+ Configuration cfg,
@Assisted("url") String url,
@Assisted("body") String body) {
this.executor = executor;
this.session = session;
+ this.cfg = cfg;
this.url = url;
this.body = body;
}
@@ -51,12 +59,42 @@
executor.execute(this);
}
+ private void reschedule() {
+ executor.schedule(this, cfg.getRetryInterval(), TimeUnit.MILLISECONDS);
+ }
+
@Override
public void run() {
try {
- session.post(url, body);
+ execCnt++;
+ HttpResult result = session.post(url, body);
+ if (!result.successful && execCnt < cfg.getMaxTries()) {
+ logRetry(result.message);
+ reschedule();
+ }
} catch (IOException e) {
- log.error("Couldn't post {}", body, e);
+ if (isRecoverable(e) && execCnt < cfg.getMaxTries()) {
+ logRetry(e);
+ reschedule();
+ } else {
+ log.error("Failed to post: {}", body, e);
+ }
+ }
+ }
+
+ private boolean isRecoverable(IOException e) {
+ return !(e instanceof SSLException);
+ }
+
+ private void logRetry(String reason) {
+ if (log.isDebugEnabled()) {
+ log.debug("Retrying {} in {}ms. Reason: {}", toString(), cfg.getRetryInterval(), reason);
+ }
+ }
+
+ private void logRetry(Throwable cause) {
+ if (log.isDebugEnabled()) {
+ log.debug("Retrying {} in {}ms. Cause: {}", toString(), cfg.getRetryInterval(), cause);
}
}
diff --git a/src/test/java/com/googlesource/gerrit/plugins/webhooks/PostTaskTest.java b/src/test/java/com/googlesource/gerrit/plugins/webhooks/PostTaskTest.java
new file mode 100644
index 0000000..185e786
--- /dev/null
+++ b/src/test/java/com/googlesource/gerrit/plugins/webhooks/PostTaskTest.java
@@ -0,0 +1,109 @@
+// Copyright (C) 2017 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.googlesource.gerrit.plugins.webhooks;
+
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.verifyZeroInteractions;
+import static org.mockito.Mockito.when;
+
+import java.io.IOException;
+import java.util.concurrent.ScheduledThreadPoolExecutor;
+import java.util.concurrent.TimeUnit;
+
+import javax.net.ssl.SSLException;
+
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.Mock;
+import org.mockito.invocation.InvocationOnMock;
+import org.mockito.junit.MockitoJUnitRunner;
+import org.mockito.stubbing.Answer;
+
+import com.googlesource.gerrit.plugins.webhooks.HttpResponseHandler.HttpResult;
+
+@RunWith(MockitoJUnitRunner.class)
+public class PostTaskTest {
+
+ private static final String WEBHOOK_URL = "webhook-url";
+ private static final String BODY = "body";
+ private static final HttpResult OK_RESULT = new HttpResult(true, "");
+ private static final HttpResult ERR_RESULT = new HttpResult(false, "");
+ private static final int RETRY_INTERVAL = 100;
+ private static final int MAX_TRIES = 3;
+
+ @Mock
+ private Configuration cfg;
+
+ @Mock
+ private HttpSession session;
+
+ @Mock
+ private ScheduledThreadPoolExecutor executor;
+
+ private PostTask task;
+
+ @Before
+ public void setup() {
+ when(cfg.getRetryInterval()).thenReturn(RETRY_INTERVAL);
+ when(cfg.getMaxTries()).thenReturn(MAX_TRIES);
+ task = new PostTask(
+ executor, session, cfg, WEBHOOK_URL, BODY);
+ }
+
+ @Test
+ public void noRescheduleOnSuccess() throws IOException {
+ when(session.post(WEBHOOK_URL, BODY)).thenReturn(OK_RESULT);
+ task.run();
+ verifyZeroInteractions(executor);
+ }
+
+ @Test
+ public void noRescheduleOnNonRecoverableException() throws IOException {
+ when(session.post(WEBHOOK_URL, BODY)).thenThrow(SSLException.class);
+ task.run();
+ verifyZeroInteractions(executor);
+ }
+
+ @Test
+ public void rescheduleOnError() throws IOException {
+ when(session.post(WEBHOOK_URL, BODY)).thenReturn(ERR_RESULT);
+ task.run();
+ verify(executor, times(1)).schedule(task, RETRY_INTERVAL, TimeUnit.MILLISECONDS);
+ }
+
+ @Test
+ public void rescheduleOnRecoverableException() throws IOException {
+ when(session.post(WEBHOOK_URL, BODY)).thenThrow(IOException.class);
+ task.run();
+ verify(executor, times(1)).schedule(task, RETRY_INTERVAL, TimeUnit.MILLISECONDS);
+ }
+
+ @Test
+ public void keepReschedulingMaxTriesTimes() throws IOException {
+ when(session.post(WEBHOOK_URL, BODY)).thenThrow(IOException.class);
+ when(executor.schedule(task, RETRY_INTERVAL, TimeUnit.MILLISECONDS))
+ .then(new Answer<Void>() {
+ @Override
+ public Void answer(InvocationOnMock invocation) throws Throwable {
+ task.run();
+ return null;
+ }
+ });
+ task.run();
+ verify(executor, times(MAX_TRIES - 1)).schedule(task, RETRY_INTERVAL, TimeUnit.MILLISECONDS);
+ }
+}
