Skip to content

Commit

Permalink
[grid] detect a client timeout while session creation #14743
Browse files Browse the repository at this point in the history
  • Loading branch information
joerg1985 committed Nov 14, 2024
1 parent 0a724b1 commit 3af493d
Show file tree
Hide file tree
Showing 4 changed files with 220 additions and 26 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -853,11 +853,8 @@ private void handleNewSessionRequest(SessionRequest sessionRequest) {
}
}

// 'complete' will return 'true' if the session has not timed out during the creation
// process: it's still a valid session as it can be used by the client
boolean isSessionValid = sessionQueue.complete(reqId, response);
// If the session request has timed out, tell the Node to remove the session, so that does
// not stall
// terminate invalid sessions to avoid stale sessions
if (!isSessionValid && response.isRight()) {
LOG.log(
Level.INFO,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -220,10 +220,14 @@ public HttpResponse addToQueue(SessionRequest request) {
result = Either.left(new SessionNotCreatedException("New session request timed out"));
}
} catch (InterruptedException e) {
// the client will never see the session, ensure the session is disposed
data.cancel();
Thread.currentThread().interrupt();
result =
Either.left(new SessionNotCreatedException("Interrupted when creating the session", e));
} catch (RuntimeException e) {
// the client will never see the session, ensure the session is disposed
data.cancel();
result =
Either.left(
new SessionNotCreatedException("An error occurred creating the session", e));
Expand Down Expand Up @@ -367,43 +371,30 @@ public List<SessionRequest> getNextAvailable(Map<Capabilities, Long> stereotypes
}
}

/** Returns true if the session is still valid (not timed out) */
/** Returns true if the session is still valid (not timed out and not canceled) */
@Override
public boolean complete(
RequestId reqId, Either<SessionNotCreatedException, CreateSessionResponse> result) {
Require.nonNull("New session request", reqId);
Require.nonNull("Result", result);
TraceContext context = contexts.getOrDefault(reqId, tracer.getCurrentContext());
try (Span ignored = context.createSpan("sessionqueue.completed")) {
Lock readLock = lock.readLock();
readLock.lock();
Data data;
boolean isSessionTimedOut = false;
try {
data = requests.get(reqId);
} finally {
readLock.unlock();
}

if (data == null) {
return false;
} else {
isSessionTimedOut = isTimedOut(Instant.now(), data);
}

Lock writeLock = lock.writeLock();
writeLock.lock();
try {
requests.remove(reqId);
data = requests.remove(reqId);
queue.removeIf(req -> reqId.equals(req.getRequestId()));
contexts.remove(reqId);
} finally {
writeLock.unlock();
}

data.setResult(result);
if (data == null) {
return false;
}

return !isSessionTimedOut;
return data.setResult(result);
}
}

Expand Down Expand Up @@ -466,6 +457,7 @@ private class Data {
private final CountDownLatch latch = new CountDownLatch(1);
private Either<SessionNotCreatedException, CreateSessionResponse> result;
private boolean complete;
private boolean canceled;

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

public synchronized void setResult(
public synchronized void cancel() {
canceled = true;
}

public synchronized boolean setResult(
Either<SessionNotCreatedException, CreateSessionResponse> result) {
if (complete) {
return;
if (complete || canceled) {
return false;
}
this.result = result;
complete = true;
latch.countDown();
return true;
}
}
}
1 change: 1 addition & 0 deletions java/test/org/openqa/selenium/grid/router/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ load("//java:version.bzl", "TOOLS_JAVA_VERSION")
load("//java/src/org/openqa/selenium/devtools:versions.bzl", "CDP_DEPS")

LARGE_TESTS = [
"DistributedTest.java",
"DistributedCdpTest.java",
"NewSessionCreationTest.java",
"RemoteWebDriverDownloadTest.java",
Expand Down
199 changes: 199 additions & 0 deletions java/test/org/openqa/selenium/grid/router/DistributedTest.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,199 @@
// Licensed to the Software Freedom Conservancy (SFC) under one
// or more contributor license agreements. See the NOTICE file
// distributed with this work for additional information
// regarding copyright ownership. The SFC licenses this file
// to you under the Apache License, Version 2.0 (the
// "License"); you may not use this file except in compliance
// with the License. You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing,
// software distributed under the License is distributed on an
// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
// KIND, either express or implied. See the License for the
// specific language governing permissions and limitations
// under the License.

package org.openqa.selenium.grid.router;

import static java.nio.charset.StandardCharsets.UTF_8;
import static org.assertj.core.api.Assertions.assertThat;

import java.io.StringReader;
import java.time.Duration;
import java.util.LinkedList;
import java.util.List;
import java.util.Map;
import java.util.Objects;
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.openqa.selenium.SessionNotCreatedException;
import org.openqa.selenium.WebDriver;
import org.openqa.selenium.grid.config.MapConfig;
import org.openqa.selenium.grid.config.MemoizedConfig;
import org.openqa.selenium.grid.config.TomlConfig;
import org.openqa.selenium.grid.router.DeploymentTypes.Deployment;
import org.openqa.selenium.grid.server.BaseServerOptions;
import org.openqa.selenium.grid.server.Server;
import org.openqa.selenium.json.Json;
import org.openqa.selenium.json.JsonInput;
import org.openqa.selenium.netty.server.NettyServer;
import org.openqa.selenium.remote.RemoteWebDriver;
import org.openqa.selenium.remote.http.ClientConfig;
import org.openqa.selenium.remote.http.Contents;
import org.openqa.selenium.remote.http.HttpClient;
import org.openqa.selenium.remote.http.HttpMethod;
import org.openqa.selenium.remote.http.HttpRequest;
import org.openqa.selenium.remote.http.HttpResponse;
import org.openqa.selenium.testing.Safely;
import org.openqa.selenium.testing.TearDownFixture;
import org.openqa.selenium.testing.drivers.Browser;

class DistributedTest {

private final List<TearDownFixture> tearDowns = new LinkedList<>();
private Server<?> server;
private Browser browser;
private Server<?> appServer;

@BeforeEach
public void setupServers() {
browser = Objects.requireNonNull(Browser.detect());

Deployment deployment =
DeploymentTypes.DISTRIBUTED.start(
browser.getCapabilities(),
new TomlConfig(
new StringReader(
"[node]\n"
+ "driver-implementation = "
+ String.format("\"%s\"", browser.displayName())
+ "\n"
+ "session-timeout = 240"
+ "\n"
+ "override-max-sessions = true"
+ "\n"
+ "max-sessions = 2")));
tearDowns.add(deployment);

server = deployment.getServer();

appServer =
new NettyServer(
new BaseServerOptions(new MemoizedConfig(new MapConfig(Map.of()))),
req -> {
try {
Thread.sleep(2000);
} catch (InterruptedException e) {
throw new RuntimeException(e);
}
return new HttpResponse().setContent(Contents.string("<h1>Cheese</h1>", UTF_8));
});

tearDowns.add(() -> appServer.stop());
appServer.start();
}

@AfterEach
public void tearDown() {
tearDowns.parallelStream().forEach(Safely::safelyCall);
}

@Test
void clientTimeoutDoesNotLeakARunningBrowser() throws Exception {
assertThat(server.isStarted()).isTrue();

// using nanoTime is intentionally, the clock in WSL2 is jumping
var start = System.nanoTime();

// one healthy to ensure the distributed grid is working as expected
WebDriver healthy =
RemoteWebDriver.builder()
.oneOf(browser.getCapabilities())
.config(
ClientConfig.defaultConfig()
.baseUrl(server.getUrl())
// ensures the time taken * 2 is smaller than session-timeout
.readTimeout(Duration.ofSeconds(90)))
.build();

var end = System.nanoTime();

try {
// provoke the client to run into a http timeout
SessionNotCreatedException nce =
Assertions.assertThrows(
SessionNotCreatedException.class,
() ->
RemoteWebDriver.builder()
.oneOf(browser.getCapabilities())
.config(
ClientConfig.defaultConfig()
.baseUrl(server.getUrl())
.readTimeout(Duration.ofMillis(600)))
.build());

assertThat(nce.getMessage()).contains("TimeoutException");

Thread.sleep(
// ensure the grid has some time to start the browser
Duration.ofNanos((end - start) * 2)
// and shutdown the browser
.plusSeconds(20)
.toMillis());

HttpClient client = HttpClient.Factory.createDefault().createClient(server.getUrl());
try {
HttpRequest request = new HttpRequest(HttpMethod.POST, "/graphql");
request.setContent(Contents.utf8String("{\"query\": \"{grid { sessionCount }}\"}"));
HttpResponse response = client.execute(request);

JsonInput input = new Json().newInput(Contents.reader(response));
int sessionCount = -1;

input.beginObject();
while (input.hasNext()) {
switch (input.nextName()) {
case "data":
input.beginObject();
while (input.hasNext()) {
switch (input.nextName()) {
case "grid":
input.beginObject();
while (input.hasNext()) {
switch (input.nextName()) {
case "sessionCount":
sessionCount = input.read(Integer.class);
break;
default:
input.skipValue();
break;
}
}
input.endObject();
break;
default:
input.skipValue();
break;
}
}
input.endObject();
break;
default:
input.skipValue();
break;
}
}

Assertions.assertEquals(1, sessionCount);
} finally {
Safely.safelyCall(client::close);
}
} finally {
Safely.safelyCall(healthy::close);
}
}
}

0 comments on commit 3af493d

Please sign in to comment.