Repository: aurora Updated Branches: refs/heads/master 301f06636 -> 65c2288e0
Attempt #2 to fix flaky Webhook test Along the same lines of Stephan's change (https://reviews.apache.org/r/64482/), but waiting for `onThrowable` to finish since it is called asyncronously to the future being completed. Overall, this flakiness has seemed to increase in volume over the past month. I've been running into it with a noticable fraction of internal/test builds. Bugs closed: AURORA-1961 Reviewed at https://reviews.apache.org/r/64523/ Project: http://git-wip-us.apache.org/repos/asf/aurora/repo Commit: http://git-wip-us.apache.org/repos/asf/aurora/commit/65c2288e Tree: http://git-wip-us.apache.org/repos/asf/aurora/tree/65c2288e Diff: http://git-wip-us.apache.org/repos/asf/aurora/diff/65c2288e Branch: refs/heads/master Commit: 65c2288e0c99ce60eb586dc9ef64f87247d1ba5c Parents: 301f066 Author: Jordan Ly <jordan....@gmail.com> Authored: Tue Dec 12 23:28:59 2017 +0100 Committer: Stephan Erb <s...@apache.org> Committed: Tue Dec 12 23:28:59 2017 +0100 ---------------------------------------------------------------------- .../aurora/scheduler/events/WebhookTest.java | 77 ++++++++++++++++++-- 1 file changed, 70 insertions(+), 7 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/aurora/blob/65c2288e/src/test/java/org/apache/aurora/scheduler/events/WebhookTest.java ---------------------------------------------------------------------- diff --git a/src/test/java/org/apache/aurora/scheduler/events/WebhookTest.java b/src/test/java/org/apache/aurora/scheduler/events/WebhookTest.java index 1b5d2d0..3e10c57 100644 --- a/src/test/java/org/apache/aurora/scheduler/events/WebhookTest.java +++ b/src/test/java/org/apache/aurora/scheduler/events/WebhookTest.java @@ -17,7 +17,9 @@ import java.io.IOException; import java.net.URI; import java.util.List; import java.util.Map; +import java.util.concurrent.CountDownLatch; import java.util.concurrent.ExecutionException; +import java.util.concurrent.TimeUnit; import java.util.stream.Collectors; import javax.servlet.ServletException; @@ -36,6 +38,9 @@ import org.asynchttpclient.AsyncHandler; import org.asynchttpclient.AsyncHttpClient; import org.asynchttpclient.DefaultAsyncHttpClient; import org.asynchttpclient.DefaultAsyncHttpClientConfig; +import org.asynchttpclient.HttpResponseBodyPart; +import org.asynchttpclient.HttpResponseHeaders; +import org.asynchttpclient.HttpResponseStatus; import org.asynchttpclient.ListenableFuture; import org.asynchttpclient.channel.DefaultKeepAliveStrategy; import org.eclipse.jetty.server.Request; @@ -93,6 +98,59 @@ public class WebhookTest { private FakeStatsProvider statsProvider; /** + * Wraps an {@link AsyncHandler} to allow the caller to wait for + * {@link AsyncHandler#onThrowable} to complete. This is necessary because exceptions cause the + * future to complete before {@code onThrowable} can finish. See AURORA-1961 for context. + */ + static class WebhookOnThrowableHandler<T> implements AsyncHandler<T> { + private final AsyncHandler<T> handler; + private final CountDownLatch latch = new CountDownLatch(1); + + private boolean onThrowableFinished = false; + + WebhookOnThrowableHandler(AsyncHandler<T> handler) { + this.handler = handler; + } + + boolean hasOnThrowableFinished(long timeout, TimeUnit unit) { + try { + latch.await(timeout, unit); + } catch (InterruptedException e) { + // No-op + } + + return onThrowableFinished; + } + + @Override + public void onThrowable(Throwable t) { + handler.onThrowable(t); + onThrowableFinished = true; + latch.countDown(); + } + + @Override + public State onBodyPartReceived(HttpResponseBodyPart bodyPart) throws Exception { + return handler.onBodyPartReceived(bodyPart); + } + + @Override + public State onStatusReceived(HttpResponseStatus responseStatus) throws Exception { + return handler.onStatusReceived(responseStatus); + } + + @Override + public State onHeadersReceived(HttpResponseHeaders headers) throws Exception { + return handler.onHeadersReceived(headers); + } + + @Override + public T onCompleted() throws Exception { + return handler.onCompleted(); + } + } + + /** * Wrap the DefaultAsyncHttpClient for testing so we can complete futures synchronously before * validating assertions. Otherwise, we would have to call `Thread.sleep` in our tests after * each TaskStateChange. @@ -106,12 +164,15 @@ public class WebhookTest { @Override public <T> ListenableFuture<T> executeRequest(org.asynchttpclient.Request request, AsyncHandler<T> handler) { - ListenableFuture<T> future = super.executeRequest(request, handler); + + WebhookOnThrowableHandler<T> wrapped = new WebhookOnThrowableHandler<>(handler); + ListenableFuture<T> future = super.executeRequest(request, wrapped); try { future.get(); future.done(); } catch (InterruptedException | ExecutionException e) { - // Ignore, future should not fail to resolve. + // The future threw an exception, wait up to 60 seconds for onThrowable to complete. + wrapped.hasOnThrowableFinished(60, TimeUnit.SECONDS); } return future; } @@ -180,12 +241,10 @@ public class WebhookTest { @Test public void testTaskChangedWithOldStateError() throws Exception { - jettyServer.start(); - WebhookInfo webhookInfo = buildWebhookInfoWithJettyPort(WEBHOOK_INFO_BUILDER); + // Don't start Jetty server, send the request to an invalid port to force a ConnectError + WebhookInfo webhookInfo = buildWebhookInfoWithJettyPort(WEBHOOK_INFO_BUILDER, -1); Webhook webhook = new Webhook(httpClient, webhookInfo, statsProvider); - // Send the event to a stopped server to trigger `onThrowable` of the client. - jettyServer.stop(); webhook.taskChangedState(CHANGE_OLD_STATE); assertEquals(1, statsProvider.getLongValue(Webhook.ATTEMPTS_STAT_NAME)); @@ -358,7 +417,11 @@ public class WebhookTest { * should have been started before this method is called. */ private WebhookInfo buildWebhookInfoWithJettyPort(WebhookInfoBuilder builder) { - String fullUrl = String.format("http://localhost:%d", jettyServer.getURI().getPort()); + return buildWebhookInfoWithJettyPort(builder, jettyServer.getURI().getPort()); + } + + private WebhookInfo buildWebhookInfoWithJettyPort(WebhookInfoBuilder builder, int port) { + String fullUrl = String.format("http://localhost:%d", port); return builder .setTargetURL(fullUrl) .build();