Skip to content

Commit df41b58

Browse files
diemoldiemol
committed
[grid] Setting session-timeout as read timeout for http client
In short, using the default of 180s for timeout in the http client used in the factories is not a good idea because a user might set a shorter session-timeout, and therefore, a session could be stopped while a http request is being executed. This change sets session-timeout as the timeout for the http clients used in the driver factories, and with that, no session should be stopped while a http request is being executed. Fixes #10404
1 parent 699c52a commit df41b58

8 files changed

Lines changed: 102 additions & 57 deletions

File tree

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

Lines changed: 14 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,7 @@
5050

5151
import java.net.URI;
5252
import java.net.URL;
53+
import java.time.Duration;
5354
import java.time.Instant;
5455
import java.util.HashMap;
5556
import java.util.Map;
@@ -67,19 +68,22 @@ public class DriverServiceSessionFactory implements SessionFactory {
6768

6869
private final Tracer tracer;
6970
private final HttpClient.Factory clientFactory;
71+
private final Duration sessionTimeout;
7072
private final Predicate<Capabilities> predicate;
7173
private final DriverService.Builder<?, ?> builder;
7274
private final Capabilities stereotype;
7375
private final SessionCapabilitiesMutator sessionCapabilitiesMutator;
7476

7577
public DriverServiceSessionFactory(
76-
Tracer tracer,
77-
HttpClient.Factory clientFactory,
78-
Capabilities stereotype,
79-
Predicate<Capabilities> predicate,
80-
DriverService.Builder<?, ?> builder) {
78+
Tracer tracer,
79+
HttpClient.Factory clientFactory,
80+
Duration sessionTimeout,
81+
Capabilities stereotype,
82+
Predicate<Capabilities> predicate,
83+
DriverService.Builder<?, ?> builder) {
8184
this.tracer = Require.nonNull("Tracer", tracer);
8285
this.clientFactory = Require.nonNull("HTTP client factory", clientFactory);
86+
this.sessionTimeout = Require.nonNull("Session timeout", sessionTimeout);
8387
this.stereotype = ImmutableCapabilities.copyOf(Require.nonNull("Stereotype", stereotype));
8488
this.predicate = Require.nonNull("Accepted capabilities predicate", predicate);
8589
this.builder = Require.nonNull("Driver service builder", builder);
@@ -125,7 +129,11 @@ public Either<WebDriverException, ActiveSession> apply(CreateSessionRequest sess
125129
URL serviceURL = service.getUrl();
126130
attributeMap.put(AttributeKey.DRIVER_URL.getKey(),
127131
EventAttribute.setValue(serviceURL.toString()));
128-
ClientConfig clientConfig = ClientConfig.defaultConfig().baseUrl(serviceURL).withRetries();
132+
133+
ClientConfig clientConfig = ClientConfig
134+
.defaultConfig()
135+
.readTimeout(sessionTimeout)
136+
.baseUrl(serviceURL);
129137
HttpClient client = clientFactory.createClient(clientConfig);
130138

131139
Command command = new Command(null, DriverCommand.NEW_SESSION(capabilities));

‎java/src/org/openqa/selenium/grid/node/docker/DockerOptions.java‎

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -17,8 +17,6 @@
1717

1818
package org.openqa.selenium.grid.node.docker;
1919

20-
import static org.openqa.selenium.Platform.WINDOWS;
21-
2220
import com.google.common.collect.HashMultimap;
2321
import com.google.common.collect.ImmutableMultimap;
2422
import com.google.common.collect.Multimap;
@@ -42,6 +40,7 @@
4240

4341
import java.net.URI;
4442
import java.net.URISyntaxException;
43+
import java.time.Duration;
4544
import java.util.Arrays;
4645
import java.util.Collection;
4746
import java.util.List;
@@ -51,6 +50,8 @@
5150
import java.util.concurrent.ExecutionException;
5251
import java.util.logging.Logger;
5352

53+
import static org.openqa.selenium.Platform.WINDOWS;
54+
5455
public class DockerOptions {
5556

5657
static final String DOCKER_SECTION = "docker";
@@ -116,17 +117,19 @@ private boolean isEnabled(Docker docker) {
116117

117118
public Map<Capabilities, Collection<SessionFactory>> getDockerSessionFactories(
118119
Tracer tracer,
119-
HttpClient.Factory clientFactory) {
120+
HttpClient.Factory clientFactory,
121+
Duration sessionTimeout) {
120122

121-
HttpClient client = clientFactory.createClient(ClientConfig.defaultConfig().baseUri(getDockerUri()));
123+
HttpClient client = clientFactory.createClient(
124+
ClientConfig.defaultConfig().baseUri(getDockerUri()));
122125
Docker docker = new Docker(client);
123126

124127
if (!isEnabled(docker)) {
125128
throw new DockerException("Unable to reach the Docker daemon at " + getDockerUri());
126129
}
127130

128131
List<String> allConfigs = config.getAll(DOCKER_SECTION, "configs")
129-
.orElseThrow(() -> new DockerException("Unable to find docker configs"));
132+
.orElseThrow(() -> new DockerException("Unable to find docker configs"));
130133

131134
Multimap<String, Capabilities> kinds = HashMultimap.create();
132135
for (int i = 0; i < allConfigs.size(); i++) {
@@ -165,6 +168,7 @@ public Map<Capabilities, Collection<SessionFactory>> getDockerSessionFactories(
165168
new DockerSessionFactory(
166169
tracer,
167170
clientFactory,
171+
sessionTimeout,
168172
docker,
169173
getDockerUri(),
170174
image,

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

Lines changed: 16 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -17,13 +17,6 @@
1717

1818
package org.openqa.selenium.grid.node.docker;
1919

20-
import static java.util.Optional.ofNullable;
21-
import static org.openqa.selenium.docker.ContainerConfig.image;
22-
import static org.openqa.selenium.remote.Dialect.W3C;
23-
import static org.openqa.selenium.remote.http.Contents.string;
24-
import static org.openqa.selenium.remote.http.HttpMethod.GET;
25-
import static org.openqa.selenium.remote.tracing.Tags.EXCEPTION;
26-
2720
import org.openqa.selenium.Capabilities;
2821
import org.openqa.selenium.Dimension;
2922
import org.openqa.selenium.ImmutableCapabilities;
@@ -53,6 +46,7 @@
5346
import org.openqa.selenium.remote.ProtocolHandshake;
5447
import org.openqa.selenium.remote.Response;
5548
import org.openqa.selenium.remote.SessionId;
49+
import org.openqa.selenium.remote.http.ClientConfig;
5650
import org.openqa.selenium.remote.http.HttpClient;
5751
import org.openqa.selenium.remote.http.HttpRequest;
5852
import org.openqa.selenium.remote.http.HttpResponse;
@@ -84,12 +78,20 @@
8478
import java.util.logging.Level;
8579
import java.util.logging.Logger;
8680

81+
import static java.util.Optional.ofNullable;
82+
import static org.openqa.selenium.docker.ContainerConfig.image;
83+
import static org.openqa.selenium.remote.Dialect.W3C;
84+
import static org.openqa.selenium.remote.http.Contents.string;
85+
import static org.openqa.selenium.remote.http.HttpMethod.GET;
86+
import static org.openqa.selenium.remote.tracing.Tags.EXCEPTION;
87+
8788
public class DockerSessionFactory implements SessionFactory {
8889

8990
private static final Logger LOG = Logger.getLogger(DockerSessionFactory.class.getName());
9091

9192
private final Tracer tracer;
9293
private final HttpClient.Factory clientFactory;
94+
private final Duration sessionTimeout;
9395
private final Docker docker;
9496
private final URI dockerUri;
9597
private final Image browserImage;
@@ -103,6 +105,7 @@ public class DockerSessionFactory implements SessionFactory {
103105
public DockerSessionFactory(
104106
Tracer tracer,
105107
HttpClient.Factory clientFactory,
108+
Duration sessionTimeout,
106109
Docker docker,
107110
URI dockerUri,
108111
Image browserImage,
@@ -113,6 +116,7 @@ public DockerSessionFactory(
113116
boolean runningInDocker) {
114117
this.tracer = Require.nonNull("Tracer", tracer);
115118
this.clientFactory = Require.nonNull("HTTP client", clientFactory);
119+
this.sessionTimeout = Require.nonNull("Session timeout", sessionTimeout);
116120
this.docker = Require.nonNull("Docker command", docker);
117121
this.dockerUri = Require.nonNull("Docker URI", dockerUri);
118122
this.browserImage = Require.nonNull("Docker browser image", browserImage);
@@ -148,7 +152,11 @@ public Either<WebDriverException, ActiveSession> apply(CreateSessionRequest sess
148152

149153
String containerIp = containerInfo.getIp();
150154
URL remoteAddress = getUrl(port, containerIp);
151-
HttpClient client = clientFactory.createClient(remoteAddress);
155+
ClientConfig clientConfig = ClientConfig
156+
.defaultConfig()
157+
.baseUrl(remoteAddress)
158+
.readTimeout(sessionTimeout);
159+
HttpClient client = clientFactory.createClient(clientConfig);
152160

153161
attributeMap.put("docker.browser.image", EventAttribute.setValue(browserImage.toString()));
154162
attributeMap.put("container.port", EventAttribute.setValue(port));

‎java/src/org/openqa/selenium/grid/node/local/LocalNode.java‎

Lines changed: 13 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -17,18 +17,6 @@
1717

1818
package org.openqa.selenium.grid.node.local;
1919

20-
import static com.google.common.collect.ImmutableSet.toImmutableSet;
21-
import static org.openqa.selenium.grid.data.Availability.DOWN;
22-
import static org.openqa.selenium.grid.data.Availability.DRAINING;
23-
import static org.openqa.selenium.grid.data.Availability.UP;
24-
import static org.openqa.selenium.grid.node.CapabilityResponseEncoder.getEncoder;
25-
import static org.openqa.selenium.remote.HttpSessionId.getSessionId;
26-
import static org.openqa.selenium.remote.RemoteTags.CAPABILITIES;
27-
import static org.openqa.selenium.remote.RemoteTags.SESSION_ID;
28-
import static org.openqa.selenium.remote.http.Contents.asJson;
29-
import static org.openqa.selenium.remote.http.Contents.string;
30-
import static org.openqa.selenium.remote.http.HttpMethod.DELETE;
31-
3220
import com.google.common.annotations.VisibleForTesting;
3321
import com.google.common.base.Ticker;
3422
import com.google.common.cache.Cache;
@@ -105,6 +93,18 @@
10593
import java.util.logging.Logger;
10694
import java.util.stream.Collectors;
10795

96+
import static com.google.common.collect.ImmutableSet.toImmutableSet;
97+
import static org.openqa.selenium.grid.data.Availability.DOWN;
98+
import static org.openqa.selenium.grid.data.Availability.DRAINING;
99+
import static org.openqa.selenium.grid.data.Availability.UP;
100+
import static org.openqa.selenium.grid.node.CapabilityResponseEncoder.getEncoder;
101+
import static org.openqa.selenium.remote.HttpSessionId.getSessionId;
102+
import static org.openqa.selenium.remote.RemoteTags.CAPABILITIES;
103+
import static org.openqa.selenium.remote.RemoteTags.SESSION_ID;
104+
import static org.openqa.selenium.remote.http.Contents.asJson;
105+
import static org.openqa.selenium.remote.http.Contents.string;
106+
import static org.openqa.selenium.remote.http.HttpMethod.DELETE;
107+
108108
@ManagedService(objectName = "org.seleniumhq.grid:type=Node,name=LocalNode",
109109
description = "Node running the webdriver sessions.")
110110
public class LocalNode extends Node {
@@ -218,6 +218,7 @@ private LocalNode(
218218
TimeUnit.SECONDS);
219219

220220
bus.addListener(SessionClosedEvent.listener(id -> {
221+
// Shouldn't we check that the session actually belongs to this Node?
221222
// Listen to session terminated events, so we know when to fire the NodeDrainComplete event
222223
if (this.isDraining()) {
223224
int done = pendingSessions.decrementAndGet();

‎java/src/org/openqa/selenium/grid/node/local/LocalNodeFactory.java‎

Lines changed: 14 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,7 @@
3939
import org.openqa.selenium.remote.tracing.Tracer;
4040

4141
import java.io.File;
42+
import java.time.Duration;
4243
import java.util.ArrayList;
4344
import java.util.Collection;
4445
import java.util.Comparator;
@@ -58,31 +59,33 @@ public static Node create(Config config) {
5859
Tracer tracer = loggingOptions.getTracer();
5960
HttpClient.Factory clientFactory = networkOptions.getHttpClientFactory(tracer);
6061

62+
Duration sessionTimeout = nodeOptions.getSessionTimeout();
6163
LocalNode.Builder builder = LocalNode.builder(
62-
tracer,
63-
eventOptions.getEventBus(),
64-
serverOptions.getExternalUri(),
65-
nodeOptions.getPublicGridUri().orElseGet(serverOptions::getExternalUri),
66-
secretOptions.getRegistrationSecret())
64+
tracer,
65+
eventOptions.getEventBus(),
66+
serverOptions.getExternalUri(),
67+
nodeOptions.getPublicGridUri().orElseGet(serverOptions::getExternalUri),
68+
secretOptions.getRegistrationSecret())
6769
.maximumConcurrentSessions(nodeOptions.getMaxSessions())
68-
.sessionTimeout(nodeOptions.getSessionTimeout())
70+
.sessionTimeout(sessionTimeout)
6971
.heartbeatPeriod(nodeOptions.getHeartbeatPeriod());
7072

7173

7274
List<DriverService.Builder<?, ?>> builders = new ArrayList<>();
7375
ServiceLoader.load(DriverService.Builder.class).forEach(builders::add);
7476

7577
nodeOptions
76-
.getSessionFactories(caps -> createSessionFactory(tracer, clientFactory, builders, caps))
78+
.getSessionFactories(
79+
caps -> createSessionFactory(tracer, clientFactory, sessionTimeout, builders, caps))
7780
.forEach((caps, factories) -> factories.forEach(factory -> builder.add(caps, factory)));
7881

7982
if (config.getAll("docker", "configs").isPresent()) {
80-
new DockerOptions(config).getDockerSessionFactories(tracer, clientFactory)
83+
new DockerOptions(config).getDockerSessionFactories(tracer, clientFactory, sessionTimeout)
8184
.forEach((caps, factories) -> factories.forEach(factory -> builder.add(caps, factory)));
8285
}
8386

8487
if (config.getAll("relay", "configs").isPresent()) {
85-
new RelayOptions(config).getSessionFactories(tracer, clientFactory)
88+
new RelayOptions(config).getSessionFactories(tracer, clientFactory, sessionTimeout)
8689
.forEach((caps, factories) -> factories.forEach(factory -> builder.add(caps, factory)));
8790
}
8891

@@ -92,6 +95,7 @@ public static Node create(Config config) {
9295
private static Collection<SessionFactory> createSessionFactory(
9396
Tracer tracer,
9497
HttpClient.Factory clientFactory,
98+
Duration sessionTimeout,
9599
List<DriverService.Builder<?, ?>> builders,
96100
Capabilities stereotype) {
97101
ImmutableList.Builder<SessionFactory> toReturn = ImmutableList.builder();
@@ -122,6 +126,7 @@ private static Collection<SessionFactory> createSessionFactory(
122126
toReturn.add(new DriverServiceSessionFactory(
123127
tracer,
124128
clientFactory,
129+
sessionTimeout,
125130
stereotype,
126131
capabilities -> slotMatcher.matches(stereotype, capabilities),
127132
driverServiceBuilder));

‎java/src/org/openqa/selenium/grid/node/relay/RelayOptions.java‎

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -17,9 +17,6 @@
1717

1818
package org.openqa.selenium.grid.node.relay;
1919

20-
import static org.openqa.selenium.remote.http.Contents.string;
21-
import static org.openqa.selenium.remote.http.HttpMethod.GET;
22-
2320
import com.google.common.collect.HashMultimap;
2421
import com.google.common.collect.ImmutableMultimap;
2522
import com.google.common.collect.Multimap;
@@ -38,12 +35,16 @@
3835

3936
import java.net.URI;
4037
import java.net.URISyntaxException;
38+
import java.time.Duration;
4139
import java.util.Collection;
4240
import java.util.List;
4341
import java.util.Map;
4442
import java.util.Optional;
4543
import java.util.logging.Logger;
4644

45+
import static org.openqa.selenium.remote.http.Contents.string;
46+
import static org.openqa.selenium.remote.http.HttpMethod.GET;
47+
4748
public class RelayOptions {
4849

4950
static final String RELAY_SECTION = "relay";
@@ -121,7 +122,8 @@ private boolean isServiceUp(HttpClient client) {
121122

122123
public Map<Capabilities, Collection<SessionFactory>> getSessionFactories(
123124
Tracer tracer,
124-
HttpClient.Factory clientFactory) {
125+
HttpClient.Factory clientFactory,
126+
Duration sessionTimeout) {
125127

126128
HttpClient client = clientFactory
127129
.createClient(ClientConfig.defaultConfig().baseUri(getServiceUri()));
@@ -160,6 +162,7 @@ public Map<Capabilities, Collection<SessionFactory>> getSessionFactories(
160162
new RelaySessionFactory(
161163
tracer,
162164
clientFactory,
165+
sessionTimeout,
163166
getServiceUri(),
164167
getServiceStatusUri(),
165168
stereotype));

‎java/src/org/openqa/selenium/grid/node/relay/RelaySessionFactory.java‎

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@
3434
import org.openqa.selenium.remote.ProtocolHandshake;
3535
import org.openqa.selenium.remote.Response;
3636
import org.openqa.selenium.remote.SessionId;
37+
import org.openqa.selenium.remote.http.ClientConfig;
3738
import org.openqa.selenium.remote.http.Contents;
3839
import org.openqa.selenium.remote.http.HttpClient;
3940
import org.openqa.selenium.remote.http.HttpMethod;
@@ -47,6 +48,7 @@
4748
import java.net.MalformedURLException;
4849
import java.net.URI;
4950
import java.net.URL;
51+
import java.time.Duration;
5052
import java.time.Instant;
5153
import java.util.HashMap;
5254
import java.util.Map;
@@ -73,18 +75,21 @@ public class RelaySessionFactory implements SessionFactory {
7375

7476
private final Tracer tracer;
7577
private final HttpClient.Factory clientFactory;
78+
private final Duration sessionTimeout;
7679
private final URL serviceUrl;
7780
private final URL serviceStatusUrl;
7881
private final Capabilities stereotype;
7982

8083
public RelaySessionFactory(
8184
Tracer tracer,
8285
HttpClient.Factory clientFactory,
86+
Duration sessionTimeout,
8387
URI serviceUri,
8488
URI serviceStatusUri,
8589
Capabilities stereotype) {
8690
this.tracer = Require.nonNull("Tracer", tracer);
8791
this.clientFactory = Require.nonNull("HTTP client", clientFactory);
92+
this.sessionTimeout = Require.nonNull("Session timeout", sessionTimeout);
8893
this.serviceUrl = createUrlFromUri(Require.nonNull("Service URL", serviceUri));
8994
this.serviceStatusUrl = createUrlFromUri(serviceStatusUri);
9095
this.stereotype = ImmutableCapabilities
@@ -141,7 +146,11 @@ public Either<WebDriverException, ActiveSession> apply(CreateSessionRequest sess
141146
attributeMap.put(LOGGER_CLASS.getKey(), setValue(this.getClass().getName()));
142147
attributeMap.put(DRIVER_URL.getKey(), setValue(serviceUrl.toString()));
143148

144-
HttpClient client = clientFactory.createClient(serviceUrl);
149+
ClientConfig clientConfig = ClientConfig
150+
.defaultConfig()
151+
.readTimeout(sessionTimeout)
152+
.baseUrl(serviceUrl);
153+
HttpClient client = clientFactory.createClient(clientConfig);
145154

146155
Command command = new Command(null, DriverCommand.NEW_SESSION(capabilities));
147156
try {

0 commit comments

Comments
 (0)