Skip to content

Commit 26a9f94

Browse files
joerg1985joerg1985
committed
[grid] detect a client timeout while session creation #14743 
1 parent be8fc7e commit 26a9f94

File tree

4 files changed

+220
-26
lines changed

4 files changed

+220
-26
lines changed

‎java/src/org/openqa/selenium/grid/distributor/local/LocalDistributor.java

+1-4
Original file line numberDiff line numberDiff line change
@@ -853,11 +853,8 @@ private void handleNewSessionRequest(SessionRequest sessionRequest) {
853853
}
854854
}
855855

856-
// 'complete' will return 'true' if the session has not timed out during the creation
857-
// process: it's still a valid session as it can be used by the client
858856
boolean isSessionValid = sessionQueue.complete(reqId, response);
859-
// If the session request has timed out, tell the Node to remove the session, so that does
860-
// not stall
857+
// terminate invalid sessions to avoid stale sessions
861858
if (!isSessionValid && response.isRight()) {
862859
LOG.log(
863860
Level.INFO,

‎java/src/org/openqa/selenium/grid/sessionqueue/local/LocalNewSessionQueue.java

+19-22
Original file line numberDiff line numberDiff line change
@@ -220,10 +220,14 @@ public HttpResponse addToQueue(SessionRequest request) {
220220
result = Either.left(new SessionNotCreatedException("New session request timed out"));
221221
}
222222
} catch (InterruptedException e) {
223+
// the client will never see the session, ensure the session is disposed
224+
data.cancel();
223225
Thread.currentThread().interrupt();
224226
result =
225227
Either.left(new SessionNotCreatedException("Interrupted when creating the session", e));
226228
} catch (RuntimeException e) {
229+
// the client will never see the session, ensure the session is disposed
230+
data.cancel();
227231
result =
228232
Either.left(
229233
new SessionNotCreatedException("An error occurred creating the session", e));
@@ -367,43 +371,30 @@ public List<SessionRequest> getNextAvailable(Map<Capabilities, Long> stereotypes
367371
}
368372
}
369373

370-
/** Returns true if the session is still valid (not timed out) */
374+
/** Returns true if the session is still valid (not timed out and not canceled) */
371375
@Override
372376
public boolean complete(
373377
RequestId reqId, Either<SessionNotCreatedException, CreateSessionResponse> result) {
374378
Require.nonNull("New session request", reqId);
375379
Require.nonNull("Result", result);
376380
TraceContext context = contexts.getOrDefault(reqId, tracer.getCurrentContext());
377381
try (Span ignored = context.createSpan("sessionqueue.completed")) {
378-
Lock readLock = lock.readLock();
379-
readLock.lock();
380382
Data data;
381-
boolean isSessionTimedOut = false;
382-
try {
383-
data = requests.get(reqId);
384-
} finally {
385-
readLock.unlock();
386-
}
387-
388-
if (data == null) {
389-
return false;
390-
} else {
391-
isSessionTimedOut = isTimedOut(Instant.now(), data);
392-
}
393-
394383
Lock writeLock = lock.writeLock();
395384
writeLock.lock();
396385
try {
397-
requests.remove(reqId);
386+
data = requests.remove(reqId);
398387
queue.removeIf(req -> reqId.equals(req.getRequestId()));
399388
contexts.remove(reqId);
400389
} finally {
401390
writeLock.unlock();
402391
}
403392

404-
data.setResult(result);
393+
if (data == null) {
394+
return false;
395+
}
405396

406-
return !isSessionTimedOut;
397+
return data.setResult(result);
407398
}
408399
}
409400

@@ -466,6 +457,7 @@ private class Data {
466457
private final CountDownLatch latch = new CountDownLatch(1);
467458
private Either<SessionNotCreatedException, CreateSessionResponse> result;
468459
private boolean complete;
460+
private boolean canceled;
469461

470462
public Data(Instant enqueued) {
471463
this.endTime = enqueued.plus(requestTimeout);
@@ -476,14 +468,19 @@ public synchronized Either<SessionNotCreatedException, CreateSessionResponse> ge
476468
return result;
477469
}
478470

479-
public synchronized void setResult(
471+
public synchronized void cancel() {
472+
canceled = true;
473+
}
474+
475+
public synchronized boolean setResult(
480476
Either<SessionNotCreatedException, CreateSessionResponse> result) {
481-
if (complete) {
482-
return;
477+
if (complete || canceled) {
478+
return false;
483479
}
484480
this.result = result;
485481
complete = true;
486482
latch.countDown();
483+
return true;
487484
}
488485
}
489486
}

‎java/test/org/openqa/selenium/grid/router/BUILD.bazel

+1
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ load("//java:version.bzl", "TOOLS_JAVA_VERSION")
44
load("//java/src/org/openqa/selenium/devtools:versions.bzl", "CDP_DEPS")
55

66
LARGE_TESTS = [
7+
"DistributedTest.java",
78
"DistributedCdpTest.java",
89
"NewSessionCreationTest.java",
910
"RemoteWebDriverDownloadTest.java",

‎java/test/org/openqa/selenium/grid/router/DistributedTest.java

+199
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,199 @@
1+
// Licensed to the Software Freedom Conservancy (SFC) under one
2+
// or more contributor license agreements. See the NOTICE file
3+
// distributed with this work for additional information
4+
// regarding copyright ownership. The SFC licenses this file
5+
// to you under the Apache License, Version 2.0 (the
6+
// "License"); you may not use this file except in compliance
7+
// with the License. You may obtain a copy of the License at
8+
//
9+
// http://www.apache.org/licenses/LICENSE-2.0
10+
//
11+
// Unless required by applicable law or agreed to in writing,
12+
// software distributed under the License is distributed on an
13+
// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
14+
// KIND, either express or implied. See the License for the
15+
// specific language governing permissions and limitations
16+
// under the License.
17+
18+
package org.openqa.selenium.grid.router;
19+
20+
import static java.nio.charset.StandardCharsets.UTF_8;
21+
import static org.assertj.core.api.Assertions.assertThat;
22+
23+
import java.io.StringReader;
24+
import java.time.Duration;
25+
import java.util.LinkedList;
26+
import java.util.List;
27+
import java.util.Map;
28+
import java.util.Objects;
29+
import org.junit.jupiter.api.AfterEach;
30+
import org.junit.jupiter.api.Assertions;
31+
import org.junit.jupiter.api.BeforeEach;
32+
import org.junit.jupiter.api.Test;
33+
import org.openqa.selenium.SessionNotCreatedException;
34+
import org.openqa.selenium.WebDriver;
35+
import org.openqa.selenium.grid.config.MapConfig;
36+
import org.openqa.selenium.grid.config.MemoizedConfig;
37+
import org.openqa.selenium.grid.config.TomlConfig;
38+
import org.openqa.selenium.grid.router.DeploymentTypes.Deployment;
39+
import org.openqa.selenium.grid.server.BaseServerOptions;
40+
import org.openqa.selenium.grid.server.Server;
41+
import org.openqa.selenium.json.Json;
42+
import org.openqa.selenium.json.JsonInput;
43+
import org.openqa.selenium.netty.server.NettyServer;
44+
import org.openqa.selenium.remote.RemoteWebDriver;
45+
import org.openqa.selenium.remote.http.ClientConfig;
46+
import org.openqa.selenium.remote.http.Contents;
47+
import org.openqa.selenium.remote.http.HttpClient;
48+
import org.openqa.selenium.remote.http.HttpMethod;
49+
import org.openqa.selenium.remote.http.HttpRequest;
50+
import org.openqa.selenium.remote.http.HttpResponse;
51+
import org.openqa.selenium.testing.Safely;
52+
import org.openqa.selenium.testing.TearDownFixture;
53+
import org.openqa.selenium.testing.drivers.Browser;
54+
55+
class DistributedTest {
56+
57+
private final List<TearDownFixture> tearDowns = new LinkedList<>();
58+
private Server<?> server;
59+
private Browser browser;
60+
private Server<?> appServer;
61+
62+
@BeforeEach
63+
public void setupServers() {
64+
browser = Objects.requireNonNull(Browser.detect());
65+
66+
Deployment deployment =
67+
DeploymentTypes.DISTRIBUTED.start(
68+
browser.getCapabilities(),
69+
new TomlConfig(
70+
new StringReader(
71+
"[node]\n"
72+
+ "driver-implementation = "
73+
+ String.format("\"%s\"", browser.displayName())
74+
+ "\n"
75+
+ "session-timeout = 240"
76+
+ "\n"
77+
+ "override-max-sessions = true"
78+
+ "\n"
79+
+ "max-sessions = 2")));
80+
tearDowns.add(deployment);
81+
82+
server = deployment.getServer();
83+
84+
appServer =
85+
new NettyServer(
86+
new BaseServerOptions(new MemoizedConfig(new MapConfig(Map.of()))),
87+
req -> {
88+
try {
89+
Thread.sleep(2000);
90+
} catch (InterruptedException e) {
91+
throw new RuntimeException(e);
92+
}
93+
return new HttpResponse().setContent(Contents.string("<h1>Cheese</h1>", UTF_8));
94+
});
95+
96+
tearDowns.add(() -> appServer.stop());
97+
appServer.start();
98+
}
99+
100+
@AfterEach
101+
public void tearDown() {
102+
tearDowns.parallelStream().forEach(Safely::safelyCall);
103+
}
104+
105+
@Test
106+
void clientTimeoutDoesNotLeakARunningBrowser() throws Exception {
107+
assertThat(server.isStarted()).isTrue();
108+
109+
// using nanoTime is intentionally, the clock in WSL2 is jumping
110+
var start = System.nanoTime();
111+
112+
// one healthy to ensure the distributed grid is working as expected
113+
WebDriver healthy =
114+
RemoteWebDriver.builder()
115+
.oneOf(browser.getCapabilities())
116+
.config(
117+
ClientConfig.defaultConfig()
118+
.baseUrl(server.getUrl())
119+
// ensures the time taken * 2 is smaller than session-timeout
120+
.readTimeout(Duration.ofSeconds(90)))
121+
.build();
122+
123+
var end = System.nanoTime();
124+
125+
try {
126+
// provoke the client to run into a http timeout
127+
SessionNotCreatedException nce =
128+
Assertions.assertThrows(
129+
SessionNotCreatedException.class,
130+
() ->
131+
RemoteWebDriver.builder()
132+
.oneOf(browser.getCapabilities())
133+
.config(
134+
ClientConfig.defaultConfig()
135+
.baseUrl(server.getUrl())
136+
.readTimeout(Duration.ofMillis(600)))
137+
.build());
138+
139+
assertThat(nce.getMessage()).contains("TimeoutException");
140+
141+
Thread.sleep(
142+
// ensure the grid has some time to start the browser
143+
Duration.ofNanos((end - start) * 2)
144+
// and shutdown the browser
145+
.plusSeconds(20)
146+
.toMillis());
147+
148+
HttpClient client = HttpClient.Factory.createDefault().createClient(server.getUrl());
149+
try {
150+
HttpRequest request = new HttpRequest(HttpMethod.POST, "/graphql");
151+
request.setContent(Contents.utf8String("{\"query\": \"{grid { sessionCount }}\"}"));
152+
HttpResponse response = client.execute(request);
153+
154+
JsonInput input = new Json().newInput(Contents.reader(response));
155+
int sessionCount = -1;
156+
157+
input.beginObject();
158+
while (input.hasNext()) {
159+
switch (input.nextName()) {
160+
case "data":
161+
input.beginObject();
162+
while (input.hasNext()) {
163+
switch (input.nextName()) {
164+
case "grid":
165+
input.beginObject();
166+
while (input.hasNext()) {
167+
switch (input.nextName()) {
168+
case "sessionCount":
169+
sessionCount = input.read(Integer.class);
170+
break;
171+
default:
172+
input.skipValue();
173+
break;
174+
}
175+
}
176+
input.endObject();
177+
break;
178+
default:
179+
input.skipValue();
180+
break;
181+
}
182+
}
183+
input.endObject();
184+
break;
185+
default:
186+
input.skipValue();
187+
break;
188+
}
189+
}
190+
191+
Assertions.assertEquals(1, sessionCount);
192+
} finally {
193+
Safely.safelyCall(client::close);
194+
}
195+
} finally {
196+
Safely.safelyCall(healthy::close);
197+
}
198+
}
199+
}

0 commit comments

Comments
 (0)