Migrate most command handlers to http handlers in router

SeleniumHQ · Jul 4, 2019 · c5b8a70 · c5b8a70
1 parent f17525c
commit c5b8a70
Show file tree

Hide file tree

Showing 4 changed files with 41 additions and 40 deletions.
diff --git a/java/server/src/org/openqa/selenium/grid/router/GridStatusHandler.java b/java/server/src/org/openqa/selenium/grid/router/GridStatusHandler.java
@@ -17,21 +17,12 @@
 
 package org.openqa.selenium.grid.router;
 
-import static java.util.concurrent.TimeUnit.MILLISECONDS;
-import static java.util.concurrent.TimeUnit.SECONDS;
-import static java.util.stream.Collectors.toList;
-import static org.openqa.selenium.json.Json.MAP_TYPE;
-import static org.openqa.selenium.remote.http.Contents.string;
-import static org.openqa.selenium.remote.http.Contents.utf8String;
-import static org.openqa.selenium.remote.http.HttpMethod.GET;
-
 import com.google.common.collect.ImmutableMap;
-
 import org.openqa.selenium.grid.data.DistributorStatus;
 import org.openqa.selenium.grid.distributor.Distributor;
-import org.openqa.selenium.grid.web.CommandHandler;
 import org.openqa.selenium.json.Json;
 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;
 
@@ -47,7 +38,15 @@
 import java.util.concurrent.ScheduledExecutorService;
 import java.util.concurrent.TimeoutException;
 
-class GridStatusHandler implements CommandHandler {
+import static java.util.concurrent.TimeUnit.MILLISECONDS;
+import static java.util.concurrent.TimeUnit.SECONDS;
+import static java.util.stream.Collectors.toList;
+import static org.openqa.selenium.json.Json.MAP_TYPE;
+import static org.openqa.selenium.remote.http.Contents.string;
+import static org.openqa.selenium.remote.http.Contents.utf8String;
+import static org.openqa.selenium.remote.http.HttpMethod.GET;
+
+class GridStatusHandler implements HttpHandler {
 
   private static final ScheduledExecutorService
       SCHEDULED_SERVICE =
@@ -79,18 +78,17 @@ public GridStatusHandler(Json json, HttpClient.Factory clientFactory, Distributo
   }
 
   @Override
-  public void execute(HttpRequest req, HttpResponse resp) {
+  public HttpResponse execute(HttpRequest req) {
     long start = System.currentTimeMillis();
 
     DistributorStatus status;
     try {
       status = EXECUTOR_SERVICE.submit(distributor::getStatus).get(2, SECONDS);
     } catch (ExecutionException | InterruptedException | TimeoutException e) {
-      resp.setContent(utf8String(json.toJson(
+      return new HttpResponse().setContent(utf8String(json.toJson(
           ImmutableMap.of("value", ImmutableMap.of(
               "ready", false,
               "message", "Unable to read distributor status.")))));
-      return;
     }
 
     boolean ready = status.hasCapacity();
@@ -153,7 +151,7 @@ public void execute(HttpRequest req, HttpResponse resp) {
         })
         .collect(toList()));
 
-    resp.setContent(utf8String(json.toJson(ImmutableMap.of("value", value.build()))));
+    return new HttpResponse().setContent(utf8String(json.toJson(ImmutableMap.of("value", value.build()))));
   }
 
   private RuntimeException wrap(Exception e) {

diff --git a/java/server/src/org/openqa/selenium/grid/router/HandleSession.java b/java/server/src/org/openqa/selenium/grid/router/HandleSession.java
@@ -17,12 +17,9 @@
 
 package org.openqa.selenium.grid.router;
 
-import static org.openqa.selenium.remote.HttpSessionId.getSessionId;
-
 import com.google.common.cache.CacheBuilder;
 import com.google.common.cache.CacheLoader;
 import com.google.common.cache.LoadingCache;
-
 import org.openqa.selenium.NoSuchSessionException;
 import org.openqa.selenium.grid.data.Session;
 import org.openqa.selenium.grid.sessionmap.SessionMap;
@@ -31,17 +28,21 @@
 import org.openqa.selenium.net.Urls;
 import org.openqa.selenium.remote.SessionId;
 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.DistributedTracer;
 import org.openqa.selenium.remote.tracing.Span;
 
 import java.io.IOException;
+import java.io.UncheckedIOException;
 import java.time.Duration;
 import java.util.Objects;
 import java.util.concurrent.ExecutionException;
 
-class HandleSession implements CommandHandler {
+import static org.openqa.selenium.remote.HttpSessionId.getSessionId;
+
+class HandleSession implements HttpHandler {
 
   private final LoadingCache<SessionId, CommandHandler> knownSessions;
   private final DistributedTracer tracer;
@@ -69,7 +70,7 @@ public CommandHandler load(SessionId id) {
   }
 
   @Override
-  public void execute(HttpRequest req, HttpResponse resp) throws IOException {
+  public HttpResponse execute(HttpRequest req) {
     try (Span span = tracer.createSpan("router.webdriver-command", tracer.getActiveSpan())) {
       span.addTag("http.method", req.getMethod());
       span.addTag("http.url", req.getUri());
@@ -80,8 +81,10 @@ public void execute(HttpRequest req, HttpResponse resp) throws IOException {
       span.addTag("session.id", id);
 
       try {
+        HttpResponse resp = new HttpResponse();
         knownSessions.get(id).execute(req, resp);
         span.addTag("http.status", resp.getStatus());
+        return resp;
       } catch (ExecutionException e) {
         span.addTag("exception", e.getMessage());
 
@@ -90,7 +93,9 @@ public void execute(HttpRequest req, HttpResponse resp) throws IOException {
           throw (RuntimeException) cause;
         }
         throw new RuntimeException(cause);
+      } catch (IOException e) {
+        throw new UncheckedIOException(e);
       }
     }
-  }
+  };
 }
diff --git a/java/server/src/org/openqa/selenium/grid/router/Router.java b/java/server/src/org/openqa/selenium/grid/router/Router.java
@@ -19,15 +19,15 @@
 
 import org.openqa.selenium.grid.distributor.Distributor;
 import org.openqa.selenium.grid.sessionmap.SessionMap;
-import org.openqa.selenium.grid.web.CommandHandler;
 import org.openqa.selenium.json.Json;
 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.http.Routable;
 import org.openqa.selenium.remote.http.Route;
 import org.openqa.selenium.remote.tracing.DistributedTracer;
 
-import java.io.IOException;
 import java.util.function.Predicate;
 
 import static org.openqa.selenium.remote.http.Route.combine;
@@ -37,7 +37,7 @@
 /**
  * A simple router that is aware of the selenium-protocol.
  */
-public class Router implements Predicate<HttpRequest>, CommandHandler {
+public class Router implements Predicate<HttpRequest>, Routable, HttpHandler {
 
   private final Route routes;
 
@@ -61,7 +61,12 @@ public boolean test(HttpRequest req) {
   }
 
   @Override
-  public void execute(HttpRequest req, HttpResponse resp) throws IOException {
-    copyResponse(routes.execute(req), resp);
+  public boolean matches(HttpRequest req) {
+    return routes.matches(req);
+  }
+
+  @Override
+  public HttpResponse execute(HttpRequest req) {
+    return routes.execute(req);
   }
 }
diff --git a/java/server/test/org/openqa/selenium/grid/router/RouterTest.java b/java/server/test/org/openqa/selenium/grid/router/RouterTest.java
@@ -17,12 +17,6 @@
 
 package org.openqa.selenium.grid.router;
 
-import static org.junit.Assert.assertFalse;
-import static org.junit.Assert.assertNotNull;
-import static org.junit.Assert.assertTrue;
-import static org.openqa.selenium.json.Json.MAP_TYPE;
-import static org.openqa.selenium.remote.http.HttpMethod.GET;
-
 import org.junit.Before;
 import org.junit.Test;
 import org.openqa.selenium.Capabilities;
@@ -46,13 +40,17 @@
 import org.openqa.selenium.remote.http.HttpResponse;
 import org.openqa.selenium.remote.tracing.DistributedTracer;
 
-import java.io.IOException;
-import java.io.UncheckedIOException;
 import java.net.URI;
 import java.net.URISyntaxException;
 import java.util.Map;
 import java.util.concurrent.atomic.AtomicBoolean;
 
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertTrue;
+import static org.openqa.selenium.json.Json.MAP_TYPE;
+import static org.openqa.selenium.remote.http.HttpMethod.GET;
+
 public class RouterTest {
 
   private EventBus bus;
@@ -133,12 +131,7 @@ public void ifNodesHaveSpareSlotsButAlreadyHaveMaxSessionsGridIsNotReady() {
   }
 
   private Map<String, Object> getStatus(Router router) {
-    HttpResponse response = new HttpResponse();
-    try {
-      router.execute(new HttpRequest(GET, "/status"), response);
-    } catch (IOException e) {
-      throw new UncheckedIOException(e);
-    }
+    HttpResponse response = router.execute(new HttpRequest(GET, "/status"));
     Map<String, Object> status = Values.get(response, MAP_TYPE);
     assertNotNull(status);
     return status;