Skip to content

Commit e874b53

Browse files
shs96cshs96c
committed
Review places where we create an HttpClient
This is a source of us leaking threads, since the underlying async httpclient needs to be released at some point.
1 parent c800a7b commit e874b53

File tree

16 files changed

+108
-119
lines changed

16 files changed

+108
-119
lines changed

‎java/client/src/org/openqa/selenium/remote/RemoteWebDriverBuilder.java

+1-1
Original file line numberDiff line numberDiff line change
@@ -111,7 +111,7 @@ public class RemoteWebDriverBuilder {
111111
if (("/session/" + id).equals(req.getUri())) {
112112
try {
113113
client.close();
114-
} catch (IOException e) {
114+
} catch (UncheckedIOException e) {
115115
LOG.log(WARNING, "Swallowing exception while closing http client", e);
116116
}
117117
factory.cleanupIdleClients();

‎java/client/src/org/openqa/selenium/remote/http/HttpClient.java

+3-4
Original file line numberDiff line numberDiff line change
@@ -17,8 +17,9 @@
1717

1818
package org.openqa.selenium.remote.http;
1919

20+
import org.openqa.selenium.internal.Require;
21+
2022
import java.io.Closeable;
21-
import java.io.IOException;
2223
import java.net.URL;
2324
import java.util.ServiceLoader;
2425
import java.util.Set;
@@ -27,16 +28,14 @@
2728

2829
import static org.openqa.selenium.remote.http.ClientConfig.defaultConfig;
2930

30-
import org.openqa.selenium.internal.Require;
31-
3231
/**
3332
* Defines a simple client for making HTTP requests.
3433
*/
3534
public interface HttpClient extends Closeable, HttpHandler {
3635

3736
WebSocket openSocket(HttpRequest request, WebSocket.Listener listener);
3837

39-
default void close() throws IOException {}
38+
default void close() {}
4039

4140
interface Factory {
4241

‎java/client/src/org/openqa/selenium/remote/http/netty/NettyClient.java

+7-2
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,7 @@
3636
import org.openqa.selenium.remote.http.WebSocket;
3737

3838
import java.io.IOException;
39+
import java.io.UncheckedIOException;
3940
import java.util.concurrent.ThreadFactory;
4041
import java.util.concurrent.TimeUnit;
4142
import java.util.function.BiFunction;
@@ -110,8 +111,12 @@ public HttpClient with(Filter filter) {
110111
}
111112

112113
@Override
113-
public void close() throws IOException {
114-
client.close();
114+
public void close() {
115+
try {
116+
client.close();
117+
} catch (IOException e) {
118+
throw new UncheckedIOException(e);
119+
}
115120
}
116121

117122
@AutoService(HttpClient.Factory.class)

‎java/client/src/org/openqa/selenium/remote/tracing/TracedHttpClient.java

+6-7
Original file line numberDiff line numberDiff line change
@@ -17,21 +17,20 @@
1717

1818
package org.openqa.selenium.remote.tracing;
1919

20-
import static org.openqa.selenium.remote.tracing.Tags.HTTP_REQUEST;
21-
import static org.openqa.selenium.remote.tracing.Tags.HTTP_RESPONSE;
22-
import static org.openqa.selenium.remote.tracing.HttpTracing.newSpanAsChildOf;
23-
import static org.openqa.selenium.remote.tracing.Tags.KIND;
24-
2520
import org.openqa.selenium.internal.Require;
2621
import org.openqa.selenium.remote.http.ClientConfig;
2722
import org.openqa.selenium.remote.http.HttpClient;
2823
import org.openqa.selenium.remote.http.HttpRequest;
2924
import org.openqa.selenium.remote.http.HttpResponse;
3025
import org.openqa.selenium.remote.http.WebSocket;
3126

32-
import java.io.IOException;
3327
import java.net.URL;
3428

29+
import static org.openqa.selenium.remote.tracing.HttpTracing.newSpanAsChildOf;
30+
import static org.openqa.selenium.remote.tracing.Tags.HTTP_REQUEST;
31+
import static org.openqa.selenium.remote.tracing.Tags.HTTP_RESPONSE;
32+
import static org.openqa.selenium.remote.tracing.Tags.KIND;
33+
3534
public class TracedHttpClient implements HttpClient {
3635

3736
private final Tracer tracer;
@@ -60,7 +59,7 @@ public HttpResponse execute(HttpRequest req) {
6059
}
6160

6261
@Override
63-
public void close() throws IOException {
62+
public void close() {
6463
delegate.close();
6564
}
6665

‎java/client/test/org/openqa/selenium/environment/webserver/NettyAppServer.java

+3-7
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@
2929
import org.openqa.selenium.json.Json;
3030
import org.openqa.selenium.net.PortProber;
3131
import org.openqa.selenium.netty.server.NettyServer;
32+
import org.openqa.selenium.remote.http.Contents;
3233
import org.openqa.selenium.remote.http.HttpClient;
3334
import org.openqa.selenium.remote.http.HttpHandler;
3435
import org.openqa.selenium.remote.http.HttpMethod;
@@ -163,15 +164,10 @@ private String createUrl(Server<?> server, String protocol, String hostName, Str
163164

164165
@Override
165166
public String create(Page page) {
166-
try {
167-
byte[] data = new Json()
168-
.toJson(ImmutableMap.of("content", page.toString()))
169-
.getBytes(UTF_8);
170-
171-
HttpClient client = HttpClient.Factory.createDefault().createClient(new URL(whereIs("/")));
167+
try (HttpClient client = HttpClient.Factory.createDefault().createClient(new URL(whereIs("/")))) {
172168
HttpRequest request = new HttpRequest(HttpMethod.POST, "/common/createPage");
173169
request.setHeader(CONTENT_TYPE, JSON_UTF_8);
174-
request.setContent(bytes(data));
170+
request.setContent(Contents.asJson(ImmutableMap.of("content", page.toString())));
175171
HttpResponse response = client.execute(request);
176172
return string(response);
177173
} catch (IOException ex) {

‎java/client/test/org/openqa/selenium/remote/internal/HttpClientTestBase.java

+10-9
Original file line numberDiff line numberDiff line change
@@ -157,16 +157,16 @@ public void shouldAllowUrlsWithSchemesToBeUsed() throws Exception {
157157
delegate = req -> new HttpResponse().setContent(Contents.utf8String("Hello, World!"));
158158

159159
// This is a terrible choice of URL
160-
HttpClient client = createFactory().createClient(new URL("http://example.com"));
160+
try (HttpClient client = createFactory().createClient(new URL("http://example.com"))) {
161161

162-
URI uri = URI.create(server.whereIs("/"));
163-
HttpRequest request = new HttpRequest(
164-
GET,
165-
String.format("http://%s:%s/hello", uri.getHost(), uri.getPort()));
162+
URI uri = URI.create(server.whereIs("/"));
163+
HttpRequest request =
164+
new HttpRequest(GET, String.format("http://%s:%s/hello", uri.getHost(), uri.getPort()));
166165

167-
HttpResponse response = client.execute(request);
166+
HttpResponse response = client.execute(request);
168167

169-
assertThat(string(response)).isEqualTo("Hello, World!");
168+
assertThat(string(response)).isEqualTo("Hello, World!");
169+
}
170170
}
171171

172172
@Test
@@ -225,8 +225,9 @@ private HttpResponse getQueryParameterResponse(HttpRequest request) {
225225

226226
private HttpResponse executeWithinServer(HttpRequest request, HttpHandler handler) {
227227
delegate = handler;
228-
HttpClient client = createFactory().createClient(fromUri(URI.create(server.whereIs("/"))));
229-
return client.execute(request);
228+
try (HttpClient client = createFactory().createClient(fromUri(URI.create(server.whereIs("/"))))) {
229+
return client.execute(request);
230+
}
230231
}
231232

232233
private HttpResponse executeWithinServer(HttpRequest request, HttpHandler handler, ClientConfig config) {

‎java/client/test/org/openqa/selenium/testing/drivers/GridSupplier.java

+13-12
Original file line numberDiff line numberDiff line change
@@ -84,18 +84,19 @@ private synchronized void startServers() {
8484
}
8585

8686
// Keep polling the status page of the hub until it claims to be ready
87-
HttpClient client = HttpClient.Factory.createDefault().createClient(hub.getWebDriverUrl());
88-
Json json = new Json();
89-
Wait<HttpClient> wait = new FluentWait<>(client)
90-
.ignoring(RuntimeException.class)
91-
.withTimeout(Duration.ofSeconds(30));
92-
wait.until(c -> {
93-
HttpRequest req = new HttpRequest(HttpMethod.GET, "/status");
94-
HttpResponse response = c.execute(req);
95-
Map<?, ?> value = json.toType(string(response), Map.class);
96-
97-
return Boolean.TRUE.equals(((Map<?, ?>) value.get("value")).get("ready"));
98-
});
87+
try (HttpClient client = HttpClient.Factory.createDefault().createClient(hub.getWebDriverUrl())) {
88+
Json json = new Json();
89+
Wait<HttpClient> wait = new FluentWait<>(client)
90+
.ignoring(RuntimeException.class)
91+
.withTimeout(Duration.ofSeconds(30));
92+
wait.until(c -> {
93+
HttpRequest req = new HttpRequest(HttpMethod.GET, "/status");
94+
HttpResponse response = c.execute(req);
95+
Map<?, ?> value = json.toType(string(response), Map.class);
96+
97+
return Boolean.TRUE.equals(((Map<?, ?>) value.get("value")).get("ready"));
98+
});
99+
}
99100

100101
started = true;
101102
}

‎java/server/src/org/openqa/selenium/grid/docker/DockerSessionFactory.java

+2-2
Original file line numberDiff line numberDiff line change
@@ -126,9 +126,9 @@ public Optional<ActiveSession> apply(CreateSessionRequest sessionRequest) {
126126
LOG.info("Starting session for " + sessionRequest.getCapabilities());
127127
int port = PortProber.findFreePort();
128128
URL remoteAddress = getUrl(port);
129-
HttpClient client = clientFactory.createClient(remoteAddress);
130129

131-
try (Span span = tracer.getCurrentContext().createSpan("docker_session_factory.apply")) {
130+
try (HttpClient client = clientFactory.createClient(remoteAddress);
131+
Span span = tracer.getCurrentContext().createSpan("docker_session_factory.apply")) {
132132
Map<String, EventAttributeValue> attributeMap = new HashMap<>();
133133
attributeMap.put(AttributeKey.LOGGER_CLASS.getKey(),
134134
EventAttribute.setValue(this.getClass().getName()));

‎java/server/src/org/openqa/selenium/grid/node/config/DriverServiceSessionFactory.java

+1-5
Original file line numberDiff line numberDiff line change
@@ -154,11 +154,7 @@ public Optional<ActiveSession> apply(CreateSessionRequest sessionRequest) {
154154
@Override
155155
public void stop() {
156156
service.stop();
157-
try {
158-
client.close();
159-
} catch (IOException e) {
160-
throw new UncheckedIOException(e);
161-
}
157+
client.close();
162158
}
163159
});
164160
} catch (Exception e) {

‎java/server/src/org/openqa/selenium/grid/router/GridStatusHandler.java

+1-2
Original file line numberDiff line numberDiff line change
@@ -160,8 +160,7 @@ public HttpResponse execute(HttpRequest req) {
160160

161161
Future<?> future = EXECUTOR_SERVICE.submit(
162162
() -> {
163-
try {
164-
HttpClient client = clientFactory.createClient(node.getUri().toURL());
163+
try (HttpClient client = clientFactory.createClient(node.getUri().toURL())) {
165164
HttpRequest nodeStatusReq = new HttpRequest(GET, "/se/grid/node/status");
166165
HttpTracing.inject(tracer, span, nodeStatusReq);
167166
HttpResponse res = client.execute(nodeStatusReq);

‎java/server/src/org/openqa/selenium/grid/router/HandleSession.java

+1-9
Original file line numberDiff line numberDiff line change
@@ -74,15 +74,7 @@ class HandleSession implements HttpHandler {
7474

7575
this.httpClients = CacheBuilder.newBuilder()
7676
.expireAfterAccess(Duration.ofMinutes(1))
77-
.removalListener(
78-
(RemovalListener<URL, HttpClient>) removal -> {
79-
try {
80-
removal.getValue().close();
81-
} catch (IOException e) {
82-
throw new UncheckedIOException(e);
83-
}
84-
}
85-
)
77+
.removalListener((RemovalListener<URL, HttpClient>) removal -> removal.getValue().close())
8678
.build();
8779

8880
new Regularly("Clean up http clients cache").submit(

‎java/server/test/org/openqa/selenium/grid/gridui/OverallGridTest.java

+10-9
Original file line numberDiff line numberDiff line change
@@ -122,15 +122,16 @@ private Server<?> createStandalone() {
122122
}
123123

124124
private void waitUntilReady(Server<?> server) {
125-
HttpClient client = HttpClient.Factory.createDefault().createClient(server.getUrl());
126-
127-
new FluentWait<>(client)
128-
.withTimeout(Duration.ofSeconds(5))
129-
.until(c -> {
130-
HttpResponse response = c.execute(new HttpRequest(GET, "/status"));
131-
Map<String, Object> status = Values.get(response, MAP_TYPE);
132-
return status != null && Boolean.TRUE.equals(status.get("ready"));
133-
});
125+
try (HttpClient client = HttpClient.Factory.createDefault().createClient(server.getUrl())) {
126+
new FluentWait<>(client)
127+
.withTimeout(Duration.ofSeconds(5))
128+
.until(
129+
c -> {
130+
HttpResponse response = c.execute(new HttpRequest(GET, "/status"));
131+
Map<String, Object> status = Values.get(response, MAP_TYPE);
132+
return status != null && Boolean.TRUE.equals(status.get("ready"));
133+
});
134+
}
134135
}
135136

136137
}

‎java/server/test/org/openqa/selenium/grid/router/EndToEndTest.java

+18-20
Original file line numberDiff line numberDiff line change
@@ -126,20 +126,19 @@ public void stopServers() {
126126
}
127127

128128
private static void waitUntilReady(Server<?> server, Duration duration) {
129-
HttpClient client = HttpClient.Factory.createDefault().createClient(server.getUrl());
130-
131-
new FluentWait<>(client)
132-
.withTimeout(duration)
133-
.pollingEvery(Duration.ofSeconds(1))
134-
.until(c -> {
135-
HttpResponse response = c.execute(new HttpRequest(GET, "/status"));
136-
System.out.println(Contents.string(response));
137-
Map<String, Object> status = Values.get(response, MAP_TYPE);
138-
return Boolean.TRUE.equals(status != null &&
139-
Boolean.parseBoolean(status.get("ready").toString()));
140-
});
141-
142-
Safely.safelyCall(client::close);
129+
try (HttpClient client = HttpClient.Factory.createDefault().createClient(server.getUrl())) {
130+
new FluentWait<>(client)
131+
.withTimeout(duration)
132+
.pollingEvery(Duration.ofSeconds(1))
133+
.until(
134+
c -> {
135+
HttpResponse response = c.execute(new HttpRequest(GET, "/status"));
136+
System.out.println(Contents.string(response));
137+
Map<String, Object> status = Values.get(response, MAP_TYPE);
138+
return Boolean.TRUE.equals(
139+
status != null && Boolean.parseBoolean(status.get("ready").toString()));
140+
});
141+
}
143142
}
144143

145144
// Hahahaha. Java naming.
@@ -285,13 +284,12 @@ public void shouldDoProtocolTranslationFromJWPLocalEndToW3CRemoteEnd() {
285284

286285
@Test
287286
public void responseShouldHaveContentTypeAndCacheControlHeaders() {
288-
HttpClient client = HttpClient.Factory.createDefault().createClient(server.getUrl());
287+
try (HttpClient client = HttpClient.Factory.createDefault().createClient(server.getUrl())) {
289288

290-
HttpResponse response = client.execute(new HttpRequest(GET, "/status"));
289+
HttpResponse response = client.execute(new HttpRequest(GET, "/status"));
291290

292-
assertThat(response.getHeader("Content-Type"))
293-
.isEqualTo("application/json; charset=utf-8");
294-
assertThat(response.getHeader("Cache-Control"))
295-
.isEqualTo("no-cache");
291+
assertThat(response.getHeader("Content-Type")).isEqualTo("application/json; charset=utf-8");
292+
assertThat(response.getHeader("Cache-Control")).isEqualTo("no-cache");
293+
}
296294
}
297295
}

‎java/server/test/org/openqa/selenium/grid/router/NewSessionCreationTest.java

+26-25
Original file line numberDiff line numberDiff line change
@@ -136,28 +136,28 @@ public void ensureJsCannotCreateANewSession() throws URISyntaxException {
136136
.build();
137137
distributor.add(node);
138138

139-
HttpClient client = HttpClient.Factory.createDefault().createClient(server.getUrl());
140-
141-
// Attempt to create a session with an origin header but content type set
142-
HttpResponse res = client.execute(
143-
new HttpRequest(POST, "/session")
144-
.addHeader("Content-Type", JSON_UTF_8)
145-
.addHeader("Origin", "localhost")
146-
.setContent(Contents.asJson(ImmutableMap.of(
147-
"capabilities", ImmutableMap.of(
148-
"alwaysMatch", Browser.detect().getCapabilities())))));
149-
150-
assertThat(res.getStatus()).isEqualTo(HTTP_INTERNAL_ERROR);
151-
152-
// And now make sure the session is just fine
153-
res = client.execute(
154-
new HttpRequest(POST, "/session")
155-
.addHeader("Content-Type", JSON_UTF_8)
156-
.setContent(Contents.asJson(ImmutableMap.of(
157-
"capabilities", ImmutableMap.of(
158-
"alwaysMatch", Browser.detect().getCapabilities())))));
159-
160-
assertThat(res.isSuccessful()).isTrue();
139+
try (HttpClient client = HttpClient.Factory.createDefault().createClient(server.getUrl())) {
140+
// Attempt to create a session with an origin header but content type set
141+
HttpResponse res = client.execute(
142+
new HttpRequest(POST, "/session")
143+
.addHeader("Content-Type", JSON_UTF_8)
144+
.addHeader("Origin", "localhost")
145+
.setContent(Contents.asJson(ImmutableMap.of(
146+
"capabilities", ImmutableMap.of(
147+
"alwaysMatch", Browser.detect().getCapabilities())))));
148+
149+
assertThat(res.getStatus()).isEqualTo(HTTP_INTERNAL_ERROR);
150+
151+
// And now make sure the session is just fine
152+
res = client.execute(
153+
new HttpRequest(POST, "/session")
154+
.addHeader("Content-Type", JSON_UTF_8)
155+
.setContent(Contents.asJson(ImmutableMap.of(
156+
"capabilities", ImmutableMap.of(
157+
"alwaysMatch", Browser.detect().getCapabilities())))));
158+
159+
assertThat(res.isSuccessful()).isTrue();
160+
}
161161
}
162162

163163
@Test
@@ -228,9 +228,10 @@ public void shouldRetryNewSessionRequestOnUnexpectedError() throws URISyntaxExce
228228
"capabilities", ImmutableMap.of(
229229
"alwaysMatch", capabilities))));
230230

231-
HttpClient client = clientFactory.createClient(server.getUrl());
232-
HttpResponse httpResponse = client.execute(request);
233-
assertThat(httpResponse.getStatus()).isEqualTo(HTTP_OK);
231+
try (HttpClient client = clientFactory.createClient(server.getUrl())) {
232+
HttpResponse httpResponse = client.execute(request);
233+
assertThat(httpResponse.getStatus()).isEqualTo(HTTP_OK);
234+
}
234235
}
235236

236237
}

‎java/server/test/org/openqa/selenium/grid/router/SessionQueueGridTest.java

+3-2
Original file line numberDiff line numberDiff line change
@@ -244,7 +244,8 @@ private HttpResponse createSession(ImmutableMap<String, String> caps) {
244244
"capabilities", ImmutableMap.of(
245245
"alwaysMatch", caps))));
246246

247-
HttpClient client = clientFactory.createClient(server.getUrl());
248-
return client.execute(request);
247+
try (HttpClient client = clientFactory.createClient(server.getUrl())) {
248+
return client.execute(request);
249+
}
249250
}
250251
}

0 commit comments

Comments
 (0)