commit b13bec918b75c2fb9e498bfee96cbaa0df33b899
author Dave Borowitz <dborowitz@google.com> Fri May 24 11:16:54 2019 -0700
committer David Pursehouse <dpursehouse@collab.net> Sat May 25 14:22:03 2019 +0900
tree b0fce7108ff029466d7fdbd4b978e120961fe645
parent b31519f021a6d8aee5b53ea05c2e1b4d4d07796a

Error Prone: Enable and fix SynchronizeOnNonFinalField

The one instance in StreamEvents was not an actual bug, but honestly
it's pretty hard to follow and I could be wrong. The final
CancelableRunnable instance was instantiated along with the
StreamEvents, and if it ever actually called writeEvents, it would be
synchronizing on the wrong stdout instance. Looking a little closer, it
shouldn't be possible to call run() until after the runnable is
submitted to some executor, which only happens after initializing
stdout. Plus, stdout can only ever be the one instance or else null, so
this would have resulted in NPE if it ever happened in practice.

The new implementation makes both writer and stdout effectively final
variables at the time start is called, and passes them through as
arguments. This is still not especially easy to follow, but at least
Error Prone is able to help us out by running this analysis.

Change-Id: Ie9ba5d16e328aca5fcfb2c69b9ba5d4776ea095e

java/com/google/gerrit/sshd/commands/StreamEvents.java

diff --git a/java/com/google/gerrit/sshd/commands/StreamEvents.java b/java/com/google/gerrit/sshd/commands/StreamEvents.java
index 447f7ec..c680d30 100644
--- a/java/com/google/gerrit/sshd/commands/StreamEvents.java
+++ b/java/com/google/gerrit/sshd/commands/StreamEvents.java
@@ -87,29 +87,6 @@
     EventTypes.register(DroppedOutputEvent.TYPE, DroppedOutputEvent.class);
   }
 
-  private final CancelableRunnable writer =
-      new CancelableRunnable() {
-        @Override
-        public void run() {
-          writeEvents();
-        }
-
-        @Override
-        public void cancel() {
-          onExit(0);
-        }
-
-        @Override
-        public String toString() {
-          StringBuilder b = new StringBuilder();
-          b.append("Stream Events");
-          if (currentUser.getUserName().isPresent()) {
-            b.append(" (").append(currentUser.getUserName().get()).append(")");
-          }
-          return b.toString();
-        }
-      };
-
   /** True if {@link DroppedOutputEvent} needs to be sent. */
   private volatile boolean dropped;
 
@@ -127,8 +104,6 @@
    */
   private Future<?> task;
 
-  private PrintWriter stdout;
-
   @Override
   public void start(Environment env) throws IOException {
     try {
@@ -144,7 +119,30 @@
       return;
     }
 
-    stdout = toPrintWriter(out);
+    PrintWriter stdout = toPrintWriter(out);
+    CancelableRunnable writer =
+        new CancelableRunnable() {
+          @Override
+          public void run() {
+            writeEvents(this, stdout);
+          }
+
+          @Override
+          public void cancel() {
+            onExit(0);
+          }
+
+          @Override
+          public String toString() {
+            StringBuilder b = new StringBuilder();
+            b.append("Stream Events");
+            if (currentUser.getUserName().isPresent()) {
+              b.append(" (").append(currentUser.getUserName().get()).append(")");
+            }
+            return b.toString();
+          }
+        };
+
     eventListenerRegistration =
         eventListeners.add(
             "gerrit",
@@ -152,7 +150,7 @@
               @Override
               public void onEvent(Event event) {
                 if (subscribedToEvents.isEmpty() || subscribedToEvents.contains(event.getType())) {
-                  offer(event);
+                  offer(writer, event);
                 }
               }
 
@@ -199,7 +197,7 @@
     }
   }
 
-  private void offer(Event event) {
+  private void offer(CancelableRunnable writer, Event event) {
     synchronized (taskLock) {
       if (!queue.offer(event)) {
         dropped = true;
@@ -221,7 +219,7 @@
     }
   }
 
-  private void writeEvents() {
+  private void writeEvents(CancelableRunnable writer, PrintWriter stdout) {
     int processed = 0;
 
     while (processed < BATCH_SIZE) {
@@ -231,13 +229,13 @@
         // accepting output. Either way terminate this instance.
         //
         removeEventListenerRegistration();
-        flush();
+        flush(stdout);
         onExit(0);
         return;
       }
 
       if (dropped) {
-        write(new DroppedOutputEvent());
+        write(stdout, new DroppedOutputEvent());
         dropped = false;
       }
 
@@ -246,11 +244,11 @@
         break;
       }
 
-      write(event);
+      write(stdout, event);
       processed++;
     }
 
-    flush();
+    flush(stdout);
 
     if (BATCH_SIZE <= processed) {
       // We processed the limit, but more might remain in the queue.
@@ -263,7 +261,7 @@
     }
   }
 
-  private void write(Object message) {
+  private void write(PrintWriter stdout, Object message) {
     String msg = null;
     try {
       msg = gson.toJson(message) + "\n";
@@ -277,7 +275,7 @@
     }
   }
 
-  private void flush() {
+  private void flush(PrintWriter stdout) {
     synchronized (stdout) {
       stdout.flush();
     }

tools/BUILD

diff --git a/tools/BUILD b/tools/BUILD
index ceb5c3d..65ac23d 100644
--- a/tools/BUILD
+++ b/tools/BUILD
@@ -91,7 +91,7 @@
         "-Xep:SimpleDateFormatConstant:ERROR",
         "-Xep:StaticGuardedByInstance:ERROR",
         "-Xep:StringEquality:ERROR",
-        #"-Xep:SynchronizeOnNonFinalField:ERROR",
+        "-Xep:SynchronizeOnNonFinalField:ERROR",
         "-Xep:TruthConstantAsserts:ERROR",
         "-Xep:TypeParameterShadowing:ERROR",
         "-Xep:TypeParameterUnusedInFormals:ERROR",