Repository: aurora Updated Branches: refs/heads/master 4ead1893b -> 60e5e4e67
Fixing connection leak in webhook by making sure stream is closed. Last refactoring of Webhook did not correctly close out connections so some webhook requests would not complete. Testing Done: Verified in vagrant + added unit tests. Bugs closed: AURORA-1783 Reviewed at https://reviews.apache.org/r/52276/ Project: http://git-wip-us.apache.org/repos/asf/aurora/repo Commit: http://git-wip-us.apache.org/repos/asf/aurora/commit/60e5e4e6 Tree: http://git-wip-us.apache.org/repos/asf/aurora/tree/60e5e4e6 Diff: http://git-wip-us.apache.org/repos/asf/aurora/diff/60e5e4e6 Branch: refs/heads/master Commit: 60e5e4e67d6adbf775c6c2f22f44d3f61e66636b Parents: 4ead189 Author: Dmitriy Shirchenko <[email protected]> Authored: Mon Sep 26 14:55:58 2016 -0700 Committer: Zameer Manji <[email protected]> Committed: Mon Sep 26 14:55:58 2016 -0700 ---------------------------------------------------------------------- .../apache/aurora/scheduler/events/Webhook.java | 17 ++++++++++++----- .../aurora/scheduler/events/WebhookModule.java | 6 +++--- .../aurora/scheduler/events/WebhookTest.java | 18 ++++++++++++++---- 3 files changed, 29 insertions(+), 12 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/aurora/blob/60e5e4e6/src/main/java/org/apache/aurora/scheduler/events/Webhook.java ---------------------------------------------------------------------- diff --git a/src/main/java/org/apache/aurora/scheduler/events/Webhook.java b/src/main/java/org/apache/aurora/scheduler/events/Webhook.java index 66c1344..321cab3 100644 --- a/src/main/java/org/apache/aurora/scheduler/events/Webhook.java +++ b/src/main/java/org/apache/aurora/scheduler/events/Webhook.java @@ -23,9 +23,12 @@ import com.google.inject.Inject; import org.apache.aurora.scheduler.events.PubsubEvent.EventSubscriber; import org.apache.aurora.scheduler.events.PubsubEvent.TaskStateChange; -import org.apache.http.client.HttpClient; +import org.apache.http.HttpEntity; +import org.apache.http.client.methods.CloseableHttpResponse; import org.apache.http.client.methods.HttpPost; import org.apache.http.entity.StringEntity; +import org.apache.http.impl.client.CloseableHttpClient; +import org.apache.http.util.EntityUtils; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -37,10 +40,10 @@ public class Webhook implements EventSubscriber { private static final Logger LOG = LoggerFactory.getLogger(Webhook.class); private final WebhookInfo webhookInfo; - private final HttpClient httpClient; + private final CloseableHttpClient httpClient; @Inject - Webhook(HttpClient httpClient, WebhookInfo webhookInfo) { + Webhook(CloseableHttpClient httpClient, WebhookInfo webhookInfo) { this.webhookInfo = webhookInfo; this.httpClient = httpClient; LOG.info("Webhook enabled with info" + this.webhookInfo); @@ -73,8 +76,12 @@ public class Webhook implements EventSubscriber { if (stateChange.getOldState().isPresent()) { try { HttpPost post = createPostRequest(stateChange); - try { - httpClient.execute(post); + // Using try-with-resources on closeable and following + // https://hc.apache.org/httpcomponents-client-4.5.x/quickstart.html to make sure stream is + // closed after we get back a response to not leak http connections. + try (CloseableHttpResponse httpResponse = httpClient.execute(post)) { + HttpEntity entity = httpResponse.getEntity(); + EntityUtils.consumeQuietly(entity); } catch (IOException exp) { LOG.error("Error sending a Webhook event", exp); } http://git-wip-us.apache.org/repos/asf/aurora/blob/60e5e4e6/src/main/java/org/apache/aurora/scheduler/events/WebhookModule.java ---------------------------------------------------------------------- diff --git a/src/main/java/org/apache/aurora/scheduler/events/WebhookModule.java b/src/main/java/org/apache/aurora/scheduler/events/WebhookModule.java index 71aae98..05e19f4 100644 --- a/src/main/java/org/apache/aurora/scheduler/events/WebhookModule.java +++ b/src/main/java/org/apache/aurora/scheduler/events/WebhookModule.java @@ -30,9 +30,9 @@ import org.apache.aurora.common.args.Arg; import org.apache.aurora.common.args.CmdLine; import org.apache.aurora.common.args.constraints.CanRead; import org.apache.aurora.common.args.constraints.Exists; -import org.apache.http.client.HttpClient; import org.apache.http.client.config.RequestConfig; import org.apache.http.conn.ConnectionKeepAliveStrategy; +import org.apache.http.impl.client.CloseableHttpClient; import org.apache.http.impl.client.DefaultConnectionKeepAliveStrategy; import org.apache.http.impl.client.HttpClientBuilder; import org.codehaus.jackson.map.ObjectMapper; @@ -78,7 +78,7 @@ public class WebhookModule extends AbstractModule { .setSocketTimeout(timeout) // wait for data after connection was established. .build(); ConnectionKeepAliveStrategy connectionStrategy = new DefaultConnectionKeepAliveStrategy(); - HttpClient client = + CloseableHttpClient client = HttpClientBuilder.create() .setDefaultRequestConfig(config) // being explicit about using default Keep-Alive strategy. @@ -86,7 +86,7 @@ public class WebhookModule extends AbstractModule { .build(); bind(WebhookInfo.class).toInstance(webhookInfo); - bind(HttpClient.class).toInstance(client); + bind(CloseableHttpClient.class).toInstance(client); PubsubEventModule.bindSubscriber(binder(), Webhook.class); bind(Webhook.class).in(Singleton.class); } http://git-wip-us.apache.org/repos/asf/aurora/blob/60e5e4e6/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 6f37baa..e8335d9 100644 --- a/src/test/java/org/apache/aurora/scheduler/events/WebhookTest.java +++ b/src/test/java/org/apache/aurora/scheduler/events/WebhookTest.java @@ -26,8 +26,10 @@ import org.apache.aurora.scheduler.base.TaskTestUtil; import org.apache.aurora.scheduler.events.PubsubEvent.TaskStateChange; import org.apache.aurora.scheduler.storage.entities.IScheduledTask; import org.apache.http.Header; -import org.apache.http.client.HttpClient; +import org.apache.http.HttpEntity; +import org.apache.http.client.methods.CloseableHttpResponse; import org.apache.http.client.methods.HttpPost; +import org.apache.http.impl.client.CloseableHttpClient; import org.apache.http.util.EntityUtils; import org.easymock.Capture; import org.junit.Before; @@ -35,6 +37,7 @@ import org.junit.Test; import static org.easymock.EasyMock.capture; import static org.easymock.EasyMock.expect; +import static org.easymock.EasyMock.expectLastCall; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertTrue; @@ -47,13 +50,13 @@ public class WebhookTest extends EasyMockTest { .transition(TASK, ScheduleStatus.FAILED); private final String changeJson = changeWithOldState.toJson(); - private HttpClient httpClient; + private CloseableHttpClient httpClient; private Webhook webhook; @Before public void setUp() { WebhookInfo webhookInfo = WebhookModule.parseWebhookConfig(WebhookModule.readWebhookFile()); - httpClient = createMock(HttpClient.class); + httpClient = createMock(CloseableHttpClient.class); webhook = new Webhook(httpClient, webhookInfo); } @@ -67,8 +70,15 @@ public class WebhookTest extends EasyMockTest { @Test public void testTaskChangedWithOldState() throws Exception { + CloseableHttpResponse httpResponse = createMock(CloseableHttpResponse.class); + HttpEntity entity = createMock(HttpEntity.class); + Capture<HttpPost> httpPostCapture = createCapture(); - expect(httpClient.execute(capture(httpPostCapture))).andReturn(null); + expect(entity.isStreaming()).andReturn(false); + expect(httpResponse.getEntity()).andReturn(entity); + httpResponse.close(); + expectLastCall().once(); + expect(httpClient.execute(capture(httpPostCapture))).andReturn(httpResponse); control.replay();
