Address issues in PR and remove static state

Author: SamCarlberg

core/src/main/java/edu/wpi/grip/core/http/ContextStore.java

@@ -0,0 +1,55 @@
+package edu.wpi.grip.core.http;
+
+import com.google.inject.Singleton;
+
+import javax.annotation.Nullable;
+import java.util.HashSet;
+import java.util.Set;
+
+import static com.google.common.base.Preconditions.checkNotNull;
+
+/**
+ * Keeps a record of contexts claimed by HTTP request handlers.
+ */
+@Singleton
+public class ContextStore {
+
+    private final Set<String> store = new HashSet<>();
+
+    /**
+     * Records the given context.
+     *
+     * @param context the context to record. This cannot be null.
+     * @throws IllegalArgumentException if the given context has already been claimed
+     */
+    public void record(String context) throws IllegalArgumentException {
+        checkNotNull(context);
+        if (!store.add(context)) {
+            throw new IllegalArgumentException("Context is already claimed: " + context);
+        }
+    }
+
+    /**
+     * Erases the given context from this store, if it's present. If {@code context} is {@code null}, this will
+     * do nothing and return {@code false}.
+     *
+     * @param context the context to erase
+     * @return true if the context was erased, false if it wasn't erased or if it wasn't present to begin with.
+     */
+    public boolean erase(@Nullable String context) {
+        return store.remove(context);
+    }
+
+    /**
+     * Checks if the given context has been recorded in this store. If {@code context} is {@code null}, this will
+     * return {@code false}.
+     *
+     * @param context the context to check
+     * @return true if the given context has been recorded in this store
+     */
+    public boolean contains(@Nullable String context) {
+        return store.contains(context);
+    }
+
+
+}

core/src/main/java/edu/wpi/grip/core/http/GenericHandler.java

@@ -16,15 +16,11 @@
  * Generic Jetty handler.
  * <p>
  * Instances of this class can either claim a context, preventing other instances from handling
- * events on that context, or not, in which case it will run on any context. The second type may use
- * {@link #isClaimed(String) isClaimed} to check if a context has been claimed if it shouldn't run on a claimed context.
+ * events on that context, or not, in which case it will run on any context.
  */
 public abstract class GenericHandler extends AbstractHandler {
 
-    /**
-     * Claimed contexts.
-     */
-    private static final Set<String> claimedContexts = new HashSet<>();
+    private final ContextStore contextStore;
 
     /**
      * The context that this handles.
@@ -51,6 +47,7 @@ public abstract class GenericHandler extends AbstractHandler {
      */
     protected GenericHandler() {
         super();
+        contextStore = new ContextStore();
         context = null;
     }
 
@@ -60,11 +57,12 @@ protected GenericHandler() {
      * Note that the context <strong>is case sensitive</strong>.
      * </p>
      *
+     * @param store   the context store to use to check for claimed contexts
      * @param context the context for this handler
      * @throws IllegalArgumentException if the given context has already been claimed
      */
-    protected GenericHandler(String context) {
-        this(context, false);
+    protected GenericHandler(ContextStore store, String context) {
+        this(store, context, false);
     }
 
     /**
@@ -73,36 +71,26 @@ protected GenericHandler(String context) {
      * Note that the context <strong>is case sensitive</strong>.
      * </p>
      *
+     * @param store   the context store to use to check for claimed contexts
      * @param context the context for this handler
      * @param doClaim flag marking if the given context should be claimed
      * @throws IllegalArgumentException if the given context has already been claimed
      */
-    protected GenericHandler(String context, boolean doClaim) {
+    protected GenericHandler(ContextStore store, String context, boolean doClaim) {
         super();
         checkNotNull(context);
-        if (isClaimed(context)) {
-            throw new IllegalArgumentException("The given context has already been claimed: " + context);
-        }
-        this.context = context;
         if (doClaim) {
-            claimContext();
+            store.record(context);
         }
-    }
-
-    /**
-     * Claims the context. Fails if that context has already been claimed and not released.
-     *
-     * @return true if the context was claimed, false if it has already been claimed
-     */
-    protected boolean claimContext() {
-        return claimedContexts.add(context);
+        this.contextStore = store;
+        this.context = context;
     }
 
     /**
      * Releases the context that this handles, allowing it to be claimed by another handler.
      */
     protected void releaseContext() {
-        claimedContexts.remove(context);
+        contextStore.erase(context);
     }
 
     /**
@@ -112,16 +100,6 @@ public String getContext() {
         return context;
     }
 
-    /**
-     * Checks if the given context has been claimed. Returns {@code false} if given {@code null}.
-     *
-     * @param context the context to check
-     * @return true if the context has been claimed, false if not
-     */
-    protected static boolean isClaimed(@Nullable String context) {
-        return claimedContexts.contains(context);
-    }
-
     // Static helper methods
 
     /**

core/src/main/java/edu/wpi/grip/core/http/GripServer.java

@@ -114,12 +114,12 @@ public Server create(int port) {
     }
 
     @Inject
-    GripServer(JettyServerFactory serverFactory, SettingsProvider settingsProvider) {
+    GripServer(ContextStore contextStore, JettyServerFactory serverFactory, SettingsProvider settingsProvider) {
         this.port = settingsProvider.getProjectSettings().getServerPort();
         this.serverFactory = serverFactory;
         this.server = serverFactory.create(port);
         this.server.setHandler(handlers);
-        handlers.addHandler(new NoContextHandler());
+        handlers.addHandler(new NoContextHandler(contextStore));
 
         Runtime.getRuntime().addShutdownHook(new Thread(this::stop));
     }
@@ -182,7 +182,14 @@ public void stop() {
      */
     public void restart() {
         try {
-            stop();
+            if (state == State.RUNNING) {
+                try {
+                    server.stop();
+                } catch (Exception ex) {
+                    throw new GripServerException("Could not stop Jetty server", ex);
+                }
+                state = State.STOPPED;
+            }
             server = serverFactory.create(port);
             start();
         } catch (GripServerException | IllegalStateException ex) {

core/src/main/java/edu/wpi/grip/core/http/HttpPipelineSwitcher.java

@@ -27,8 +27,8 @@ public class HttpPipelineSwitcher extends PedanticHandler {
     private final GRIPMode mode;
 
     @Inject
-    HttpPipelineSwitcher(Project project, GRIPMode mode) {
-        super(GripServer.PIPELINE_UPLOAD_PATH, true);
+    HttpPipelineSwitcher(ContextStore store, Project project, GRIPMode mode) {
+        super(store, GripServer.PIPELINE_UPLOAD_PATH, true);
         this.project = project;
         this.mode = mode;
     }

core/src/main/java/edu/wpi/grip/core/http/NoContextHandler.java

@@ -14,13 +14,23 @@
  */
 class NoContextHandler extends GenericHandler {
 
+    private final ContextStore store;
+
     private static final String notFoundMessage = "<h1>404 - Not Found</h1>There is no context for path: '%s'";
 
-    // implicit call to super() -- no context is claimed
+    /**
+     * Creates a new {@code NoContextHandler} that handles every context not in the given {@code ContextStore}.
+     *
+     * @param store Any context not in this {@code ContextStore} will get a {@code 404 Not Found} error.
+     */
+    public NoContextHandler(ContextStore store) {
+        super();
+        this.store = store;
+    }
 
     @Override
     public void handle(String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException {
-        if (isClaimed(target)) {
+        if (store.contains(target)) {
             // Let the appropriate handler handle this
             return;
         }

core/src/main/java/edu/wpi/grip/core/http/PedanticHandler.java

@@ -21,22 +21,24 @@ public abstract class PedanticHandler extends GenericHandler {
     /**
      * Creates a new handler for the given context. That context will not be claimed.
      *
+     * @param store the {@code ContextStore} to store this context in
      * @param context the context for this handler
-     * @see GenericHandler#GenericHandler(String)
+     * @see GenericHandler#GenericHandler(ContextStore, String)
      */
-    protected PedanticHandler(String context) {
-        super(context);
+    protected PedanticHandler(ContextStore store, String context) {
+        super(store, context);
     }
 
     /**
      * Creates a new handler for the given context.
      *
+     * @param store the {@code ContextStore} to store this context in
      * @param context the context for this handler
      * @param doClaim if the context should be claimed
-     * @see GenericHandler#GenericHandler(String, boolean)
+     * @see GenericHandler#GenericHandler(ContextStore, String, boolean)
      */
-    protected PedanticHandler(String context, boolean doClaim) {
-        super(context, doClaim);
+    protected PedanticHandler(ContextStore store, String context, boolean doClaim) {
+        super(store, context, doClaim);
     }
 
     @Override

core/src/main/java/edu/wpi/grip/core/operations/network/GRIPNetworkModule.java

@@ -3,6 +3,7 @@
 import com.google.inject.AbstractModule;
 import com.google.inject.name.Names;
 
+import edu.wpi.grip.core.http.ContextStore;
 import edu.wpi.grip.core.http.GripServer;
 import edu.wpi.grip.core.http.HttpPipelineSwitcher;
 import edu.wpi.grip.core.operations.network.http.DataHandler;
@@ -26,6 +27,7 @@ protected void configure() {
         bind(GripServer.class).asEagerSingleton();
         bind(HttpPipelineSwitcher.class).asEagerSingleton();
         bind(DataHandler.class).asEagerSingleton();
+        bind(ContextStore.class).asEagerSingleton();
         // Network publishing bindings
         bind(MapNetworkPublisherFactory.class)
                 .annotatedWith(Names.named("ntManager"))

core/src/main/java/edu/wpi/grip/core/operations/network/http/DataHandler.java

@@ -3,10 +3,12 @@
 import com.google.common.eventbus.Subscribe;
 import com.google.gson.Gson;
 import com.google.gson.GsonBuilder;
+import com.google.inject.Inject;
 import com.google.inject.Singleton;
 
 import edu.wpi.grip.core.events.RunStartedEvent;
 import edu.wpi.grip.core.events.RunStoppedEvent;
+import edu.wpi.grip.core.http.ContextStore;
 import edu.wpi.grip.core.http.GripServer;
 import edu.wpi.grip.core.http.PedanticHandler;
 
@@ -51,8 +53,9 @@ public final class DataHandler extends PedanticHandler {
      */
     private final AtomicBoolean staleData;
 
-    DataHandler() {
-        super(GripServer.DATA_PATH, true);
+    @Inject
+    DataHandler(ContextStore store) {
+        super(store, GripServer.DATA_PATH, true);
         this.dataSuppliers = new HashMap<>();
         this.gson = new GsonBuilder()
                 .setPrettyPrinting()

core/src/main/java/edu/wpi/grip/core/sources/HttpImageHandler.java

@@ -1,5 +1,6 @@
 package edu.wpi.grip.core.sources;
 
+import edu.wpi.grip.core.http.ContextStore;
 import edu.wpi.grip.core.http.GripServer;
 import edu.wpi.grip.core.http.PedanticHandler;
 
@@ -55,17 +56,17 @@ public final class HttpImageHandler extends PedanticHandler {
     /**
      * Creates an image handler on the default upload path {@code /GRIP/upload/image}
      */
-    public HttpImageHandler() {
-        this(GripServer.IMAGE_UPLOAD_PATH);
+    public HttpImageHandler(ContextStore store) {
+        this(store, GripServer.IMAGE_UPLOAD_PATH);
     }
 
     /**
      * Creates an image handler on the given path.
      *
      * @param path the path on the server that images will be uploaded to
      */
-    public HttpImageHandler(String path) {
-        super(path, true);
+    public HttpImageHandler(ContextStore store, String path) {
+        super(store, path, true);
         callbacks = new ArrayList<>();
     }
 

core/src/main/java/edu/wpi/grip/core/sources/HttpSource.java

@@ -11,6 +11,7 @@
 import edu.wpi.grip.core.Source;
 import edu.wpi.grip.core.events.SourceHasPendingUpdateEvent;
 import edu.wpi.grip.core.events.SourceRemovedEvent;
+import edu.wpi.grip.core.http.ContextStore;
 import edu.wpi.grip.core.http.GripServer;
 import edu.wpi.grip.core.sockets.OutputSocket;
 import edu.wpi.grip.core.sockets.SocketHint;
@@ -66,8 +67,9 @@ public interface Factory {
             EventBus eventBus,
             OutputSocket.Factory osf,
             GripServer server,
+            ContextStore store,
             @Assisted Properties properties) {
-        this(exceptionWitnessFactory, eventBus, osf, server, properties.getProperty(PATH_PROPERTY));
+        this(exceptionWitnessFactory, eventBus, osf, server, store, properties.getProperty(PATH_PROPERTY));
     }
 
     @AssistedInject
@@ -76,10 +78,11 @@ public interface Factory {
             EventBus eventBus,
             OutputSocket.Factory osf,
             GripServer server,
+            ContextStore store,
             @Assisted String path) {
         super(exceptionWitnessFactory);
         this.path = path;
-        this.imageHandler = handlers.computeIfAbsent(path, HttpImageHandler::new);
+        this.imageHandler = handlers.computeIfAbsent(path, p -> new HttpImageHandler(store, p));
         this.imageOutput = osf.create(outputHint);
         this.eventBus = eventBus;
         // Will add the handler only when the first HttpSource is created -- no-op every subsequent time