Fix deadlock when adding/removing observers

When adding/removing observers, we locked on the observer, then
acquired the global lock. When a metric is observed, then we acquire the
global lock and then the observer lock.

If these happen at the same time, we can easily end up with a deadlock.
We simply avoid holding the observer lock for the entire add/remove
process (instead only locking when actually needed).

Closes #1639

projects/common/src/main/java/dan200/computercraft/shared/computer/metrics/GlobalMetrics.java

@@ -46,12 +46,14 @@ public final class GlobalMetrics {
      * Add a new global metrics observer. This will receive metrics data for all computers.
      *
      * @param tracker The observer to add.
+     * @return Whether the observer was added. {@code false} if the observer was already registered.
      */
-    public void addObserver(ComputerMetricsObserver tracker) {
+    public boolean addObserver(ComputerMetricsObserver tracker) {
         synchronized (lock) {
-            if (trackers.contains(tracker)) return;
+            if (trackers.contains(tracker)) return false;
             trackers.add(tracker);
             enabled = true;
+            return true;
         }
     }
 
@@ -59,11 +61,13 @@ public final class GlobalMetrics {
      * Remove a previously-registered global metrics observer.
      *
      * @param tracker The observer to add.
+     * @return Whether the observer was removed. {@code false} if the observer was not registered.
      */
-    public void removeObserver(ComputerMetricsObserver tracker) {
+    public boolean removeObserver(ComputerMetricsObserver tracker) {
         synchronized (lock) {
-            trackers.remove(tracker);
+            var changed = trackers.remove(tracker);
             enabled = !trackers.isEmpty();
+            return changed;
         }
     }
 

projects/common/src/main/java/dan200/computercraft/shared/computer/metrics/basic/BasicComputerMetricsObserver.java

@@ -10,6 +10,7 @@ import dan200.computercraft.shared.computer.core.ServerComputer;
 import dan200.computercraft.shared.computer.metrics.ComputerMetricsObserver;
 import dan200.computercraft.shared.computer.metrics.GlobalMetrics;
 
+import javax.annotation.concurrent.GuardedBy;
 import java.util.ArrayList;
 import java.util.List;
 import java.util.Map;
@@ -21,29 +22,31 @@ import java.util.Map;
  */
 public class BasicComputerMetricsObserver implements ComputerMetricsObserver {
     private final GlobalMetrics owner;
-    private boolean tracking = false;
 
+    @GuardedBy("this")
     private final List<ComputerMetrics> timings = new ArrayList<>();
+
+    @GuardedBy("this")
     private final Map<ServerComputer, ComputerMetrics> timingLookup = new MapMaker().weakKeys().makeMap();
 
     public BasicComputerMetricsObserver(GlobalMetrics owner) {
         this.owner = owner;
     }
 
-    public synchronized void start() {
+    public void start() {
-        if (!tracking) owner.addObserver(this);
+        if (!owner.addObserver(this)) return;
-        tracking = true;
 
-        timings.clear();
+        synchronized (this) {
-        timingLookup.clear();
+            timings.clear();
+            timingLookup.clear();
+        }
     }
 
-    public synchronized boolean stop() {
+    public boolean stop() {
-        if (!tracking) return false;
+        if (!owner.removeObserver(this)) return false;
-
+        synchronized (this) {
-        owner.removeObserver(this);
+            timingLookup.clear();
-        tracking = false;
+        }
-        timingLookup.clear();
         return true;
     }
 
@@ -57,6 +60,7 @@ public class BasicComputerMetricsObserver implements ComputerMetricsObserver {
         return new ArrayList<>(timings);
     }
 
+    @GuardedBy("this")
     private ComputerMetrics getMetrics(ServerComputer computer) {
         var existing = timingLookup.get(computer);
         if (existing != null) return existing;