From d18f79ff5e76e009fa1014ead2d10a8520f2a23e Mon Sep 17 00:00:00 2001 From: bhecquet bhecquet Date: Thu, 28 Sep 2023 13:38:49 +0000 Subject: [PATCH 01/23] Add logs --- .../selenium/grid/distributor/local/LocalDistributor.java | 7 +++++-- .../org/openqa/selenium/grid/node/local/LocalNode.java | 2 +- .../grid/sessionqueue/local/LocalNewSessionQueue.java | 8 ++++++-- 3 files changed, 12 insertions(+), 5 deletions(-) diff --git a/java/src/org/openqa/selenium/grid/distributor/local/LocalDistributor.java b/java/src/org/openqa/selenium/grid/distributor/local/LocalDistributor.java index 2744de1733ebb..91203fdc0b82c 100644 --- a/java/src/org/openqa/selenium/grid/distributor/local/LocalDistributor.java +++ b/java/src/org/openqa/selenium/grid/distributor/local/LocalDistributor.java @@ -506,7 +506,7 @@ protected Set getAvailableNodes() { public Either newSession( SessionRequest request) throws SessionNotCreatedException { Require.nonNull("Requests to process", request); - + LOG.info(String.format("%s: new session", request.getRequestId())); Span span = tracer.getCurrentContext().createSpan("distributor.new_session"); AttributeMap attributeMap = tracer.createAttributeMap(); try { @@ -558,6 +558,8 @@ public Either newSession( new CreateSessionRequest(request.getDownstreamDialects(), caps, request.getMetadata()); try { + + LOG.info(String.format("%s: new session => start creating", request.getRequestId())); CreateSessionResponse response = startSession(selectedSlot, singleRequest); sessions.add(response.getSession()); model.setSession(selectedSlot, response.getSession()); @@ -571,7 +573,7 @@ public Either newSession( CAPABILITIES_EVENT.accept(attributeMap, sessionCaps); span.setAttribute(SESSION_URI.getKey(), sessionUri); attributeMap.put(SESSION_URI.getKey(), sessionUri); - + LOG.info(String.format("%s: new session => created", request.getRequestId())); String sessionCreatedMessage = "Session created by the Distributor"; span.addEvent(sessionCreatedMessage, attributeMap); LOG.info( @@ -580,6 +582,7 @@ public Either newSession( return Either.right(response); } catch (SessionNotCreatedException e) { + LOG.info(String.format("%s: new session => not created", request.getRequestId())); model.setSession(selectedSlot, null); lastFailure = e; } diff --git a/java/src/org/openqa/selenium/grid/node/local/LocalNode.java b/java/src/org/openqa/selenium/grid/node/local/LocalNode.java index 9f52b048dc959..df2e5aedd4466 100644 --- a/java/src/org/openqa/selenium/grid/node/local/LocalNode.java +++ b/java/src/org/openqa/selenium/grid/node/local/LocalNode.java @@ -401,7 +401,7 @@ public boolean isSupporting(Capabilities capabilities) { public Either newSession( CreateSessionRequest sessionRequest) { Require.nonNull("Session request", sessionRequest); - + LOG.info("new session => start creating"); try (Span span = tracer.getCurrentContext().createSpan("node.new_session")) { AttributeMap attributeMap = tracer.createAttributeMap(); attributeMap.put(AttributeKey.LOGGER_CLASS.getKey(), getClass().getName()); diff --git a/java/src/org/openqa/selenium/grid/sessionqueue/local/LocalNewSessionQueue.java b/java/src/org/openqa/selenium/grid/sessionqueue/local/LocalNewSessionQueue.java index a26222971643c..5a11c610484cf 100644 --- a/java/src/org/openqa/selenium/grid/sessionqueue/local/LocalNewSessionQueue.java +++ b/java/src/org/openqa/selenium/grid/sessionqueue/local/LocalNewSessionQueue.java @@ -44,6 +44,7 @@ import java.util.concurrent.locks.ReentrantReadWriteLock; import java.util.function.Predicate; import java.util.stream.Collectors; +import java.util.logging.Logger; import org.openqa.selenium.Capabilities; import org.openqa.selenium.SessionNotCreatedException; import org.openqa.selenium.concurrent.GuardedRunnable; @@ -93,7 +94,7 @@ objectName = "org.seleniumhq.grid:type=SessionQueue,name=LocalSessionQueue", description = "New session queue") public class LocalNewSessionQueue extends NewSessionQueue implements Closeable { - + private static final Logger LOG = Logger.getLogger(LocalNewSessionQueue.class.getName()); private static final String NAME = "Local New Session Queue"; private final SlotMatcher slotMatcher; private final Duration requestTimeout; @@ -212,15 +213,18 @@ public HttpResponse addToQueue(SessionRequest request) { Either result; try { - + LOG.info(String.format("%s: start waiting session creation", request.getRequestId())); boolean sessionCreated = data.latch.await(requestTimeout.toMillis(), MILLISECONDS); if (!(sessionCreated || isRequestInQueue(request.getRequestId()))) { + LOG.info(String.format("%s: a bit more time", request.getRequestId())); sessionCreated = data.latch.await(5000, MILLISECONDS); } if (sessionCreated) { + LOG.info(String.format("%s: session created", request.getRequestId())); result = data.result; } else { + LOG.info(String.format("%s: session not created", request.getRequestId())); result = Either.left(new SessionNotCreatedException("New session request timed out")); } } catch (InterruptedException e) { From 5e023c78d666e9886cd887ec42d96e1606d01a28 Mon Sep 17 00:00:00 2001 From: bhecquet bhecquet Date: Thu, 28 Sep 2023 16:53:18 +0000 Subject: [PATCH 02/23] Prevent a session to be created while its timeout has expired --- .../local/LocalNewSessionQueue.java | 29 ++++++++-- .../local/LocalNewSessionQueueTest.java | 58 ++++++++++++++++++- 2 files changed, 81 insertions(+), 6 deletions(-) diff --git a/java/src/org/openqa/selenium/grid/sessionqueue/local/LocalNewSessionQueue.java b/java/src/org/openqa/selenium/grid/sessionqueue/local/LocalNewSessionQueue.java index 5a11c610484cf..55127e4c0973a 100644 --- a/java/src/org/openqa/selenium/grid/sessionqueue/local/LocalNewSessionQueue.java +++ b/java/src/org/openqa/selenium/grid/sessionqueue/local/LocalNewSessionQueue.java @@ -215,16 +215,20 @@ public HttpResponse addToQueue(SessionRequest request) { try { LOG.info(String.format("%s: start waiting session creation", request.getRequestId())); boolean sessionCreated = data.latch.await(requestTimeout.toMillis(), MILLISECONDS); + + // do not allow to retry once the timeout expired + data.setNoMoreRetry(true); + if (!(sessionCreated || isRequestInQueue(request.getRequestId()))) { LOG.info(String.format("%s: a bit more time", request.getRequestId())); - sessionCreated = data.latch.await(5000, MILLISECONDS); + sessionCreated = data.latch.await(15000, MILLISECONDS); } if (sessionCreated) { - LOG.info(String.format("%s: session created", request.getRequestId())); + LOG.info(String.format("%s: we got a result", request.getRequestId())); result = data.result; } else { - LOG.info(String.format("%s: session not created", request.getRequestId())); + LOG.info(String.format("%s: no result in the expected time", request.getRequestId())); result = Either.left(new SessionNotCreatedException("New session request timed out")); } } catch (InterruptedException e) { @@ -240,8 +244,8 @@ public HttpResponse addToQueue(SessionRequest request) { Lock writeLock = this.lock.writeLock(); writeLock.lock(); try { - requests.remove(request.getRequestId()); queue.remove(request); + contexts.remove(request.getRequestId()); } finally { writeLock.unlock(); } @@ -294,9 +298,15 @@ public boolean retryAddToQueue(SessionRequest request) { Lock writeLock = lock.writeLock(); writeLock.lock(); try { + if (!requests.containsKey(request.getRequestId())) { return false; - } + } else if (requests.get(request.getRequestId()).noMoreRetry) { + // as we try to re-add a session request that has already expired, force session timeout + complete(request.getRequestId(), Either.left(new SessionNotCreatedException("Timed out creating session"))); + return false; + } + if (queue.contains(request)) { // No need to re-add this @@ -475,6 +485,7 @@ private class Data { private final CountDownLatch latch = new CountDownLatch(1); public Either result; private boolean complete; + private boolean noMoreRetry = false; public Data(Instant enqueued) { this.endTime = enqueued.plus(requestTimeout); @@ -490,5 +501,13 @@ public synchronized void setResult( complete = true; latch.countDown(); } + + public void setNoMoreRetry(boolean noMoreRetry) { + this.noMoreRetry = noMoreRetry; + } + + public boolean getNoMoreRetry() { + return this.noMoreRetry; + } } } diff --git a/java/test/org/openqa/selenium/grid/sessionqueue/local/LocalNewSessionQueueTest.java b/java/test/org/openqa/selenium/grid/sessionqueue/local/LocalNewSessionQueueTest.java index 9feb1a7d80235..7308ad2f145c2 100644 --- a/java/test/org/openqa/selenium/grid/sessionqueue/local/LocalNewSessionQueueTest.java +++ b/java/test/org/openqa/selenium/grid/sessionqueue/local/LocalNewSessionQueueTest.java @@ -35,6 +35,7 @@ import java.net.URISyntaxException; import java.time.Duration; import java.time.Instant; +import java.time.LocalDateTime; import java.util.HashMap; import java.util.LinkedHashSet; import java.util.List; @@ -261,7 +262,7 @@ void shouldBeAbleToAddToQueueWithTimeoutAndGetValidResponse(Supplier s .getBytes(UTF_8)); try { - Thread.sleep(2000); // simulate session creation delay + Thread.sleep(10000); // simulate session creation delay } catch (InterruptedException ignore) { } queue.complete( @@ -275,6 +276,61 @@ void shouldBeAbleToAddToQueueWithTimeoutAndGetValidResponse(Supplier s assertEquals(HTTP_OK, httpResponse.getStatus()); } + + @ParameterizedTest + @MethodSource("data") + void shouldBeAbleToAddToQueueWithTimeoutDoNotCreateSessionAfterTimeout(Supplier supplier) { + setup(supplier); + + SessionRequest sessionRequestWithTimeout = + new SessionRequest( + new RequestId(UUID.randomUUID()), + Instant.now(), + Set.of(W3C), + Set.of(CAPS), + Map.of(), + Map.of()); + + AtomicBoolean isPresent = new AtomicBoolean(false); + + new Thread( + () -> { + waitUntilAddedToQueue(sessionRequestWithTimeout); + isPresent.set(true); + + Capabilities capabilities = new ImmutableCapabilities("browserName", "chrome"); + + try { + Thread.sleep(4000); // simulate session waiting in queue + } catch (InterruptedException ignore) { + } + + // remove request from queue + Map stereotypes = new HashMap<>(); + stereotypes.put(new ImmutableCapabilities("browserName", "cheese"), 1L); + queue.getNextAvailable(stereotypes); + try { + Thread.sleep(2000); // wait to go past the request session timeout + } catch (InterruptedException ignore) { + } + + // LocalDistributor could not distribute the session, add it back to queue + // it should not be re-added to queue and send back error on session creation + queue.retryAddToQueue(sessionRequestWithTimeout); + + }) + .start(); +System.out.println("coucou"); + LocalDateTime start = LocalDateTime.now(); + HttpResponse httpResponse = queue.addToQueue(sessionRequestWithTimeout); + + // check we do not wait more than necessary + assertThat(LocalDateTime.now().minusSeconds(10).isBefore(start)).isTrue(); + + assertThat(isPresent.get()).isTrue(); + assertEquals(HTTP_INTERNAL_ERROR, httpResponse.getStatus()); + } + @ParameterizedTest @MethodSource("data") void shouldBeAbleToAddToQueueWithTimeoutAndTimeoutResponse(Supplier supplier) { From 26976c2b3cd9c4cfc82ca2c3defa936aab03030a Mon Sep 17 00:00:00 2001 From: bhecquet bhecquet Date: Fri, 29 Sep 2023 07:18:17 +0000 Subject: [PATCH 03/23] restore missing "remove" --- .../selenium/grid/sessionqueue/local/LocalNewSessionQueue.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/java/src/org/openqa/selenium/grid/sessionqueue/local/LocalNewSessionQueue.java b/java/src/org/openqa/selenium/grid/sessionqueue/local/LocalNewSessionQueue.java index 55127e4c0973a..1951f273f0854 100644 --- a/java/src/org/openqa/selenium/grid/sessionqueue/local/LocalNewSessionQueue.java +++ b/java/src/org/openqa/selenium/grid/sessionqueue/local/LocalNewSessionQueue.java @@ -244,8 +244,8 @@ public HttpResponse addToQueue(SessionRequest request) { Lock writeLock = this.lock.writeLock(); writeLock.lock(); try { + requests.remove(request.getRequestId()); queue.remove(request); - contexts.remove(request.getRequestId()); } finally { writeLock.unlock(); } From 62de717a067a47007ba461af6aa037054ad24576 Mon Sep 17 00:00:00 2001 From: bhecquet bhecquet Date: Fri, 29 Sep 2023 08:52:24 +0000 Subject: [PATCH 04/23] Close timedout session on creation to prevent them to be staled --- .../distributor/local/LocalDistributor.java | 25 ++++++++ .../IsSessionRequestTimedOut.java | 63 +++++++++++++++++++ .../grid/sessionqueue/NewSessionQueue.java | 5 ++ .../local/LocalNewSessionQueue.java | 41 +++++++----- .../remote/RemoteNewSessionQueue.java | 18 ++++++ .../local/LocalNewSessionQueueTest.java | 2 +- 6 files changed, 137 insertions(+), 17 deletions(-) create mode 100644 java/src/org/openqa/selenium/grid/sessionqueue/IsSessionRequestTimedOut.java diff --git a/java/src/org/openqa/selenium/grid/distributor/local/LocalDistributor.java b/java/src/org/openqa/selenium/grid/distributor/local/LocalDistributor.java index 91203fdc0b82c..9a465f818c4a4 100644 --- a/java/src/org/openqa/selenium/grid/distributor/local/LocalDistributor.java +++ b/java/src/org/openqa/selenium/grid/distributor/local/LocalDistributor.java @@ -43,6 +43,7 @@ import java.util.List; import java.util.Map; import java.util.Set; +import java.util.Optional; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.Executor; import java.util.concurrent.Executors; @@ -840,7 +841,31 @@ private void handleNewSessionRequest(SessionRequest sessionRequest) { } sessionQueue.complete(reqId, response); + // is session request has timed out, tell the node to remove the session, so that it's not staled + if (sessionQueue.isSessionRequestTimedOut(reqId) && response.isRight()) { + LOG.info("Stopping expired session"); + URI nodeURI = response.right().getSession().getUri(); + Node node = getNodeFromURI(nodeURI); + node.stop(response.right().getSession().getId()); + } + } + } + } + + protected Node getNodeFromURI(URI uri) { + Lock readLock = this.lock.readLock(); + readLock.lock(); + try { + Optional nodeStatus = model.getSnapshot().stream() + .filter(node -> node.getExternalUri().equals(uri)) + .findFirst(); + if (nodeStatus.isPresent()) { + return nodes.get(nodeStatus.get().getNodeId()); + } else { + return null; } + } finally { + readLock.unlock(); } } } diff --git a/java/src/org/openqa/selenium/grid/sessionqueue/IsSessionRequestTimedOut.java b/java/src/org/openqa/selenium/grid/sessionqueue/IsSessionRequestTimedOut.java new file mode 100644 index 0000000000000..7847614c03e57 --- /dev/null +++ b/java/src/org/openqa/selenium/grid/sessionqueue/IsSessionRequestTimedOut.java @@ -0,0 +1,63 @@ +// 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.sessionqueue; + +import static java.util.Collections.singletonMap; +import static org.openqa.selenium.remote.http.Contents.asJson; +import static org.openqa.selenium.remote.tracing.HttpTracing.newSpanAsChildOf; +import static org.openqa.selenium.remote.tracing.Tags.HTTP_REQUEST; +import static org.openqa.selenium.remote.tracing.Tags.HTTP_RESPONSE; + +import org.openqa.selenium.grid.data.RequestId; +import org.openqa.selenium.grid.data.SessionRequest; +import org.openqa.selenium.internal.Require; +import org.openqa.selenium.remote.http.Contents; +import org.openqa.selenium.remote.http.HttpHandler; +import org.openqa.selenium.remote.http.HttpRequest; +import org.openqa.selenium.remote.http.HttpResponse; +import org.openqa.selenium.remote.tracing.AttributeKey; +import org.openqa.selenium.remote.tracing.Span; +import org.openqa.selenium.remote.tracing.Tracer; + +class IsSessionRequestTimedOut implements HttpHandler { + + private final Tracer tracer; + private final NewSessionQueue newSessionQueue; + private final RequestId id; + + IsSessionRequestTimedOut(Tracer tracer, NewSessionQueue newSessionQueue, RequestId id) { + this.tracer = Require.nonNull("Tracer", tracer); + this.newSessionQueue = Require.nonNull("New Session Queue", newSessionQueue); + this.id = id; + } + + @Override + public HttpResponse execute(HttpRequest req) { + try (Span span = newSpanAsChildOf(tracer, req, "sessionqueue.issessionrequesttimeout")) { + HTTP_REQUEST.accept(span, req); + + boolean value = newSessionQueue.isSessionRequestTimedOut(id); + + HttpResponse response = new HttpResponse().setContent(asJson(singletonMap("value", value))); + + HTTP_RESPONSE.accept(span, response); + + return response; + } + } +} diff --git a/java/src/org/openqa/selenium/grid/sessionqueue/NewSessionQueue.java b/java/src/org/openqa/selenium/grid/sessionqueue/NewSessionQueue.java index 172716585f5fc..2c007bcfe4f64 100644 --- a/java/src/org/openqa/selenium/grid/sessionqueue/NewSessionQueue.java +++ b/java/src/org/openqa/selenium/grid/sessionqueue/NewSessionQueue.java @@ -74,6 +74,9 @@ protected NewSessionQueue(Tracer tracer, Secret registrationSecret) { post("/se/grid/newsessionqueue/session/{requestId}/retry") .to(params -> new AddBackToSessionQueue(tracer, this, requestIdFrom(params))) .with(requiresSecret), + get("/se/grid/newsessionqueue/session/{requestId}/istimeout") + .to(params -> new IsSessionRequestTimedOut(tracer, this, requestIdFrom(params))) + .with(requiresSecret), post("/se/grid/newsessionqueue/session/{requestId}/failure") .to(params -> new SessionNotCreated(tracer, this, requestIdFrom(params))) .with(requiresSecret), @@ -109,6 +112,8 @@ private RequestId requestIdFrom(Map params) { public abstract Optional remove(RequestId reqId); + public abstract boolean isSessionRequestTimedOut(RequestId reqId); + public abstract List getNextAvailable(Map stereotypes); public abstract void complete( diff --git a/java/src/org/openqa/selenium/grid/sessionqueue/local/LocalNewSessionQueue.java b/java/src/org/openqa/selenium/grid/sessionqueue/local/LocalNewSessionQueue.java index 1951f273f0854..8e2d9cb414b28 100644 --- a/java/src/org/openqa/selenium/grid/sessionqueue/local/LocalNewSessionQueue.java +++ b/java/src/org/openqa/selenium/grid/sessionqueue/local/LocalNewSessionQueue.java @@ -215,14 +215,11 @@ public HttpResponse addToQueue(SessionRequest request) { try { LOG.info(String.format("%s: start waiting session creation", request.getRequestId())); boolean sessionCreated = data.latch.await(requestTimeout.toMillis(), MILLISECONDS); - - // do not allow to retry once the timeout expired - data.setNoMoreRetry(true); - if (!(sessionCreated || isRequestInQueue(request.getRequestId()))) { + /*if (!(sessionCreated || isRequestInQueue(request.getRequestId()))) { LOG.info(String.format("%s: a bit more time", request.getRequestId())); sessionCreated = data.latch.await(15000, MILLISECONDS); - } + }*/ if (sessionCreated) { LOG.info(String.format("%s: we got a result", request.getRequestId())); @@ -241,14 +238,14 @@ public HttpResponse addToQueue(SessionRequest request) { new SessionNotCreatedException("An error occurred creating the session", e)); } - Lock writeLock = this.lock.writeLock(); + /*Lock writeLock = this.lock.writeLock(); writeLock.lock(); try { requests.remove(request.getRequestId()); queue.remove(request); } finally { writeLock.unlock(); - } + }*/ HttpResponse res = new HttpResponse(); if (result.isRight()) { @@ -301,7 +298,7 @@ public boolean retryAddToQueue(SessionRequest request) { if (!requests.containsKey(request.getRequestId())) { return false; - } else if (requests.get(request.getRequestId()).noMoreRetry) { + } else if (isTimedOut(Instant.now(), requests.get(request.getRequestId()))) { // as we try to re-add a session request that has already expired, force session timeout complete(request.getRequestId(), Either.left(new SessionNotCreatedException("Timed out creating session"))); return false; @@ -460,6 +457,26 @@ public List getQueueContents() { } } + @Override + public boolean isSessionRequestTimedOut(RequestId reqId) { + Require.nonNull("Request ID", reqId); + + Lock readLock = lock.readLock(); + readLock.lock(); + + try { + Data data = requests.get(reqId); + + if (data == null) { + return true; + } else { + return isTimedOut(Instant.now(), data); + } + } finally { + readLock.unlock(); + } + } + @ManagedAttribute(name = "NewSessionQueueSize") public int getQueueSize() { return queue.size(); @@ -485,7 +502,6 @@ private class Data { private final CountDownLatch latch = new CountDownLatch(1); public Either result; private boolean complete; - private boolean noMoreRetry = false; public Data(Instant enqueued) { this.endTime = enqueued.plus(requestTimeout); @@ -502,12 +518,5 @@ public synchronized void setResult( latch.countDown(); } - public void setNoMoreRetry(boolean noMoreRetry) { - this.noMoreRetry = noMoreRetry; - } - - public boolean getNoMoreRetry() { - return this.noMoreRetry; - } } } diff --git a/java/src/org/openqa/selenium/grid/sessionqueue/remote/RemoteNewSessionQueue.java b/java/src/org/openqa/selenium/grid/sessionqueue/remote/RemoteNewSessionQueue.java index e80f0287433d4..1e1c85f34f218 100644 --- a/java/src/org/openqa/selenium/grid/sessionqueue/remote/RemoteNewSessionQueue.java +++ b/java/src/org/openqa/selenium/grid/sessionqueue/remote/RemoteNewSessionQueue.java @@ -136,6 +136,24 @@ public Optional remove(RequestId reqId) { return Optional.empty(); } + @Override + public boolean isSessionRequestTimedOut(RequestId reqId) { + Require.nonNull("Session request id", reqId); + + HttpRequest upstream = + new HttpRequest( + GET, + String.format("/se/grid/newsessionqueue/session/%s/istimeout", reqId)); + HttpTracing.inject(tracer, tracer.getCurrentContext(), upstream); + HttpResponse response = client.with(addSecret).execute(upstream); + if (response.isSuccessful()) { + return Values.get(response, Boolean.class); + } else { + return true; + } + + } + @Override public List getNextAvailable(Map stereotypes) { Require.nonNull("Stereotypes", stereotypes); diff --git a/java/test/org/openqa/selenium/grid/sessionqueue/local/LocalNewSessionQueueTest.java b/java/test/org/openqa/selenium/grid/sessionqueue/local/LocalNewSessionQueueTest.java index 7308ad2f145c2..53f69fe1cfefc 100644 --- a/java/test/org/openqa/selenium/grid/sessionqueue/local/LocalNewSessionQueueTest.java +++ b/java/test/org/openqa/selenium/grid/sessionqueue/local/LocalNewSessionQueueTest.java @@ -320,7 +320,7 @@ void shouldBeAbleToAddToQueueWithTimeoutDoNotCreateSessionAfterTimeout(Supplier< }) .start(); -System.out.println("coucou"); + LocalDateTime start = LocalDateTime.now(); HttpResponse httpResponse = queue.addToQueue(sessionRequestWithTimeout); From 0b8b6a49d4e9cc6bfc1cd30a333b93e4676fe286 Mon Sep 17 00:00:00 2001 From: bhecquet bhecquet Date: Fri, 29 Sep 2023 13:47:36 +0000 Subject: [PATCH 05/23] Correct stopping of expired sessions --- .../selenium/grid/distributor/local/LocalDistributor.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/java/src/org/openqa/selenium/grid/distributor/local/LocalDistributor.java b/java/src/org/openqa/selenium/grid/distributor/local/LocalDistributor.java index 9a465f818c4a4..79d124bc725cc 100644 --- a/java/src/org/openqa/selenium/grid/distributor/local/LocalDistributor.java +++ b/java/src/org/openqa/selenium/grid/distributor/local/LocalDistributor.java @@ -840,9 +840,10 @@ private void handleNewSessionRequest(SessionRequest sessionRequest) { } } + boolean isSessionExpired = sessionQueue.isSessionRequestTimedOut(reqId); sessionQueue.complete(reqId, response); // is session request has timed out, tell the node to remove the session, so that it's not staled - if (sessionQueue.isSessionRequestTimedOut(reqId) && response.isRight()) { + if (isSessionExpired && response.isRight()) { LOG.info("Stopping expired session"); URI nodeURI = response.right().getSession().getUri(); Node node = getNodeFromURI(nodeURI); From 868d9e2c612da27bd9b91f8b0a12a960837326eb Mon Sep 17 00:00:00 2001 From: bhecquet bhecquet Date: Mon, 2 Oct 2023 07:29:21 +0000 Subject: [PATCH 06/23] Remove logs for final commit --- .../grid/distributor/local/LocalDistributor.java | 6 ++---- .../grid/sessionqueue/local/LocalNewSessionQueue.java | 10 +--------- 2 files changed, 3 insertions(+), 13 deletions(-) diff --git a/java/src/org/openqa/selenium/grid/distributor/local/LocalDistributor.java b/java/src/org/openqa/selenium/grid/distributor/local/LocalDistributor.java index 79d124bc725cc..d242e42e7f12f 100644 --- a/java/src/org/openqa/selenium/grid/distributor/local/LocalDistributor.java +++ b/java/src/org/openqa/selenium/grid/distributor/local/LocalDistributor.java @@ -507,7 +507,7 @@ protected Set getAvailableNodes() { public Either newSession( SessionRequest request) throws SessionNotCreatedException { Require.nonNull("Requests to process", request); - LOG.info(String.format("%s: new session", request.getRequestId())); + Span span = tracer.getCurrentContext().createSpan("distributor.new_session"); AttributeMap attributeMap = tracer.createAttributeMap(); try { @@ -560,7 +560,6 @@ public Either newSession( try { - LOG.info(String.format("%s: new session => start creating", request.getRequestId())); CreateSessionResponse response = startSession(selectedSlot, singleRequest); sessions.add(response.getSession()); model.setSession(selectedSlot, response.getSession()); @@ -574,7 +573,7 @@ public Either newSession( CAPABILITIES_EVENT.accept(attributeMap, sessionCaps); span.setAttribute(SESSION_URI.getKey(), sessionUri); attributeMap.put(SESSION_URI.getKey(), sessionUri); - LOG.info(String.format("%s: new session => created", request.getRequestId())); + String sessionCreatedMessage = "Session created by the Distributor"; span.addEvent(sessionCreatedMessage, attributeMap); LOG.info( @@ -583,7 +582,6 @@ public Either newSession( return Either.right(response); } catch (SessionNotCreatedException e) { - LOG.info(String.format("%s: new session => not created", request.getRequestId())); model.setSession(selectedSlot, null); lastFailure = e; } diff --git a/java/src/org/openqa/selenium/grid/sessionqueue/local/LocalNewSessionQueue.java b/java/src/org/openqa/selenium/grid/sessionqueue/local/LocalNewSessionQueue.java index 8e2d9cb414b28..7b1d980ac1d0d 100644 --- a/java/src/org/openqa/selenium/grid/sessionqueue/local/LocalNewSessionQueue.java +++ b/java/src/org/openqa/selenium/grid/sessionqueue/local/LocalNewSessionQueue.java @@ -94,7 +94,6 @@ objectName = "org.seleniumhq.grid:type=SessionQueue,name=LocalSessionQueue", description = "New session queue") public class LocalNewSessionQueue extends NewSessionQueue implements Closeable { - private static final Logger LOG = Logger.getLogger(LocalNewSessionQueue.class.getName()); private static final String NAME = "Local New Session Queue"; private final SlotMatcher slotMatcher; private final Duration requestTimeout; @@ -213,19 +212,12 @@ public HttpResponse addToQueue(SessionRequest request) { Either result; try { - LOG.info(String.format("%s: start waiting session creation", request.getRequestId())); - boolean sessionCreated = data.latch.await(requestTimeout.toMillis(), MILLISECONDS); - /*if (!(sessionCreated || isRequestInQueue(request.getRequestId()))) { - LOG.info(String.format("%s: a bit more time", request.getRequestId())); - sessionCreated = data.latch.await(15000, MILLISECONDS); - }*/ + boolean sessionCreated = data.latch.await(requestTimeout.toMillis(), MILLISECONDS); if (sessionCreated) { - LOG.info(String.format("%s: we got a result", request.getRequestId())); result = data.result; } else { - LOG.info(String.format("%s: no result in the expected time", request.getRequestId())); result = Either.left(new SessionNotCreatedException("New session request timed out")); } } catch (InterruptedException e) { From 18a706e00ce156a154ea55ce8359173a19b92bef Mon Sep 17 00:00:00 2001 From: bhecquet bhecquet Date: Mon, 2 Oct 2023 12:54:33 +0000 Subject: [PATCH 07/23] removed a test that is now useless as request session timeout is now strict --- .../local/LocalNewSessionQueueTest.java | 68 ------------------- 1 file changed, 68 deletions(-) diff --git a/java/test/org/openqa/selenium/grid/sessionqueue/local/LocalNewSessionQueueTest.java b/java/test/org/openqa/selenium/grid/sessionqueue/local/LocalNewSessionQueueTest.java index 53f69fe1cfefc..2ebceeb71e500 100644 --- a/java/test/org/openqa/selenium/grid/sessionqueue/local/LocalNewSessionQueueTest.java +++ b/java/test/org/openqa/selenium/grid/sessionqueue/local/LocalNewSessionQueueTest.java @@ -209,74 +209,6 @@ void shouldBeAbleToAddToQueueAndGetValidResponse(Supplier supplier) { assertEquals(HTTP_OK, httpResponse.getStatus()); } - @ParameterizedTest - @MethodSource("data") - void shouldBeAbleToAddToQueueWithTimeoutAndGetValidResponse(Supplier supplier) { - setup(supplier); - - SessionRequest sessionRequestWithTimeout = - new SessionRequest( - new RequestId(UUID.randomUUID()), - Instant.now(), - Set.of(W3C), - Set.of(CAPS), - Map.of(), - Map.of()); - - AtomicBoolean isPresent = new AtomicBoolean(false); - - new Thread( - () -> { - waitUntilAddedToQueue(sessionRequestWithTimeout); - isPresent.set(true); - - Capabilities capabilities = new ImmutableCapabilities("browserName", "chrome"); - - try { - Thread.sleep(4000); // simulate session waiting in queue - } catch (InterruptedException ignore) { - } - - // remove request from queue - Map stereotypes = new HashMap<>(); - stereotypes.put(new ImmutableCapabilities("browserName", "cheese"), 1L); - queue.getNextAvailable(stereotypes); - - SessionId sessionId = new SessionId("123"); - Session session = - new Session( - sessionId, - URI.create("https://example.com"), - CAPS, - capabilities, - Instant.now()); - CreateSessionResponse sessionResponse = - new CreateSessionResponse( - session, - JSON.toJson( - ImmutableMap.of( - "value", - ImmutableMap.of( - "sessionId", sessionId, - "capabilities", capabilities))) - .getBytes(UTF_8)); - - try { - Thread.sleep(10000); // simulate session creation delay - } catch (InterruptedException ignore) { - } - queue.complete( - sessionRequestWithTimeout.getRequestId(), Either.right(sessionResponse)); - }) - .start(); - - HttpResponse httpResponse = queue.addToQueue(sessionRequestWithTimeout); - - assertThat(isPresent.get()).isTrue(); - assertEquals(HTTP_OK, httpResponse.getStatus()); - } - - @ParameterizedTest @MethodSource("data") void shouldBeAbleToAddToQueueWithTimeoutDoNotCreateSessionAfterTimeout(Supplier supplier) { From 19d823c7629cf3ba528393ce98e3f99b7211bc7e Mon Sep 17 00:00:00 2001 From: bhecquet bhecquet Date: Mon, 2 Oct 2023 14:27:34 +0000 Subject: [PATCH 08/23] Simplify session stopping if timeout expires --- .../distributor/local/LocalDistributor.java | 5 +- .../IsSessionRequestTimedOut.java | 63 ------------------- .../grid/sessionqueue/NewSessionQueue.java | 7 +-- .../grid/sessionqueue/SessionCreated.java | 7 ++- .../grid/sessionqueue/SessionNotCreated.java | 4 +- .../local/LocalNewSessionQueue.java | 32 +++------- .../remote/RemoteNewSessionQueue.java | 27 +++----- 7 files changed, 28 insertions(+), 117 deletions(-) delete mode 100644 java/src/org/openqa/selenium/grid/sessionqueue/IsSessionRequestTimedOut.java diff --git a/java/src/org/openqa/selenium/grid/distributor/local/LocalDistributor.java b/java/src/org/openqa/selenium/grid/distributor/local/LocalDistributor.java index d24b66de0660e..a99835ee343b4 100644 --- a/java/src/org/openqa/selenium/grid/distributor/local/LocalDistributor.java +++ b/java/src/org/openqa/selenium/grid/distributor/local/LocalDistributor.java @@ -840,10 +840,9 @@ private void handleNewSessionRequest(SessionRequest sessionRequest) { } } - boolean isSessionExpired = sessionQueue.isSessionRequestTimedOut(reqId); - sessionQueue.complete(reqId, response); + boolean isSessionValid = sessionQueue.complete(reqId, response); // is session request has timed out, tell the node to remove the session, so that it's not staled - if (isSessionExpired && response.isRight()) { + if (!isSessionValid && response.isRight()) { LOG.info("Stopping expired session"); URI nodeURI = response.right().getSession().getUri(); Node node = getNodeFromURI(nodeURI); diff --git a/java/src/org/openqa/selenium/grid/sessionqueue/IsSessionRequestTimedOut.java b/java/src/org/openqa/selenium/grid/sessionqueue/IsSessionRequestTimedOut.java deleted file mode 100644 index 7847614c03e57..0000000000000 --- a/java/src/org/openqa/selenium/grid/sessionqueue/IsSessionRequestTimedOut.java +++ /dev/null @@ -1,63 +0,0 @@ -// 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.sessionqueue; - -import static java.util.Collections.singletonMap; -import static org.openqa.selenium.remote.http.Contents.asJson; -import static org.openqa.selenium.remote.tracing.HttpTracing.newSpanAsChildOf; -import static org.openqa.selenium.remote.tracing.Tags.HTTP_REQUEST; -import static org.openqa.selenium.remote.tracing.Tags.HTTP_RESPONSE; - -import org.openqa.selenium.grid.data.RequestId; -import org.openqa.selenium.grid.data.SessionRequest; -import org.openqa.selenium.internal.Require; -import org.openqa.selenium.remote.http.Contents; -import org.openqa.selenium.remote.http.HttpHandler; -import org.openqa.selenium.remote.http.HttpRequest; -import org.openqa.selenium.remote.http.HttpResponse; -import org.openqa.selenium.remote.tracing.AttributeKey; -import org.openqa.selenium.remote.tracing.Span; -import org.openqa.selenium.remote.tracing.Tracer; - -class IsSessionRequestTimedOut implements HttpHandler { - - private final Tracer tracer; - private final NewSessionQueue newSessionQueue; - private final RequestId id; - - IsSessionRequestTimedOut(Tracer tracer, NewSessionQueue newSessionQueue, RequestId id) { - this.tracer = Require.nonNull("Tracer", tracer); - this.newSessionQueue = Require.nonNull("New Session Queue", newSessionQueue); - this.id = id; - } - - @Override - public HttpResponse execute(HttpRequest req) { - try (Span span = newSpanAsChildOf(tracer, req, "sessionqueue.issessionrequesttimeout")) { - HTTP_REQUEST.accept(span, req); - - boolean value = newSessionQueue.isSessionRequestTimedOut(id); - - HttpResponse response = new HttpResponse().setContent(asJson(singletonMap("value", value))); - - HTTP_RESPONSE.accept(span, response); - - return response; - } - } -} diff --git a/java/src/org/openqa/selenium/grid/sessionqueue/NewSessionQueue.java b/java/src/org/openqa/selenium/grid/sessionqueue/NewSessionQueue.java index 2c007bcfe4f64..aab89d92f945d 100644 --- a/java/src/org/openqa/selenium/grid/sessionqueue/NewSessionQueue.java +++ b/java/src/org/openqa/selenium/grid/sessionqueue/NewSessionQueue.java @@ -74,9 +74,6 @@ protected NewSessionQueue(Tracer tracer, Secret registrationSecret) { post("/se/grid/newsessionqueue/session/{requestId}/retry") .to(params -> new AddBackToSessionQueue(tracer, this, requestIdFrom(params))) .with(requiresSecret), - get("/se/grid/newsessionqueue/session/{requestId}/istimeout") - .to(params -> new IsSessionRequestTimedOut(tracer, this, requestIdFrom(params))) - .with(requiresSecret), post("/se/grid/newsessionqueue/session/{requestId}/failure") .to(params -> new SessionNotCreated(tracer, this, requestIdFrom(params))) .with(requiresSecret), @@ -112,11 +109,9 @@ private RequestId requestIdFrom(Map params) { public abstract Optional remove(RequestId reqId); - public abstract boolean isSessionRequestTimedOut(RequestId reqId); - public abstract List getNextAvailable(Map stereotypes); - public abstract void complete( + public abstract boolean complete( RequestId reqId, Either result); public abstract int clearQueue(); diff --git a/java/src/org/openqa/selenium/grid/sessionqueue/SessionCreated.java b/java/src/org/openqa/selenium/grid/sessionqueue/SessionCreated.java index 3b01b5b1110fe..a41f3fb096de1 100644 --- a/java/src/org/openqa/selenium/grid/sessionqueue/SessionCreated.java +++ b/java/src/org/openqa/selenium/grid/sessionqueue/SessionCreated.java @@ -17,6 +17,8 @@ package org.openqa.selenium.grid.sessionqueue; +import static java.util.Collections.singletonMap; +import static org.openqa.selenium.remote.http.Contents.asJson; import static org.openqa.selenium.remote.tracing.HttpTracing.newSpanAsChildOf; import static org.openqa.selenium.remote.tracing.Tags.HTTP_REQUEST; import static org.openqa.selenium.remote.tracing.Tags.HTTP_RESPONSE; @@ -50,9 +52,10 @@ public HttpResponse execute(HttpRequest req) throws UncheckedIOException { HTTP_REQUEST.accept(span, req); CreateSessionResponse response = Contents.fromJson(req, CreateSessionResponse.class); - queue.complete(requestId, Either.right(response)); + boolean isSessionValid = queue.complete(requestId, Either.right(response)); + + HttpResponse res = new HttpResponse().setContent(asJson(singletonMap("value", isSessionValid))); - HttpResponse res = new HttpResponse(); HTTP_RESPONSE.accept(span, res); return res; } diff --git a/java/src/org/openqa/selenium/grid/sessionqueue/SessionNotCreated.java b/java/src/org/openqa/selenium/grid/sessionqueue/SessionNotCreated.java index 63b7c7dac8317..8f82bbb419a9a 100644 --- a/java/src/org/openqa/selenium/grid/sessionqueue/SessionNotCreated.java +++ b/java/src/org/openqa/selenium/grid/sessionqueue/SessionNotCreated.java @@ -17,6 +17,8 @@ package org.openqa.selenium.grid.sessionqueue; +import static java.util.Collections.singletonMap; +import static org.openqa.selenium.remote.http.Contents.asJson; import static org.openqa.selenium.remote.tracing.HttpTracing.newSpanAsChildOf; import static org.openqa.selenium.remote.tracing.Tags.HTTP_REQUEST; import static org.openqa.selenium.remote.tracing.Tags.HTTP_RESPONSE; @@ -54,7 +56,7 @@ public HttpResponse execute(HttpRequest req) throws UncheckedIOException { SessionNotCreatedException exception = new SessionNotCreatedException(message); queue.complete(requestId, Either.left(exception)); - HttpResponse res = new HttpResponse(); + HttpResponse res = new HttpResponse().setContent(asJson(singletonMap("value", false))); HTTP_RESPONSE.accept(span, res); return res; } diff --git a/java/src/org/openqa/selenium/grid/sessionqueue/local/LocalNewSessionQueue.java b/java/src/org/openqa/selenium/grid/sessionqueue/local/LocalNewSessionQueue.java index 7b1d980ac1d0d..dbb6ebd6e60f5 100644 --- a/java/src/org/openqa/selenium/grid/sessionqueue/local/LocalNewSessionQueue.java +++ b/java/src/org/openqa/selenium/grid/sessionqueue/local/LocalNewSessionQueue.java @@ -381,8 +381,11 @@ public List getNextAvailable(Map stereotypes } } + /** + * Returns true if the session is still valid (not timed out) + */ @Override - public void complete( + public boolean complete( RequestId reqId, Either result) { Require.nonNull("New session request", reqId); Require.nonNull("Result", result); @@ -391,6 +394,7 @@ public void complete( Lock readLock = lock.readLock(); readLock.lock(); Data data; + boolean isSessionTimedOut = false; try { data = requests.get(reqId); } finally { @@ -398,7 +402,9 @@ public void complete( } if (data == null) { - return; + return false; + } else { + isSessionTimedOut = isTimedOut(Instant.now(), data); } Lock writeLock = lock.writeLock(); @@ -412,6 +418,8 @@ public void complete( } data.setResult(result); + + return !isSessionTimedOut; } } @@ -449,26 +457,6 @@ public List getQueueContents() { } } - @Override - public boolean isSessionRequestTimedOut(RequestId reqId) { - Require.nonNull("Request ID", reqId); - - Lock readLock = lock.readLock(); - readLock.lock(); - - try { - Data data = requests.get(reqId); - - if (data == null) { - return true; - } else { - return isTimedOut(Instant.now(), data); - } - } finally { - readLock.unlock(); - } - } - @ManagedAttribute(name = "NewSessionQueueSize") public int getQueueSize() { return queue.size(); diff --git a/java/src/org/openqa/selenium/grid/sessionqueue/remote/RemoteNewSessionQueue.java b/java/src/org/openqa/selenium/grid/sessionqueue/remote/RemoteNewSessionQueue.java index 1e1c85f34f218..bff5793b093a2 100644 --- a/java/src/org/openqa/selenium/grid/sessionqueue/remote/RemoteNewSessionQueue.java +++ b/java/src/org/openqa/selenium/grid/sessionqueue/remote/RemoteNewSessionQueue.java @@ -136,24 +136,6 @@ public Optional remove(RequestId reqId) { return Optional.empty(); } - @Override - public boolean isSessionRequestTimedOut(RequestId reqId) { - Require.nonNull("Session request id", reqId); - - HttpRequest upstream = - new HttpRequest( - GET, - String.format("/se/grid/newsessionqueue/session/%s/istimeout", reqId)); - HttpTracing.inject(tracer, tracer.getCurrentContext(), upstream); - HttpResponse response = client.with(addSecret).execute(upstream); - if (response.isSuccessful()) { - return Values.get(response, Boolean.class); - } else { - return true; - } - - } - @Override public List getNextAvailable(Map stereotypes) { Require.nonNull("Stereotypes", stereotypes); @@ -172,7 +154,7 @@ public List getNextAvailable(Map stereotypes } @Override - public void complete( + public boolean complete( RequestId reqId, Either result) { Require.nonNull("Request ID", reqId); Require.nonNull("Result", result); @@ -189,7 +171,12 @@ public void complete( } HttpTracing.inject(tracer, tracer.getCurrentContext(), upstream); - client.with(addSecret).execute(upstream); + HttpResponse response = client.with(addSecret).execute(upstream); + if (response.isSuccessful()) { + return Values.get(response, Boolean.class); + } else { + return false; + } } @Override From 2c84bee07cef6a3b2faa0162e85531a6a6f44ddc Mon Sep 17 00:00:00 2001 From: bhecquet bhecquet Date: Mon, 2 Oct 2023 14:58:00 +0000 Subject: [PATCH 09/23] Add some tests --- .../local/LocalNewSessionQueue.java | 9 -- .../local/LocalNewSessionQueueTest.java | 92 +++++++++++++++++++ 2 files changed, 92 insertions(+), 9 deletions(-) diff --git a/java/src/org/openqa/selenium/grid/sessionqueue/local/LocalNewSessionQueue.java b/java/src/org/openqa/selenium/grid/sessionqueue/local/LocalNewSessionQueue.java index dbb6ebd6e60f5..9bedbb3e26b14 100644 --- a/java/src/org/openqa/selenium/grid/sessionqueue/local/LocalNewSessionQueue.java +++ b/java/src/org/openqa/selenium/grid/sessionqueue/local/LocalNewSessionQueue.java @@ -230,15 +230,6 @@ public HttpResponse addToQueue(SessionRequest request) { new SessionNotCreatedException("An error occurred creating the session", e)); } - /*Lock writeLock = this.lock.writeLock(); - writeLock.lock(); - try { - requests.remove(request.getRequestId()); - queue.remove(request); - } finally { - writeLock.unlock(); - }*/ - HttpResponse res = new HttpResponse(); if (result.isRight()) { res.setContent(Contents.bytes(result.right().getDownstreamEncodedResponse())); diff --git a/java/test/org/openqa/selenium/grid/sessionqueue/local/LocalNewSessionQueueTest.java b/java/test/org/openqa/selenium/grid/sessionqueue/local/LocalNewSessionQueueTest.java index 2ebceeb71e500..7bc25ae840cf2 100644 --- a/java/test/org/openqa/selenium/grid/sessionqueue/local/LocalNewSessionQueueTest.java +++ b/java/test/org/openqa/selenium/grid/sessionqueue/local/LocalNewSessionQueueTest.java @@ -167,6 +167,97 @@ private void waitUntilAddedToQueue(SessionRequest request) { sessionRequestCapability.getRequestId().equals(r.getRequestId()))); } + @ParameterizedTest + @MethodSource("data") + void testCompleteWithCreatedSession(Supplier supplier) { + setup(supplier); + + AtomicBoolean isCompleted = new AtomicBoolean(false); + + new Thread( + () -> { + waitUntilAddedToQueue(sessionRequest); + + Capabilities capabilities = new ImmutableCapabilities("browserName", "chrome"); + SessionId sessionId = new SessionId("123"); + Session session = + new Session( + sessionId, + URI.create("https://example.com"), + CAPS, + capabilities, + Instant.now()); + CreateSessionResponse sessionResponse = + new CreateSessionResponse( + session, + JSON.toJson( + ImmutableMap.of( + "value", + ImmutableMap.of( + "sessionId", sessionId, + "capabilities", capabilities))) + .getBytes(UTF_8)); + + isCompleted.set(queue.complete(sessionRequest.getRequestId(), Either.right(sessionResponse))); + }) + .start(); + + HttpResponse httpResponse = queue.addToQueue(sessionRequest); + try { + Thread.sleep(400); // wait for thread to complete + } catch (InterruptedException ignore) { + } + assertThat(isCompleted.get()).isTrue(); + } + + + @ParameterizedTest + @MethodSource("data") + void testCompleteWithSessionInTimeout(Supplier supplier) { + setup(supplier); + + AtomicBoolean isCompleted = new AtomicBoolean(false); + + new Thread( + () -> { + waitUntilAddedToQueue(sessionRequest); + try { + Thread.sleep(5500); // simulate session long to create + } catch (InterruptedException ignore) { + } + Capabilities capabilities = new ImmutableCapabilities("browserName", "chrome"); + SessionId sessionId = new SessionId("123"); + Session session = + new Session( + sessionId, + URI.create("https://example.com"), + CAPS, + capabilities, + Instant.now()); + CreateSessionResponse sessionResponse = + new CreateSessionResponse( + session, + JSON.toJson( + ImmutableMap.of( + "value", + ImmutableMap.of( + "sessionId", sessionId, + "capabilities", capabilities))) + .getBytes(UTF_8)); + + isCompleted.set(queue.complete(sessionRequest.getRequestId(), Either.left(new SessionNotCreatedException("not created")))); + }) + .start(); + + HttpResponse httpResponse = queue.addToQueue(sessionRequest); + try { + Thread.sleep(400); // wait for thread to complete + } catch (InterruptedException ignore) { + } + + assertThat(isCompleted.get()).isFalse(); + } + @ParameterizedTest @MethodSource("data") void shouldBeAbleToAddToQueueAndGetValidResponse(Supplier supplier) { @@ -320,6 +411,7 @@ void shouldBeAbleToAddToQueueWithTimeoutAndTimeoutResponse(Supplier su assertThat(isPresent.get()).isTrue(); assertEquals(HTTP_INTERNAL_ERROR, httpResponse.getStatus()); } + @ParameterizedTest @MethodSource("data") From 7adaac9b781329a5dcd19d0601f4f2a87a732e31 Mon Sep 17 00:00:00 2001 From: bhecquet bhecquet Date: Mon, 2 Oct 2023 15:01:14 +0000 Subject: [PATCH 10/23] remove logs --- java/src/org/openqa/selenium/grid/node/local/LocalNode.java | 1 - 1 file changed, 1 deletion(-) diff --git a/java/src/org/openqa/selenium/grid/node/local/LocalNode.java b/java/src/org/openqa/selenium/grid/node/local/LocalNode.java index df2e5aedd4466..72362ea618d46 100644 --- a/java/src/org/openqa/selenium/grid/node/local/LocalNode.java +++ b/java/src/org/openqa/selenium/grid/node/local/LocalNode.java @@ -401,7 +401,6 @@ public boolean isSupporting(Capabilities capabilities) { public Either newSession( CreateSessionRequest sessionRequest) { Require.nonNull("Session request", sessionRequest); - LOG.info("new session => start creating"); try (Span span = tracer.getCurrentContext().createSpan("node.new_session")) { AttributeMap attributeMap = tracer.createAttributeMap(); attributeMap.put(AttributeKey.LOGGER_CLASS.getKey(), getClass().getName()); From 40ae455e4f9536f021475dd325394630e99a871b Mon Sep 17 00:00:00 2001 From: bhecquet bhecquet Date: Tue, 3 Oct 2023 05:50:28 +0000 Subject: [PATCH 11/23] Return value directly --- .../grid/sessionqueue/remote/RemoteNewSessionQueue.java | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/java/src/org/openqa/selenium/grid/sessionqueue/remote/RemoteNewSessionQueue.java b/java/src/org/openqa/selenium/grid/sessionqueue/remote/RemoteNewSessionQueue.java index bff5793b093a2..f36cdd6bbfa72 100644 --- a/java/src/org/openqa/selenium/grid/sessionqueue/remote/RemoteNewSessionQueue.java +++ b/java/src/org/openqa/selenium/grid/sessionqueue/remote/RemoteNewSessionQueue.java @@ -172,11 +172,7 @@ public boolean complete( HttpTracing.inject(tracer, tracer.getCurrentContext(), upstream); HttpResponse response = client.with(addSecret).execute(upstream); - if (response.isSuccessful()) { - return Values.get(response, Boolean.class); - } else { - return false; - } + return Values.get(response, Boolean.class); } @Override From 89d98517a7c4e98852bcb5e9cc4d58b6f9d74229 Mon Sep 17 00:00:00 2001 From: bhecquet bhecquet Date: Tue, 3 Oct 2023 05:55:39 +0000 Subject: [PATCH 12/23] remove unused method --- .../sessionqueue/local/LocalNewSessionQueue.java | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/java/src/org/openqa/selenium/grid/sessionqueue/local/LocalNewSessionQueue.java b/java/src/org/openqa/selenium/grid/sessionqueue/local/LocalNewSessionQueue.java index 9bedbb3e26b14..1db529451ba20 100644 --- a/java/src/org/openqa/selenium/grid/sessionqueue/local/LocalNewSessionQueue.java +++ b/java/src/org/openqa/selenium/grid/sessionqueue/local/LocalNewSessionQueue.java @@ -324,19 +324,6 @@ public Optional remove(RequestId reqId) { } } - private boolean isRequestInQueue(RequestId requestId) { - Lock readLock = lock.readLock(); - readLock.lock(); - - try { - Optional result = - queue.stream().filter(req -> req.getRequestId().equals(requestId)).findAny(); - return result.isPresent(); - } finally { - readLock.unlock(); - } - } - @Override public List getNextAvailable(Map stereotypes) { Require.nonNull("Stereotypes", stereotypes); From 944618c2c5739a1b1e2abf59e02531b264b946d2 Mon Sep 17 00:00:00 2001 From: bhecquet bhecquet Date: Tue, 3 Oct 2023 05:59:23 +0000 Subject: [PATCH 13/23] Removed hard coded value --- .../openqa/selenium/grid/sessionqueue/SessionNotCreated.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/java/src/org/openqa/selenium/grid/sessionqueue/SessionNotCreated.java b/java/src/org/openqa/selenium/grid/sessionqueue/SessionNotCreated.java index 8f82bbb419a9a..ede4a04faf378 100644 --- a/java/src/org/openqa/selenium/grid/sessionqueue/SessionNotCreated.java +++ b/java/src/org/openqa/selenium/grid/sessionqueue/SessionNotCreated.java @@ -54,9 +54,9 @@ public HttpResponse execute(HttpRequest req) throws UncheckedIOException { String message = Contents.fromJson(req, String.class); SessionNotCreatedException exception = new SessionNotCreatedException(message); - queue.complete(requestId, Either.left(exception)); + boolean isSessionValid = queue.complete(requestId, Either.left(exception)); - HttpResponse res = new HttpResponse().setContent(asJson(singletonMap("value", false))); + HttpResponse res = new HttpResponse().setContent(asJson(singletonMap("value", isSessionValid))); HTTP_RESPONSE.accept(span, res); return res; } From 37c23aac3fafae20cd1e01d718172ad3ba4d1349 Mon Sep 17 00:00:00 2001 From: bhecquet bhecquet Date: Wed, 4 Oct 2023 08:16:31 +0000 Subject: [PATCH 14/23] correct according to comments --- .../distributor/local/LocalDistributor.java | 1 - .../selenium/grid/node/local/LocalNode.java | 1 + .../local/LocalNewSessionQueue.java | 19 +++++++++++++++---- 3 files changed, 16 insertions(+), 5 deletions(-) diff --git a/java/src/org/openqa/selenium/grid/distributor/local/LocalDistributor.java b/java/src/org/openqa/selenium/grid/distributor/local/LocalDistributor.java index a99835ee343b4..a77ca5fb54d6c 100644 --- a/java/src/org/openqa/selenium/grid/distributor/local/LocalDistributor.java +++ b/java/src/org/openqa/selenium/grid/distributor/local/LocalDistributor.java @@ -561,7 +561,6 @@ public Either newSession( new CreateSessionRequest(request.getDownstreamDialects(), caps, request.getMetadata()); try { - CreateSessionResponse response = startSession(selectedSlot, singleRequest); sessions.add(response.getSession()); model.setSession(selectedSlot, response.getSession()); diff --git a/java/src/org/openqa/selenium/grid/node/local/LocalNode.java b/java/src/org/openqa/selenium/grid/node/local/LocalNode.java index 72362ea618d46..b7ac7f4129bd4 100644 --- a/java/src/org/openqa/selenium/grid/node/local/LocalNode.java +++ b/java/src/org/openqa/selenium/grid/node/local/LocalNode.java @@ -401,6 +401,7 @@ public boolean isSupporting(Capabilities capabilities) { public Either newSession( CreateSessionRequest sessionRequest) { Require.nonNull("Session request", sessionRequest); + try (Span span = tracer.getCurrentContext().createSpan("node.new_session")) { AttributeMap attributeMap = tracer.createAttributeMap(); attributeMap.put(AttributeKey.LOGGER_CLASS.getKey(), getClass().getName()); diff --git a/java/src/org/openqa/selenium/grid/sessionqueue/local/LocalNewSessionQueue.java b/java/src/org/openqa/selenium/grid/sessionqueue/local/LocalNewSessionQueue.java index 1db529451ba20..18389f0a64ec8 100644 --- a/java/src/org/openqa/selenium/grid/sessionqueue/local/LocalNewSessionQueue.java +++ b/java/src/org/openqa/selenium/grid/sessionqueue/local/LocalNewSessionQueue.java @@ -94,6 +94,7 @@ objectName = "org.seleniumhq.grid:type=SessionQueue,name=LocalSessionQueue", description = "New session queue") public class LocalNewSessionQueue extends NewSessionQueue implements Closeable { + private static final String NAME = "Local New Session Queue"; private final SlotMatcher slotMatcher; private final Duration requestTimeout; @@ -230,6 +231,15 @@ public HttpResponse addToQueue(SessionRequest request) { new SessionNotCreatedException("An error occurred creating the session", e)); } + Lock writeLock = this.lock.writeLock(); + writeLock.lock(); + try { + requests.remove(request.getRequestId()); + queue.remove(request); + } finally { + writeLock.unlock(); + } + HttpResponse res = new HttpResponse(); if (result.isRight()) { res.setContent(Contents.bytes(result.right().getDownstreamEncodedResponse())); @@ -278,13 +288,14 @@ public boolean retryAddToQueue(SessionRequest request) { Lock writeLock = lock.writeLock(); writeLock.lock(); try { - if (!requests.containsKey(request.getRequestId())) { return false; - } else if (isTimedOut(Instant.now(), requests.get(request.getRequestId()))) { + } + if (isTimedOut(Instant.now(), requests.get(request.getRequestId()))) { // as we try to re-add a session request that has already expired, force session timeout - complete(request.getRequestId(), Either.left(new SessionNotCreatedException("Timed out creating session"))); - return false; + failDueToTimeout(request.getRequestId()); + // return true to avoid handleNewSessionRequest to call 'complete' an other time + return true; } From 708da750d451b78d96c65e29ad27ca712620087f Mon Sep 17 00:00:00 2001 From: bhecquet bhecquet Date: Wed, 4 Oct 2023 13:13:27 +0000 Subject: [PATCH 15/23] Correct according to comments --- .../distributor/local/LocalDistributor.java | 9 +++++--- .../local/LocalNewSessionQueueTest.java | 22 +++++++++---------- 2 files changed, 17 insertions(+), 14 deletions(-) diff --git a/java/src/org/openqa/selenium/grid/distributor/local/LocalDistributor.java b/java/src/org/openqa/selenium/grid/distributor/local/LocalDistributor.java index a77ca5fb54d6c..832bda1bb25ec 100644 --- a/java/src/org/openqa/selenium/grid/distributor/local/LocalDistributor.java +++ b/java/src/org/openqa/selenium/grid/distributor/local/LocalDistributor.java @@ -840,12 +840,15 @@ private void handleNewSessionRequest(SessionRequest sessionRequest) { } boolean isSessionValid = sessionQueue.complete(reqId, response); - // is session request has timed out, tell the node to remove the session, so that it's not staled + // If the session request has timed out, tell the Node to remove the session, so that does not stall if (!isSessionValid && response.isRight()) { - LOG.info("Stopping expired session"); + LOG.log( + Debug.getDebugLogLevel(), "Session for request {0} has been created but it has timed out, stopping it to avoid stalled browser", reqId.toString()); URI nodeURI = response.right().getSession().getUri(); Node node = getNodeFromURI(nodeURI); - node.stop(response.right().getSession().getId()); + if (node != null) { + node.stop(response.right().getSession().getId()); + } } } } diff --git a/java/test/org/openqa/selenium/grid/sessionqueue/local/LocalNewSessionQueueTest.java b/java/test/org/openqa/selenium/grid/sessionqueue/local/LocalNewSessionQueueTest.java index 7bc25ae840cf2..a70fb0cfe0594 100644 --- a/java/test/org/openqa/selenium/grid/sessionqueue/local/LocalNewSessionQueueTest.java +++ b/java/test/org/openqa/selenium/grid/sessionqueue/local/LocalNewSessionQueueTest.java @@ -21,6 +21,7 @@ import static java.net.HttpURLConnection.HTTP_OK; import static java.nio.charset.StandardCharsets.UTF_8; import static java.util.concurrent.TimeUnit.SECONDS; +import static java.util.concurrent.TimeUnit.MILLISECONDS; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.fail; import static org.junit.jupiter.api.Assertions.assertEquals; @@ -48,6 +49,7 @@ import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; import java.util.concurrent.Future; +import java.util.concurrent.CountDownLatch; import java.util.concurrent.TimeoutException; import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicInteger; @@ -169,10 +171,11 @@ private void waitUntilAddedToQueue(SessionRequest request) { @ParameterizedTest @MethodSource("data") - void testCompleteWithCreatedSession(Supplier supplier) { + void testCompleteWithCreatedSession(Supplier supplier) throws InterruptedException { setup(supplier); AtomicBoolean isCompleted = new AtomicBoolean(false); + CountDownLatch latch = new CountDownLatch(1); new Thread( () -> { @@ -199,24 +202,24 @@ void testCompleteWithCreatedSession(Supplier supplier) { .getBytes(UTF_8)); isCompleted.set(queue.complete(sessionRequest.getRequestId(), Either.right(sessionResponse))); + latch.countDown(); }) .start(); HttpResponse httpResponse = queue.addToQueue(sessionRequest); - try { - Thread.sleep(400); // wait for thread to complete - } catch (InterruptedException ignore) { - } + + assertThat(latch.await(1000, MILLISECONDS)).isTrue(); assertThat(isCompleted.get()).isTrue(); } @ParameterizedTest @MethodSource("data") - void testCompleteWithSessionInTimeout(Supplier supplier) { + void testCompleteWithSessionInTimeout(Supplier supplier) throws InterruptedException { setup(supplier); AtomicBoolean isCompleted = new AtomicBoolean(false); + CountDownLatch latch = new CountDownLatch(1); new Thread( () -> { @@ -246,15 +249,12 @@ void testCompleteWithSessionInTimeout(Supplier supplier) { .getBytes(UTF_8)); isCompleted.set(queue.complete(sessionRequest.getRequestId(), Either.left(new SessionNotCreatedException("not created")))); + latch.countDown(); }) .start(); HttpResponse httpResponse = queue.addToQueue(sessionRequest); - try { - Thread.sleep(400); // wait for thread to complete - } catch (InterruptedException ignore) { - } - + assertThat(latch.await(1000, MILLISECONDS)).isTrue(); assertThat(isCompleted.get()).isFalse(); } From 3d098673d07a10e9d9b2ec7b9771c06a0879e645 Mon Sep 17 00:00:00 2001 From: bhecquet Date: Wed, 4 Oct 2023 15:41:30 +0200 Subject: [PATCH 16/23] Remove spaces --- java/src/org/openqa/selenium/grid/node/local/LocalNode.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/java/src/org/openqa/selenium/grid/node/local/LocalNode.java b/java/src/org/openqa/selenium/grid/node/local/LocalNode.java index b7ac7f4129bd4..9f52b048dc959 100644 --- a/java/src/org/openqa/selenium/grid/node/local/LocalNode.java +++ b/java/src/org/openqa/selenium/grid/node/local/LocalNode.java @@ -401,7 +401,7 @@ public boolean isSupporting(Capabilities capabilities) { public Either newSession( CreateSessionRequest sessionRequest) { Require.nonNull("Session request", sessionRequest); - + try (Span span = tracer.getCurrentContext().createSpan("node.new_session")) { AttributeMap attributeMap = tracer.createAttributeMap(); attributeMap.put(AttributeKey.LOGGER_CLASS.getKey(), getClass().getName()); From 0afa08979167acf3d648f726b87767be8ec506b0 Mon Sep 17 00:00:00 2001 From: bhecquet Date: Wed, 4 Oct 2023 15:43:05 +0200 Subject: [PATCH 17/23] remove spaces --- .../selenium/grid/sessionqueue/local/LocalNewSessionQueue.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/java/src/org/openqa/selenium/grid/sessionqueue/local/LocalNewSessionQueue.java b/java/src/org/openqa/selenium/grid/sessionqueue/local/LocalNewSessionQueue.java index 18389f0a64ec8..2c68cf4a494ec 100644 --- a/java/src/org/openqa/selenium/grid/sessionqueue/local/LocalNewSessionQueue.java +++ b/java/src/org/openqa/selenium/grid/sessionqueue/local/LocalNewSessionQueue.java @@ -94,7 +94,7 @@ objectName = "org.seleniumhq.grid:type=SessionQueue,name=LocalSessionQueue", description = "New session queue") public class LocalNewSessionQueue extends NewSessionQueue implements Closeable { - + private static final String NAME = "Local New Session Queue"; private final SlotMatcher slotMatcher; private final Duration requestTimeout; From 93fc4f04c4ae5a2ceef9670882a006dbd7eac68e Mon Sep 17 00:00:00 2001 From: bhecquet Date: Wed, 4 Oct 2023 15:44:09 +0200 Subject: [PATCH 18/23] Remove unused import --- .../selenium/grid/sessionqueue/local/LocalNewSessionQueue.java | 1 - 1 file changed, 1 deletion(-) diff --git a/java/src/org/openqa/selenium/grid/sessionqueue/local/LocalNewSessionQueue.java b/java/src/org/openqa/selenium/grid/sessionqueue/local/LocalNewSessionQueue.java index 2c68cf4a494ec..00beeb368729a 100644 --- a/java/src/org/openqa/selenium/grid/sessionqueue/local/LocalNewSessionQueue.java +++ b/java/src/org/openqa/selenium/grid/sessionqueue/local/LocalNewSessionQueue.java @@ -44,7 +44,6 @@ import java.util.concurrent.locks.ReentrantReadWriteLock; import java.util.function.Predicate; import java.util.stream.Collectors; -import java.util.logging.Logger; import org.openqa.selenium.Capabilities; import org.openqa.selenium.SessionNotCreatedException; import org.openqa.selenium.concurrent.GuardedRunnable; From 5a80fcdc47ffa2ff8e5d607af2a83d9a05f5ec68 Mon Sep 17 00:00:00 2001 From: bhecquet bhecquet Date: Fri, 6 Oct 2023 09:13:25 +0000 Subject: [PATCH 19/23] Add comments to tell why session is valid --- .../selenium/grid/distributor/local/LocalDistributor.java | 1 + .../org/openqa/selenium/grid/sessionqueue/SessionCreated.java | 2 ++ .../openqa/selenium/grid/sessionqueue/SessionNotCreated.java | 2 ++ 3 files changed, 5 insertions(+) diff --git a/java/src/org/openqa/selenium/grid/distributor/local/LocalDistributor.java b/java/src/org/openqa/selenium/grid/distributor/local/LocalDistributor.java index 832bda1bb25ec..9c4a7d5acbd1f 100644 --- a/java/src/org/openqa/selenium/grid/distributor/local/LocalDistributor.java +++ b/java/src/org/openqa/selenium/grid/distributor/local/LocalDistributor.java @@ -839,6 +839,7 @@ 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 if (!isSessionValid && response.isRight()) { diff --git a/java/src/org/openqa/selenium/grid/sessionqueue/SessionCreated.java b/java/src/org/openqa/selenium/grid/sessionqueue/SessionCreated.java index a41f3fb096de1..de433e1f4450b 100644 --- a/java/src/org/openqa/selenium/grid/sessionqueue/SessionCreated.java +++ b/java/src/org/openqa/selenium/grid/sessionqueue/SessionCreated.java @@ -52,6 +52,8 @@ public HttpResponse execute(HttpRequest req) throws UncheckedIOException { HTTP_REQUEST.accept(span, req); CreateSessionResponse response = Contents.fromJson(req, CreateSessionResponse.class); + + // '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 = queue.complete(requestId, Either.right(response)); HttpResponse res = new HttpResponse().setContent(asJson(singletonMap("value", isSessionValid))); diff --git a/java/src/org/openqa/selenium/grid/sessionqueue/SessionNotCreated.java b/java/src/org/openqa/selenium/grid/sessionqueue/SessionNotCreated.java index ede4a04faf378..a09e0420133e8 100644 --- a/java/src/org/openqa/selenium/grid/sessionqueue/SessionNotCreated.java +++ b/java/src/org/openqa/selenium/grid/sessionqueue/SessionNotCreated.java @@ -54,6 +54,8 @@ public HttpResponse execute(HttpRequest req) throws UncheckedIOException { String message = Contents.fromJson(req, String.class); SessionNotCreatedException exception = new SessionNotCreatedException(message); + + // '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 = queue.complete(requestId, Either.left(exception)); HttpResponse res = new HttpResponse().setContent(asJson(singletonMap("value", isSessionValid))); From e3d42967fcfe893d6a3428711bb1c01aa06680da Mon Sep 17 00:00:00 2001 From: titusfortner Date: Fri, 6 Oct 2023 13:27:48 -0500 Subject: [PATCH 20/23] fix linter issues --- .../distributor/local/LocalDistributor.java | 20 ++++++++++++------- .../grid/sessionqueue/SessionCreated.java | 6 ++++-- .../grid/sessionqueue/SessionNotCreated.java | 6 ++++-- .../local/LocalNewSessionQueue.java | 10 +++------- .../local/LocalNewSessionQueueTest.java | 18 +++++++++-------- 5 files changed, 34 insertions(+), 26 deletions(-) diff --git a/java/src/org/openqa/selenium/grid/distributor/local/LocalDistributor.java b/java/src/org/openqa/selenium/grid/distributor/local/LocalDistributor.java index 9c4a7d5acbd1f..d46b66bc167e6 100644 --- a/java/src/org/openqa/selenium/grid/distributor/local/LocalDistributor.java +++ b/java/src/org/openqa/selenium/grid/distributor/local/LocalDistributor.java @@ -42,8 +42,8 @@ import java.util.HashMap; import java.util.List; import java.util.Map; -import java.util.Set; import java.util.Optional; +import java.util.Set; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.Executor; import java.util.concurrent.ExecutorService; @@ -839,12 +839,17 @@ 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 + // '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 + // If the session request has timed out, tell the Node to remove the session, so that does + // not stall if (!isSessionValid && response.isRight()) { LOG.log( - Debug.getDebugLogLevel(), "Session for request {0} has been created but it has timed out, stopping it to avoid stalled browser", reqId.toString()); + Debug.getDebugLogLevel(), + "Session for request {0} has been created but it has timed out, stopping it to avoid" + + " stalled browser", + reqId.toString()); URI nodeURI = response.right().getSession().getUri(); Node node = getNodeFromURI(nodeURI); if (node != null) { @@ -859,9 +864,10 @@ protected Node getNodeFromURI(URI uri) { Lock readLock = this.lock.readLock(); readLock.lock(); try { - Optional nodeStatus = model.getSnapshot().stream() - .filter(node -> node.getExternalUri().equals(uri)) - .findFirst(); + Optional nodeStatus = + model.getSnapshot().stream() + .filter(node -> node.getExternalUri().equals(uri)) + .findFirst(); if (nodeStatus.isPresent()) { return nodes.get(nodeStatus.get().getNodeId()); } else { diff --git a/java/src/org/openqa/selenium/grid/sessionqueue/SessionCreated.java b/java/src/org/openqa/selenium/grid/sessionqueue/SessionCreated.java index de433e1f4450b..674797d0aa2f3 100644 --- a/java/src/org/openqa/selenium/grid/sessionqueue/SessionCreated.java +++ b/java/src/org/openqa/selenium/grid/sessionqueue/SessionCreated.java @@ -53,10 +53,12 @@ public HttpResponse execute(HttpRequest req) throws UncheckedIOException { CreateSessionResponse response = Contents.fromJson(req, CreateSessionResponse.class); - // '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 + // '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 = queue.complete(requestId, Either.right(response)); - HttpResponse res = new HttpResponse().setContent(asJson(singletonMap("value", isSessionValid))); + HttpResponse res = + new HttpResponse().setContent(asJson(singletonMap("value", isSessionValid))); HTTP_RESPONSE.accept(span, res); return res; diff --git a/java/src/org/openqa/selenium/grid/sessionqueue/SessionNotCreated.java b/java/src/org/openqa/selenium/grid/sessionqueue/SessionNotCreated.java index a09e0420133e8..7a8d4e8181d89 100644 --- a/java/src/org/openqa/selenium/grid/sessionqueue/SessionNotCreated.java +++ b/java/src/org/openqa/selenium/grid/sessionqueue/SessionNotCreated.java @@ -55,10 +55,12 @@ public HttpResponse execute(HttpRequest req) throws UncheckedIOException { String message = Contents.fromJson(req, String.class); SessionNotCreatedException exception = new SessionNotCreatedException(message); - // '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 + // '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 = queue.complete(requestId, Either.left(exception)); - HttpResponse res = new HttpResponse().setContent(asJson(singletonMap("value", isSessionValid))); + HttpResponse res = + new HttpResponse().setContent(asJson(singletonMap("value", isSessionValid))); HTTP_RESPONSE.accept(span, res); return res; } diff --git a/java/src/org/openqa/selenium/grid/sessionqueue/local/LocalNewSessionQueue.java b/java/src/org/openqa/selenium/grid/sessionqueue/local/LocalNewSessionQueue.java index 00beeb368729a..d673cc342de95 100644 --- a/java/src/org/openqa/selenium/grid/sessionqueue/local/LocalNewSessionQueue.java +++ b/java/src/org/openqa/selenium/grid/sessionqueue/local/LocalNewSessionQueue.java @@ -289,14 +289,13 @@ public boolean retryAddToQueue(SessionRequest request) { try { if (!requests.containsKey(request.getRequestId())) { return false; - } + } if (isTimedOut(Instant.now(), requests.get(request.getRequestId()))) { // as we try to re-add a session request that has already expired, force session timeout failDueToTimeout(request.getRequestId()); // return true to avoid handleNewSessionRequest to call 'complete' an other time return true; - } - + } if (queue.contains(request)) { // No need to re-add this @@ -369,9 +368,7 @@ public List getNextAvailable(Map stereotypes } } - /** - * Returns true if the session is still valid (not timed out) - */ + /** Returns true if the session is still valid (not timed out) */ @Override public boolean complete( RequestId reqId, Either result) { @@ -485,6 +482,5 @@ public synchronized void setResult( complete = true; latch.countDown(); } - } } diff --git a/java/test/org/openqa/selenium/grid/sessionqueue/local/LocalNewSessionQueueTest.java b/java/test/org/openqa/selenium/grid/sessionqueue/local/LocalNewSessionQueueTest.java index a70fb0cfe0594..b099243066e26 100644 --- a/java/test/org/openqa/selenium/grid/sessionqueue/local/LocalNewSessionQueueTest.java +++ b/java/test/org/openqa/selenium/grid/sessionqueue/local/LocalNewSessionQueueTest.java @@ -20,8 +20,8 @@ import static java.net.HttpURLConnection.HTTP_INTERNAL_ERROR; import static java.net.HttpURLConnection.HTTP_OK; import static java.nio.charset.StandardCharsets.UTF_8; -import static java.util.concurrent.TimeUnit.SECONDS; import static java.util.concurrent.TimeUnit.MILLISECONDS; +import static java.util.concurrent.TimeUnit.SECONDS; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.fail; import static org.junit.jupiter.api.Assertions.assertEquals; @@ -45,11 +45,11 @@ import java.util.Set; import java.util.UUID; import java.util.concurrent.Callable; +import java.util.concurrent.CountDownLatch; import java.util.concurrent.ExecutionException; import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; import java.util.concurrent.Future; -import java.util.concurrent.CountDownLatch; import java.util.concurrent.TimeoutException; import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicInteger; @@ -201,7 +201,8 @@ void testCompleteWithCreatedSession(Supplier supplier) throws Interrup "capabilities", capabilities))) .getBytes(UTF_8)); - isCompleted.set(queue.complete(sessionRequest.getRequestId(), Either.right(sessionResponse))); + isCompleted.set( + queue.complete(sessionRequest.getRequestId(), Either.right(sessionResponse))); latch.countDown(); }) .start(); @@ -212,7 +213,6 @@ void testCompleteWithCreatedSession(Supplier supplier) throws Interrup assertThat(isCompleted.get()).isTrue(); } - @ParameterizedTest @MethodSource("data") void testCompleteWithSessionInTimeout(Supplier supplier) throws InterruptedException { @@ -248,7 +248,10 @@ void testCompleteWithSessionInTimeout(Supplier supplier) throws Interr "capabilities", capabilities))) .getBytes(UTF_8)); - isCompleted.set(queue.complete(sessionRequest.getRequestId(), Either.left(new SessionNotCreatedException("not created")))); + isCompleted.set( + queue.complete( + sessionRequest.getRequestId(), + Either.left(new SessionNotCreatedException("not created")))); latch.countDown(); }) .start(); @@ -302,7 +305,8 @@ void shouldBeAbleToAddToQueueAndGetValidResponse(Supplier supplier) { @ParameterizedTest @MethodSource("data") - void shouldBeAbleToAddToQueueWithTimeoutDoNotCreateSessionAfterTimeout(Supplier supplier) { + void shouldBeAbleToAddToQueueWithTimeoutDoNotCreateSessionAfterTimeout( + Supplier supplier) { setup(supplier); SessionRequest sessionRequestWithTimeout = @@ -340,7 +344,6 @@ void shouldBeAbleToAddToQueueWithTimeoutDoNotCreateSessionAfterTimeout(Supplier< // LocalDistributor could not distribute the session, add it back to queue // it should not be re-added to queue and send back error on session creation queue.retryAddToQueue(sessionRequestWithTimeout); - }) .start(); @@ -411,7 +414,6 @@ void shouldBeAbleToAddToQueueWithTimeoutAndTimeoutResponse(Supplier su assertThat(isPresent.get()).isTrue(); assertEquals(HTTP_INTERNAL_ERROR, httpResponse.getStatus()); } - @ParameterizedTest @MethodSource("data") From 9af02d43d31ff708641570ee0e91d2743d624a20 Mon Sep 17 00:00:00 2001 From: bhecquet bhecquet Date: Wed, 11 Oct 2023 13:25:50 +0000 Subject: [PATCH 21/23] Add a test when session time out --- .../SessionQueueGridWithTimeoutTest.java | 198 ++++++++++++++++++ 1 file changed, 198 insertions(+) create mode 100644 java/test/org/openqa/selenium/grid/router/SessionQueueGridWithTimeoutTest.java diff --git a/java/test/org/openqa/selenium/grid/router/SessionQueueGridWithTimeoutTest.java b/java/test/org/openqa/selenium/grid/router/SessionQueueGridWithTimeoutTest.java new file mode 100644 index 0000000000000..48b2a95b54512 --- /dev/null +++ b/java/test/org/openqa/selenium/grid/router/SessionQueueGridWithTimeoutTest.java @@ -0,0 +1,198 @@ +// 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.net.HttpURLConnection.HTTP_INTERNAL_ERROR; +import static java.net.HttpURLConnection.HTTP_OK; +import static java.util.concurrent.TimeUnit.SECONDS; +import static org.assertj.core.api.Assertions.assertThat; +import static org.junit.jupiter.api.Assertions.fail; +import static org.openqa.selenium.remote.http.Contents.asJson; +import static org.openqa.selenium.remote.http.HttpMethod.DELETE; +import static org.openqa.selenium.remote.http.HttpMethod.POST; + +import com.google.common.collect.ImmutableMap; +import java.net.MalformedURLException; +import java.net.URI; +import java.net.URISyntaxException; +import java.time.Duration; +import java.time.Instant; +import java.util.Arrays; +import java.util.List; +import java.util.concurrent.Callable; +import java.util.concurrent.ExecutionException; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.Future; +import java.util.concurrent.ScheduledExecutorService; +import java.util.concurrent.TimeoutException; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.openqa.selenium.Capabilities; +import org.openqa.selenium.ImmutableCapabilities; +import org.openqa.selenium.events.EventBus; +import org.openqa.selenium.events.local.GuavaEventBus; +import org.openqa.selenium.grid.config.MapConfig; +import org.openqa.selenium.grid.data.DefaultSlotMatcher; +import org.openqa.selenium.grid.data.Session; +import org.openqa.selenium.grid.distributor.Distributor; +import org.openqa.selenium.grid.distributor.local.LocalDistributor; +import org.openqa.selenium.grid.distributor.selector.DefaultSlotSelector; +import org.openqa.selenium.grid.node.local.LocalNode; +import org.openqa.selenium.grid.security.AddSecretFilter; +import org.openqa.selenium.grid.security.Secret; +import org.openqa.selenium.grid.server.BaseServerOptions; +import org.openqa.selenium.grid.server.Server; +import org.openqa.selenium.grid.sessionmap.SessionMap; +import org.openqa.selenium.grid.sessionmap.local.LocalSessionMap; +import org.openqa.selenium.grid.sessionqueue.NewSessionQueue; +import org.openqa.selenium.grid.sessionqueue.local.LocalNewSessionQueue; +import org.openqa.selenium.grid.testing.PassthroughHttpClient; +import org.openqa.selenium.grid.testing.TestSessionFactory; +import org.openqa.selenium.grid.web.CombinedHandler; +import org.openqa.selenium.grid.web.RoutableHttpClientFactory; +import org.openqa.selenium.net.PortProber; +import org.openqa.selenium.netty.server.NettyServer; +import org.openqa.selenium.remote.http.HttpClient; +import org.openqa.selenium.remote.http.HttpHandler; +import org.openqa.selenium.remote.http.HttpRequest; +import org.openqa.selenium.remote.http.HttpResponse; +import org.openqa.selenium.remote.tracing.DefaultTestTracer; +import org.openqa.selenium.remote.tracing.Tracer; + +class SessionQueueGridWithTimeoutTest { + private static final Capabilities CAPS = new ImmutableCapabilities("browserName", "cheese"); + private HttpClient.Factory clientFactory; + private Secret registrationSecret; + private Server server; + private EventBus bus; + + private static Server createServer(HttpHandler handler) { + return new NettyServer( + new BaseServerOptions( + new MapConfig( + ImmutableMap.of("server", ImmutableMap.of("port", PortProber.findFreePort())))), + handler); + } + + @BeforeEach + public void setup() throws URISyntaxException, MalformedURLException { + Tracer tracer = DefaultTestTracer.createTracer(); + bus = new GuavaEventBus(); + int nodePort = PortProber.findFreePort(); + URI nodeUri = new URI("http://localhost:" + nodePort); + CombinedHandler handler = new CombinedHandler(); + clientFactory = + new RoutableHttpClientFactory(nodeUri.toURL(), handler, HttpClient.Factory.createDefault()); + + registrationSecret = new Secret("cheese"); + + SessionMap sessions = new LocalSessionMap(tracer, bus); + NewSessionQueue queue = + new LocalNewSessionQueue( + tracer, + new DefaultSlotMatcher(), + Duration.ofSeconds(1), + Duration.ofSeconds(5), // low timeout to allow simulating it + registrationSecret, + 5); + handler.addHandler(queue); + + LocalNode localNode = + LocalNode.builder(tracer, bus, nodeUri, nodeUri, registrationSecret) + .add( + CAPS, + new TestSessionFactory( + (id, caps) -> { + try { + Thread.sleep(6000); // simulate a session that takes long to create + } catch (Exception e) {} + return new Session(id, nodeUri, new ImmutableCapabilities(), caps, Instant.now()); + })) + .maximumConcurrentSessions(1) + .build(); + handler.addHandler(localNode); + + Distributor distributor = + new LocalDistributor( + tracer, + bus, + new PassthroughHttpClient.Factory(localNode), + sessions, + queue, + new DefaultSlotSelector(), + registrationSecret, + Duration.ofMinutes(5), + false, + Duration.ofSeconds(5), + Runtime.getRuntime().availableProcessors(), + new DefaultSlotMatcher()); + handler.addHandler(distributor); + + distributor.add(localNode); + + Router router = new Router(tracer, clientFactory, sessions, queue, distributor); + + server = createServer(router); + server.start(); + } + + @Test + void shouldBeAbleToDeleteTimedoutSessions() { + ImmutableMap caps = ImmutableMap.of("browserName", "cheese"); + ExecutorService fixedThreadPoolService = Executors.newFixedThreadPool(1); + ScheduledExecutorService scheduler = Executors.newSingleThreadScheduledExecutor(); + + try { + Callable sessionCreationTask = () -> createSession(caps); + List> futureList = + fixedThreadPoolService.invokeAll(Arrays.asList(sessionCreationTask)); + + for (Future future : futureList) { + HttpResponse httpResponse = future.get(10, SECONDS); + assertThat(httpResponse.getStatus()).isEqualTo(HTTP_INTERNAL_ERROR); + } + } catch (InterruptedException e) { + fail("Unable to create session. Thread Interrupted"); + } catch (ExecutionException e) { + fail("Unable to create session due to execution exception."); + } catch (TimeoutException e) { + fail("Unable to create session. Timeout occurred."); + } finally { + fixedThreadPoolService.shutdownNow(); + scheduler.shutdownNow(); + } + } + + @AfterEach + public void stopServer() { + bus.close(); + server.stop(); + } + + private HttpResponse createSession(ImmutableMap caps) { + HttpRequest request = new HttpRequest(POST, "/session"); + request.setContent( + asJson(ImmutableMap.of("capabilities", ImmutableMap.of("alwaysMatch", caps)))); + + try (HttpClient client = clientFactory.createClient(server.getUrl())) { + return client.execute(request); + } + } +} From 618581820b4d32ad58713c22654a41ac21ed2e6f Mon Sep 17 00:00:00 2001 From: bhecquet bhecquet Date: Wed, 11 Oct 2023 16:06:23 +0000 Subject: [PATCH 22/23] test improved --- .../router/SessionQueueGridWithTimeoutTest.java | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/java/test/org/openqa/selenium/grid/router/SessionQueueGridWithTimeoutTest.java b/java/test/org/openqa/selenium/grid/router/SessionQueueGridWithTimeoutTest.java index 48b2a95b54512..cb59cc1cad0b6 100644 --- a/java/test/org/openqa/selenium/grid/router/SessionQueueGridWithTimeoutTest.java +++ b/java/test/org/openqa/selenium/grid/router/SessionQueueGridWithTimeoutTest.java @@ -75,6 +75,7 @@ import org.openqa.selenium.remote.http.HttpResponse; import org.openqa.selenium.remote.tracing.DefaultTestTracer; import org.openqa.selenium.remote.tracing.Tracer; +import org.openqa.selenium.support.ui.FluentWait; class SessionQueueGridWithTimeoutTest { private static final Capabilities CAPS = new ImmutableCapabilities("browserName", "cheese"); @@ -82,6 +83,7 @@ class SessionQueueGridWithTimeoutTest { private Secret registrationSecret; private Server server; private EventBus bus; + private LocalNode localNode; private static Server createServer(HttpHandler handler) { return new NettyServer( @@ -114,7 +116,7 @@ public void setup() throws URISyntaxException, MalformedURLException { 5); handler.addHandler(queue); - LocalNode localNode = + localNode = LocalNode.builder(tracer, bus, nodeUri, nodeUri, registrationSecret) .add( CAPS, @@ -167,7 +169,17 @@ void shouldBeAbleToDeleteTimedoutSessions() { for (Future future : futureList) { HttpResponse httpResponse = future.get(10, SECONDS); assertThat(httpResponse.getStatus()).isEqualTo(HTTP_INTERNAL_ERROR); + // session is creating, so a slot is used + assertThat(localNode.getUsedSlots()).isEqualTo(1); + + // session has been destroyed on node as it's not used + new FluentWait<>(localNode) + .withTimeout(Duration.ofSeconds(7)) + .until(node -> node.getUsedSlots() == 0); } + + + } catch (InterruptedException e) { fail("Unable to create session. Thread Interrupted"); } catch (ExecutionException e) { From 9129264d081a6997227d3631860f723ebb803f74 Mon Sep 17 00:00:00 2001 From: Diego Molina Date: Tue, 17 Oct 2023 14:21:22 +0000 Subject: [PATCH 23/23] Running format script --- .../BrowsingContextInspectorTest.java | 1 - .../SessionQueueGridWithTimeoutTest.java | 21 ++++++++----------- 2 files changed, 9 insertions(+), 13 deletions(-) diff --git a/java/test/org/openqa/selenium/bidi/browsingcontext/BrowsingContextInspectorTest.java b/java/test/org/openqa/selenium/bidi/browsingcontext/BrowsingContextInspectorTest.java index 4291eb5d12ef2..dc7b584996ab4 100644 --- a/java/test/org/openqa/selenium/bidi/browsingcontext/BrowsingContextInspectorTest.java +++ b/java/test/org/openqa/selenium/bidi/browsingcontext/BrowsingContextInspectorTest.java @@ -24,7 +24,6 @@ import java.util.concurrent.ExecutionException; import java.util.concurrent.TimeUnit; import java.util.concurrent.TimeoutException; - import org.junit.Ignore; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; diff --git a/java/test/org/openqa/selenium/grid/router/SessionQueueGridWithTimeoutTest.java b/java/test/org/openqa/selenium/grid/router/SessionQueueGridWithTimeoutTest.java index cb59cc1cad0b6..455749debff96 100644 --- a/java/test/org/openqa/selenium/grid/router/SessionQueueGridWithTimeoutTest.java +++ b/java/test/org/openqa/selenium/grid/router/SessionQueueGridWithTimeoutTest.java @@ -18,12 +18,10 @@ package org.openqa.selenium.grid.router; import static java.net.HttpURLConnection.HTTP_INTERNAL_ERROR; -import static java.net.HttpURLConnection.HTTP_OK; import static java.util.concurrent.TimeUnit.SECONDS; import static org.assertj.core.api.Assertions.assertThat; import static org.junit.jupiter.api.Assertions.fail; import static org.openqa.selenium.remote.http.Contents.asJson; -import static org.openqa.selenium.remote.http.HttpMethod.DELETE; import static org.openqa.selenium.remote.http.HttpMethod.POST; import com.google.common.collect.ImmutableMap; @@ -55,7 +53,6 @@ import org.openqa.selenium.grid.distributor.local.LocalDistributor; import org.openqa.selenium.grid.distributor.selector.DefaultSlotSelector; import org.openqa.selenium.grid.node.local.LocalNode; -import org.openqa.selenium.grid.security.AddSecretFilter; import org.openqa.selenium.grid.security.Secret; import org.openqa.selenium.grid.server.BaseServerOptions; import org.openqa.selenium.grid.server.Server; @@ -122,11 +119,13 @@ public void setup() throws URISyntaxException, MalformedURLException { CAPS, new TestSessionFactory( (id, caps) -> { - try { - Thread.sleep(6000); // simulate a session that takes long to create - } catch (Exception e) {} - return new Session(id, nodeUri, new ImmutableCapabilities(), caps, Instant.now()); - })) + try { + Thread.sleep(6000); // simulate a session that takes long to create + } catch (Exception e) { + } + return new Session( + id, nodeUri, new ImmutableCapabilities(), caps, Instant.now()); + })) .maximumConcurrentSessions(1) .build(); handler.addHandler(localNode); @@ -174,12 +173,10 @@ void shouldBeAbleToDeleteTimedoutSessions() { // session has been destroyed on node as it's not used new FluentWait<>(localNode) - .withTimeout(Duration.ofSeconds(7)) - .until(node -> node.getUsedSlots() == 0); + .withTimeout(Duration.ofSeconds(7)) + .until(node -> node.getUsedSlots() == 0); } - - } catch (InterruptedException e) { fail("Unable to create session. Thread Interrupted"); } catch (ExecutionException e) {