Handle RuntimeException during webhooks notification
Issue:
When RuntimeException is being thrown during webhooks post it results in
IllegalMonitorStateException [1] (reproduced only once :/) being thrown
from underlying executor leaving it completely broken. As a result
further notifications will not be neither processed nor posted.
Solution:
Catch Throwable and handle it gently.
[1]
2018-03-19 17:40:08,646|ERROR|webhooks-1|WorkQueue.java|81| WorkQueue thread webhooks-1 threw exception
java.lang.IllegalMonitorStateException
at java.base/java.util.concurrent.locks.ReentrantLock$Sync.tryRelease(Unknown Source)
at java.base/java.util.concurrent.locks.AbstractQueuedSynchronizer.release(Unknown Source)
at java.base/java.util.concurrent.locks.ReentrantLock.unlock(Unknown Source)
at java.base/java.util.concurrent.ScheduledThreadPoolExecutor$DelayedWorkQueue.take(Unknown Source)
at java.base/java.util.concurrent.ScheduledThreadPoolExecutor$DelayedWorkQueue.take(Unknown Source)
at java.base/java.util.concurrent.ThreadPoolExecutor.getTask(Unknown Source)
at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(Unknown Source)
at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(Unknown Source)
at java.base/java.lang.Thread.run(Unknown Source)
Change-Id: I4ebda02295c0261c746a1b7b74a2f0bd7978da6c
Signed-off-by: Jacek Centkowski <jcentkowski@collab.net>
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 f86703a..9a52f55 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/webhooks/PostTask.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/webhooks/PostTask.java
@@ -79,7 +79,7 @@
logRetry(result.message);
reschedule();
}
- } catch (IOException e) {
+ } catch (Throwable e) {
if (isRecoverable(e) && execCnt < remote.getMaxTries()) {
logRetry(e);
reschedule();
@@ -89,8 +89,8 @@
}
}
- private boolean isRecoverable(IOException e) {
- return !(e instanceof SSLException);
+ private boolean isRecoverable(Throwable e) {
+ return (e instanceof IOException) && !(e instanceof SSLException);
}
private void logRetry(String reason) {
diff --git a/src/test/java/com/googlesource/gerrit/plugins/webhooks/PostTaskTest.java b/src/test/java/com/googlesource/gerrit/plugins/webhooks/PostTaskTest.java
index 623fbb0..9e7d5f8 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/webhooks/PostTaskTest.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/webhooks/PostTaskTest.java
@@ -24,6 +24,7 @@
import com.googlesource.gerrit.plugins.webhooks.HttpResponseHandler.HttpResult;
import java.io.IOException;
import java.util.Optional;
+import java.util.concurrent.ExecutionException;
import java.util.concurrent.ScheduledThreadPoolExecutor;
import java.util.concurrent.TimeUnit;
import javax.net.ssl.SSLException;
@@ -123,4 +124,24 @@
task.run();
verify(executor, times(MAX_TRIES - 1)).schedule(task, RETRY_INTERVAL, TimeUnit.MILLISECONDS);
}
+
+ @Test
+ public void executorSurvivesNonRecoverableExceptions()
+ throws IOException, InterruptedException, ExecutionException {
+ executor = new ScheduledThreadPoolExecutor(1);
+
+ // schedule erroneous task for the first time
+ when(session.post(eq(remote), eq(content))).thenThrow(RuntimeException.class);
+ executor.schedule(task, 0L, TimeUnit.SECONDS).get();
+
+ // schedule erroneous task again (with another non-recoverable exception)
+ when(session.post(eq(remote), eq(content))).thenThrow(SSLException.class);
+ executor.schedule(task, 0L, TimeUnit.SECONDS).get();
+
+ // schedule task that finishes with success
+ when(session.post(eq(remote), eq(content))).thenReturn(OK_RESULT);
+ executor.schedule(task, 0L, TimeUnit.SECONDS).get();
+
+ verify(session, times(3)).post(eq(remote), eq(content));
+ }
}
