You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@aurora.apache.org by zm...@apache.org on 2016/09/26 21:56:13 UTC
aurora git commit: Fixing connection leak in webhook by making sure
stream is closed.
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 <ca...@gmail.com>
Authored: Mon Sep 26 14:55:58 2016 -0700
Committer: Zameer Manji <zm...@apache.org>
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();