ZOOKEEPER-3223: Configure Spotbugs

- add spotbugs configuration (default)
- make build pass spotbugs

Author: Enrico Olivelli <eolivelli@apache.org>

Reviewers: andor@apache.org

Closes #742 from eolivelli/fix/ZOOKEEPER-3223-spotbugs and squashes the following commits:

a43cecf41 [Enrico Olivelli] Fix false positive
c00d296ad [Enrico Olivelli] Add Suppression for false positive
35c8a4dde [Enrico Olivelli] fix tests
1ae629bcd [Enrico Olivelli] revert file
c0bb9d903 [Enrico Olivelli] Add spotbugs annotations to ant based build
dabe4fafc [Enrico Olivelli] [ZOOKEEPER-3223] Configure Spotbugs - add spotbugs configuration - make build pass spotbugs

build.xml

@@ -28,6 +28,7 @@ xmlns:cs="antlib:com.puppycrawl.tools.checkstyle.ant">
     <!-- ====================================================== -->
     <property name="slf4j.version" value="1.7.25"/>
     <property name="commons-cli.version" value="1.2"/>
+    <property name="spotbugsannotations.version" value="3.1.8"/>
 
     <property name="wagon-http.version" value="2.4"/>
     <property name="maven-ant-tasks.version" value="2.1.3"/>

excludeFindBugsFilter.xml

@@ -0,0 +1,14 @@
+<?xml version="1.0" encoding="UTF-8"?>
+
+<FindBugsFilter>
+    <!-- this work work on JDK11  https://github.com/spotbugs/spotbugs-maven-plugin/issues/92 -->
+    <Bug pattern="RCN_REDUNDANT_NULLCHECK_WOULD_HAVE_BEEN_A_NPE"/>
+
+    <!-- this problem is to be addressed in ZOOKEEPER-3227 -->
+    <Bug pattern="DM_DEFAULT_ENCODING"/>
+
+    <!-- not really a problem -->
+    <Bug pattern="DM_EXIT"/>
+
+</FindBugsFilter>
+

ivy.xml

@@ -46,6 +46,7 @@
     <dependency org="org.slf4j" name="slf4j-api" rev="${slf4j.version}"/>
     <dependency org="org.slf4j" name="slf4j-log4j12" rev="${slf4j.version}" transitive="false"/>
     <dependency org="commons-cli" name="commons-cli" rev="${commons-cli.version}" />
+    <dependency org="com.github.spotbugs" name="spotbugs-annotations" rev="${spotbugsannotations.version}" />
   
     <dependency org="org.apache.maven.wagon" name="wagon-http" rev="${wagon-http.version}"
                 conf="mvn-ant-task->default"/>

pom.xml

@@ -276,6 +276,7 @@
     <kerby.version>1.1.0</kerby.version>
     <bouncycastle.version>1.56</bouncycastle.version>
     <commons-collections.version>3.2.2</commons-collections.version>
+    <spotbugsannotations.version>3.1.8</spotbugsannotations.version>
   </properties>
 
   <dependencyManagement>
@@ -408,6 +409,13 @@
         <artifactId>jline</artifactId>
         <version>${jline.version}</version>
       </dependency>
+      <dependency>
+         <groupId>com.github.spotbugs</groupId>
+         <artifactId>spotbugs-annotations</artifactId>
+         <version>${spotbugsannotations.version}</version>
+         <scope>provided</scope>
+         <optional>true</optional>
+        </dependency>
     </dependencies>
   </dependencyManagement>
 
@@ -459,6 +467,14 @@
           <artifactId>clover-maven-plugin</artifactId>
           <version>4.3.1</version>
         </plugin>
+        <plugin>
+          <groupId>com.github.spotbugs</groupId>
+          <artifactId>spotbugs-maven-plugin</artifactId>
+          <version>3.1.8</version>
+          <configuration>
+            <excludeFilterFile>excludeFindBugsFilter.xml</excludeFilterFile>
+          </configuration>
+        </plugin>
       </plugins>
     </pluginManagement>
 
@@ -486,7 +502,11 @@
           </execution>
         </executions>
       </plugin>
-    </plugins>
+      <plugin>
+        <groupId>com.github.spotbugs</groupId>
+        <artifactId>spotbugs-maven-plugin</artifactId>
+      </plugin>
+      </plugins>
   </build>
 
   <reporting>

zookeeper-jute/pom.xml

@@ -145,6 +145,14 @@
           </execution>
         </executions>
       </plugin>
+      <plugin>
+        <!-- spotbugs does not make sense for generated code -->
+        <groupId>com.github.spotbugs</groupId>
+        <artifactId>spotbugs-maven-plugin</artifactId>
+        <configuration>
+            <skip>true</skip>
+        </configuration>
+      </plugin>
     </plugins>
   </build>
 

zookeeper-server/pom.xml

@@ -34,6 +34,12 @@
   <description>ZooKeeper server</description>
 
   <dependencies>
+    <dependency>
+      <groupId>com.github.spotbugs</groupId>
+      <artifactId>spotbugs-annotations</artifactId>
+      <scope>provided</scope>
+      <optional>true</optional>
+    </dependency>
     <dependency>
       <groupId>org.hamcrest</groupId>
       <artifactId>hamcrest-all</artifactId>

zookeeper-server/src/main/java/org/apache/zookeeper/ClientCnxn.java

@@ -18,6 +18,7 @@
 
 package org.apache.zookeeper;
 
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
 import java.io.BufferedReader;
 import java.io.ByteArrayOutputStream;
 import java.io.IOException;
@@ -97,6 +98,7 @@ import org.slf4j.MDC;
  * connected to as needed.
  *
  */
+@SuppressFBWarnings({"EI_EXPOSE_REP", "EI_EXPOSE_REP2"})
 public class ClientCnxn {
     private static final Logger LOG = LoggerFactory.getLogger(ClientCnxn.class);
 
@@ -479,6 +481,7 @@ public class ClientCnxn {
             waitingEvents.add(new LocalCallback(cb, rc, path, ctx));
         }
 
+       @SuppressFBWarnings("JLM_JSR166_UTILCONCURRENT_MONITORENTER")
        public void queuePacket(Packet packet) {
           if (wasKilled) {
              synchronized (waitingEvents) {
@@ -495,6 +498,7 @@ public class ClientCnxn {
         }
 
         @Override
+        @SuppressFBWarnings("JLM_JSR166_UTILCONCURRENT_MONITORENTER")
         public void run() {
            try {
               isRunning = true;

zookeeper-server/src/main/java/org/apache/zookeeper/ZooDefs.java

@@ -18,6 +18,7 @@
 
 package org.apache.zookeeper;
 
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
 import java.util.ArrayList;
 import java.util.Collections;
 
@@ -118,18 +119,21 @@ public class ZooDefs {
         /**
          * This is a completely open ACL .
          */
+        @SuppressFBWarnings(value = "MS_MUTABLE_COLLECTION", justification = "Cannot break API")
         public final ArrayList<ACL> OPEN_ACL_UNSAFE = new ArrayList<ACL>(
                 Collections.singletonList(new ACL(Perms.ALL, ANYONE_ID_UNSAFE)));
 
         /**
          * This ACL gives the creators authentication id's all permissions.
          */
+        @SuppressFBWarnings(value = "MS_MUTABLE_COLLECTION", justification = "Cannot break API")
         public final ArrayList<ACL> CREATOR_ALL_ACL = new ArrayList<ACL>(
                 Collections.singletonList(new ACL(Perms.ALL, AUTH_IDS)));
 
         /**
          * This ACL gives the world the ability to read.
          */
+        @SuppressFBWarnings(value = "MS_MUTABLE_COLLECTION", justification = "Cannot break API")
         public final ArrayList<ACL> READ_ACL_UNSAFE = new ArrayList<ACL>(
                 Collections
                         .singletonList(new ACL(Perms.READ, ANYONE_ID_UNSAFE)));

zookeeper-server/src/main/java/org/apache/zookeeper/server/DataNode.java

@@ -18,6 +18,7 @@
 
 package org.apache.zookeeper.server;
 
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
 import java.io.IOException;
 import java.util.HashSet;
 import java.util.Set;
@@ -36,6 +37,7 @@ import org.apache.zookeeper.data.StatPersisted;
  * array of ACLs, a stat object, and a set of its children's paths.
  * 
  */
+@SuppressFBWarnings("EI_EXPOSE_REP2")
 public class DataNode implements Record {
     /** the data for this datanode */
     byte data[];

zookeeper-server/src/main/java/org/apache/zookeeper/server/EphemeralType.java

@@ -18,6 +18,7 @@
 
 package org.apache.zookeeper.server;
 
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
 import org.apache.zookeeper.CreateMode;
 
 import java.util.Collections;
@@ -210,6 +211,8 @@ public enum EphemeralType {
      * @param ttl  ttl
      * @throws IllegalArgumentException if the ttl is not valid for the mode
      */
+    @SuppressFBWarnings(value = "RV_RETURN_VALUE_IGNORED_NO_SIDE_EFFECT",
+            justification = "toEphemeralOwner may throw IllegalArgumentException")
     public static void validateTTL(CreateMode mode, long ttl) {
         if (mode.isTTL()) {
             TTL.toEphemeralOwner(ttl);

zookeeper-server/src/main/java/org/apache/zookeeper/server/persistence/FileTxnLog.java

@@ -180,7 +180,7 @@ public class FileTxnLog implements TxnLog {
      * @param serverStats used to update fsyncThresholdExceedCount
      */
     @Override
-    public void setServerStats(ServerStats serverStats) {
+    public synchronized void setServerStats(ServerStats serverStats) {
         this.serverStats = serverStats;
     }
 

zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/AuthFastLeaderElection.java

@@ -18,6 +18,7 @@
 
 package org.apache.zookeeper.server.quorum;
 
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
 import java.io.IOException;
 import java.net.DatagramPacket;
 import java.net.DatagramSocket;
@@ -460,6 +461,8 @@ public class AuthFastLeaderElection implements Election {
                 }
             }
 
+            @SuppressFBWarnings(value = "RV_RETURN_VALUE_IGNORED",
+                    justification = "tryAcquire result not chacked, but it is not an issue")
             private void process(ToSend m) {
                 int attempts = 0;
                 byte zeroes[];

zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/CommitProcessor.java

@@ -18,6 +18,7 @@
 
 package org.apache.zookeeper.server.quorum;
 
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
 import java.io.IOException;
 import java.util.HashMap;
 import java.util.LinkedList;
@@ -395,6 +396,7 @@ public class CommitProcessor extends ZooKeeperCriticalThread implements
         }
     }
 
+    @SuppressFBWarnings("NN_NAKED_NOTIFY")
     synchronized private void wakeup() {
         notifyAll();
     }
@@ -416,6 +418,7 @@ public class CommitProcessor extends ZooKeeperCriticalThread implements
         wakeup();
     }
 
+    @Override
     public void processRequest(Request request) {
         if (stopped) {
             return;

zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/ObserverMaster.java

@@ -461,9 +461,14 @@ public class ObserverMaster implements LearnerMaster, Runnable {
     }
 
     public void run() {
+        ServerSocket ss;
+        synchronized(this) {
+             ss = this.ss;
+        }
         while (listenerRunning) {
             try {
                 Socket s = ss.accept();
+
                 // start with the initLimit, once the ack is processed
                 // in LearnerHandler switch to the syncLimit
                 s.setSoTimeout(self.tickTime * self.initLimit);

zookeeper-server/src/main/java/org/apache/zookeeper/server/util/BitHashSet.java

@@ -120,7 +120,10 @@ public class BitHashSet implements Iterable<Integer> {
      */
     @Override
     public Iterator<Integer> iterator() {
-        if (cache.size() == elementCount) {
+        // sample current size at the beginning
+        int currentSize = size();
+
+        if (cache.size() == currentSize) {
             return cache.iterator();
         }
 
@@ -130,7 +133,7 @@ public class BitHashSet implements Iterable<Integer> {
 
             @Override
             public boolean hasNext() {
-                return returnedCount < elementCount;
+                return returnedCount < currentSize;
             }
 
             @Override

zookeeper-server/src/main/java/org/apache/zookeeper/server/util/BitMap.java

@@ -18,6 +18,7 @@
 
 package org.apache.zookeeper.server.util;
 
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
 import java.util.Map;
 import java.util.HashMap;
 import java.util.BitSet;
@@ -37,6 +38,8 @@ public class BitMap<T> {
 
     private final ReentrantReadWriteLock rwLock = new ReentrantReadWriteLock();
 
+    @SuppressFBWarnings(value = "DLS_DEAD_LOCAL_STORE",
+            justification = "SpotBugs false positive")
     public Integer add(T value) {
         /*
          * Optimized for code which will add the same value again and again,