KNOX-1668 - Enable PMD multithreading ruleset

Signed-off-by: Kevin Risden <[email protected]>

build-tools/src/main/resources/build-tools/pmd/pmd-ruleset.xml

@@ -65,7 +65,10 @@ limitations under the License.
         <exclude name="ReturnEmptyArrayRatherThanNull" />
         <exclude name="UseProperClassLoader" />
     </rule>
-    <!--<rule ref="category/java/multithreading.xml" />-->
+    <rule ref="category/java/multithreading.xml">
+        <exclude name="AvoidSynchronizedAtMethodLevel" />
+        <exclude name="UseConcurrentHashMap" />
+    </rule>
     <rule ref="category/java/performance.xml">
         <exclude name="AvoidInstantiatingObjectsInLoops" />
         <exclude name="InefficientEmptyStringCheck" />

build-tools/src/main/resources/build-tools/spotbugs-filter.xml

@@ -20,16 +20,6 @@ limitations under the License.
   xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
   xsi:schemaLocation="https://github.com/spotbugs/filter/3.0.0 https://raw.githubusercontent.com/spotbugs/spotbugs/3.1.0/spotbugs/etc/findbugsfilter.xsd">
 
-  <Match>
-    <Class name="org.apache.knox.gateway.i18n.messages.loggers.sout.SoutMessageLoggerFactory" />
-    <Bug pattern="ST_WRITE_TO_STATIC_FROM_INSTANCE_METHOD" />
-  </Match>
-
-  <Match>
-    <Class name="org.apache.knox.gateway.i18n.messages.loggers.test.TestMessageLoggerFactory" />
-    <Bug pattern="ST_WRITE_TO_STATIC_FROM_INSTANCE_METHOD" />
-  </Match>
-
   <Match>
     <Class name="org.apache.knox.gateway.security.ldap.SimpleLdapDirectoryServer" />
     <Bug pattern="PATH_TRAVERSAL_IN" />

gateway-discovery-ambari/src/main/java/org/apache/knox/gateway/topology/discovery/ambari/AmbariConfigurationMonitor.java

@@ -39,7 +39,6 @@
 import java.util.concurrent.locks.ReentrantReadWriteLock;
 
 class AmbariConfigurationMonitor implements ClusterConfigurationMonitor {
-
     private static final String TYPE = "Ambari";
 
     private static final String CLUSTERS_DATA_DIR_NAME = "clusters";
@@ -54,7 +53,6 @@ class AmbariConfigurationMonitor implements ClusterConfigurationMonitor {
 
     static final String INTERVAL_PROPERTY_NAME = "org.apache.knox.gateway.topology.discovery.ambari.monitor.interval";
 
-
     private static final AmbariServiceDiscoveryMessages log = MessagesFactory.get(AmbariServiceDiscoveryMessages.class);
 
     // Ambari address
@@ -263,6 +261,7 @@ private void addClusterConfigVersions(String address, String clusterName, Map<St
         }
     }
 
+    @SuppressWarnings("PMD.DoNotUseThreads")
     @Override
     public void start() {
         (new Thread(internalMonitor, "AmbariConfigurationMonitor")).start();
@@ -294,7 +293,6 @@ void addDiscoveryConfig(String clusterName, ServiceDiscoveryConfig config) {
         clusterMonitorConfigurations.computeIfAbsent(config.getAddress(), k -> new HashMap<>()).put(clusterName, config);
     }
 
-
     /**
      * Get the service discovery configuration associated with the specified Ambari instance and cluster.
      *
@@ -311,7 +309,6 @@ ServiceDiscoveryConfig getDiscoveryConfig(String address, String clusterName) {
         return config;
     }
 
-
     /**
      * Add cluster configuration data to the monitor, which it will use when determining if configuration has changed.
      *
@@ -342,7 +339,6 @@ void addClusterConfigVersions(AmbariCluster cluster, ServiceDiscoveryConfig disc
         addClusterConfigVersions(discoveryConfig.getAddress(), clusterName, configVersions);
     }
 
-
     /**
      * Remove the configuration record for the specified Ambari instance and cluster name.
      *
@@ -395,7 +391,6 @@ Map<String, String> getClusterConfigVersions(String address, String clusterName)
         return result;
     }
 
-
     /**
      * Get all the clusters the monitor knows about.
      *
@@ -415,10 +410,8 @@ Map<String, List<String>> getClusterNames() {
         }
 
         return result;
-
     }
 
-
     /**
      * Notify registered change listeners.
      *
@@ -431,7 +424,6 @@ void notifyChangeListeners(String source, String clusterName) {
         }
     }
 
-
     /**
      * Request the current active configuration version info from Ambari.
      *
@@ -458,11 +450,11 @@ Map<String, String> getUpdatedConfigVersions(String address, String clusterName)
         return configVersions;
     }
 
-
     /**
      * The thread that polls Ambari for configuration details for clusters associated with discovered topologies,
      * compares them with the current recorded values, and notifies any listeners when differences are discovered.
      */
+    @SuppressWarnings("PMD.DoNotUseThreads")
     static final class PollingConfigAnalyzer implements Runnable {
 
         private static final int DEFAULT_POLLING_INTERVAL = 60;
@@ -534,5 +526,4 @@ public void run() {
             }
         }
     }
-
 }

gateway-i18n/src/main/java/org/apache/knox/gateway/i18n/messages/MessagesFactory.java

@@ -49,7 +49,7 @@ private static MessageLoggerFactory getMessageLoggerFactory() {
     MessageLoggerFactory factory;
     ServiceLoader<MessageLoggerFactory> loader = ServiceLoader.load( MessageLoggerFactory.class );
     Iterator<MessageLoggerFactory> factories = loader.iterator();
-    if( factories != null && factories.hasNext() ) {
+    if(factories.hasNext()) {
       factory = loader.iterator().next();
     } else {
       factory = new JdkMessageLoggerFactory();

gateway-i18n/src/main/java/org/apache/knox/gateway/i18n/messages/loggers/jdk/JdkMessageLoggerFactory.java

@@ -23,10 +23,8 @@
 import java.util.logging.Logger;
 
 public class JdkMessageLoggerFactory implements MessageLoggerFactory {
-
   @Override
   public MessageLogger getLogger( String name ) {
     return new JdkMessageLogger( Logger.getLogger( name ) );
   }
-
 }

gateway-i18n/src/main/java/org/apache/knox/gateway/i18n/messages/loggers/sout/SoutMessageLoggerFactory.java

@@ -24,19 +24,14 @@
 import java.util.concurrent.ConcurrentHashMap;
 
 public class SoutMessageLoggerFactory implements MessageLoggerFactory {
-
-  private static SoutMessageLoggerFactory INSTANCE;
+  private static final SoutMessageLoggerFactory INSTANCE = new SoutMessageLoggerFactory();
   private static final Map<String,MessageLogger> LOGGERS = new ConcurrentHashMap<>();
 
   public static SoutMessageLoggerFactory getFactory() {
-    if( INSTANCE == null ) {
-      INSTANCE = new SoutMessageLoggerFactory();
-    }
     return INSTANCE;
   }
 
   public SoutMessageLoggerFactory() {
-    INSTANCE = this;
   }
 
   @Override
@@ -48,5 +43,4 @@ public MessageLogger getLogger( String name ) {
     }
     return logger;
   }
-
 }

gateway-i18n/src/main/java/org/apache/knox/gateway/i18n/messages/loggers/test/TestMessageLoggerFactory.java

@@ -24,19 +24,14 @@
 import java.util.concurrent.ConcurrentHashMap;
 
 public class TestMessageLoggerFactory implements MessageLoggerFactory {
-
-  private static TestMessageLoggerFactory INSTANCE;
+  private static final TestMessageLoggerFactory INSTANCE = new TestMessageLoggerFactory();
   private static final Map<String,MessageLogger> LOGGERS = new ConcurrentHashMap<>();
 
   public static TestMessageLoggerFactory getFactory() {
-    if( INSTANCE == null ) {
-      INSTANCE = new TestMessageLoggerFactory();
-    }
     return INSTANCE;
   }
 
   public TestMessageLoggerFactory() {
-    INSTANCE = this;
   }
 
   @Override
@@ -48,5 +43,4 @@ public MessageLogger getLogger( String name ) {
     }
     return logger;
   }
-
 }

gateway-i18n/src/test/java/org/apache/knox/gateway/i18n/messages/MessagesTest.java

@@ -45,7 +45,5 @@ public void testFirst() {
 
     assertThat( record.getCaller().getClassName(), is( this.getClass().getName() ) );
     assertThat( record.getCaller().getMethodName(), is( "testFirst" ) );
-
   }
-
 }

gateway-server/src/main/java/org/apache/knox/gateway/GatewayServlet.java

@@ -64,7 +64,7 @@ public class GatewayServlet implements Servlet, Filter {
           AuditConstants.KNOX_SERVICE_NAME, AuditConstants.KNOX_COMPONENT_NAME );
 
   private FilterConfigAdapter filterConfig;
-  private volatile GatewayFilter filter;
+  private GatewayFilter filter;
 
   public GatewayServlet( GatewayFilter filter ) {
     this.filterConfig = null;

gateway-server/src/main/java/org/apache/knox/gateway/services/topology/impl/DefaultTopologyService.java

@@ -111,7 +111,7 @@ public class DefaultTopologyService
   private DescriptorsMonitor descriptorsMonitor;
 
   private Set<TopologyListener> listeners;
-  private volatile Map<File, Topology> topologies;
+  private Map<File, Topology> topologies;
   private AliasService aliasService;
 
   private RemoteConfigurationMonitor remoteMonitor;

gateway-server/src/main/java/org/apache/knox/gateway/topology/monitor/RemoteConfigurationMonitorFactory.java

@@ -26,16 +26,15 @@
 import java.util.ServiceLoader;
 
 public class RemoteConfigurationMonitorFactory {
-
     private static final GatewayMessages log = MessagesFactory.get(GatewayMessages.class);
 
     private static RemoteConfigurationRegistryClientService remoteConfigRegistryClientService;
 
-    public static void setClientService(RemoteConfigurationRegistryClientService clientService) {
+    static void setClientService(RemoteConfigurationRegistryClientService clientService) {
         remoteConfigRegistryClientService = clientService;
     }
 
-    private static RemoteConfigurationRegistryClientService getClientService() {
+    private static synchronized RemoteConfigurationRegistryClientService getClientService() {
         if (remoteConfigRegistryClientService == null) {
             GatewayServices services = GatewayServer.getGatewayServices();
             if (services != null) {
@@ -70,5 +69,4 @@ public static RemoteConfigurationMonitor get(GatewayConfig config) {
 
         return rcm;
     }
-
 }

gateway-server/src/main/java/org/apache/knox/gateway/websockets/ProxyWebSocketAdapter.java

@@ -42,9 +42,7 @@
  * @since 0.10
  */
 public class ProxyWebSocketAdapter extends WebSocketAdapter {
-
-  private static final WebsocketLogMessages LOG = MessagesFactory
-      .get(WebsocketLogMessages.class);
+  private static final WebsocketLogMessages LOG = MessagesFactory.get(WebsocketLogMessages.class);
 
   /* URI for the backend */
   private final URI backend;
@@ -78,7 +76,6 @@ public ProxyWebSocketAdapter(final URI backend, final ExecutorService pool, fina
 
   @Override
   public void onWebSocketConnect(final Session frontEndSession) {
-
     /*
      * Let's connect to the backend, this is where the Backend-to-frontend
      * plumbing takes place
@@ -109,9 +106,7 @@ public void onWebSocketConnect(final Session frontEndSession) {
   }
 
   @Override
-  public void onWebSocketBinary(final byte[] payload, final int offset,
-      final int length) {
-
+  public void onWebSocketBinary(final byte[] payload, final int offset, final int length) {
     if (isNotConnected()) {
       return;
     }
@@ -122,7 +117,6 @@ public void onWebSocketBinary(final byte[] payload, final int offset,
 
   @Override
   public void onWebSocketText(final String message) {
-
     if (isNotConnected()) {
       return;
     }
@@ -136,23 +130,13 @@ public void onWebSocketText(final String message) {
     } catch (IOException e) {
       LOG.connectionFailed(e);
     }
-
   }
 
   @Override
   public void onWebSocketClose(int statusCode, String reason) {
     super.onWebSocketClose(statusCode, reason);
-
-    /* do the cleaning business in seperate thread so we don't block */
-    pool.execute(new Runnable() {
-      @Override
-      public void run() {
-        closeQuietly();
-      }
-    });
-
+    cleanup();
     LOG.onConnectionClose(backend.toString());
-
   }
 
   @Override
@@ -176,32 +160,21 @@ private void cleanupOnError(final Throwable t) {
       if(frontendSession != null && !frontendSession.isOpen()) {
         frontendSession.close(StatusCode.SERVER_ERROR, t.getMessage());
       }
-
-      /* do the cleaning business in seperate thread so we don't block */
-      pool.execute(new Runnable() {
-        @Override
-        public void run() {
-          closeQuietly();
-        }
-      });
-
+      cleanup();
     }
   }
 
   private MessageEventCallback getMessageCallback() {
-
     return new MessageEventCallback() {
 
       @Override
       public void doCallback(String message) {
         /* do nothing */
-
       }
 
       @Override
       public void onConnectionOpen(Object session) {
         /* do nothing */
-
       }
 
       @Override
@@ -210,17 +183,8 @@ public void onConnectionClose(final CloseReason reason) {
           frontendSession.close(reason.getCloseCode().getCode(),
               reason.getReasonPhrase());
         } finally {
-
-          /* do the cleaning business in seperate thread so we don't block */
-          pool.execute(new Runnable() {
-            @Override
-            public void run() {
-              closeQuietly();
-            }
-          });
-
+          cleanup();
         }
-
       }
 
       @Override
@@ -259,8 +223,18 @@ public void onMessageBinary(byte[] message, boolean last,
 
   }
 
-  private void closeQuietly() {
+  @SuppressWarnings("PMD.DoNotUseThreads")
+  private void cleanup() {
+    /* do the cleaning business in separate thread so we don't block */
+    pool.execute(new Runnable() {
+      @Override
+      public void run() {
+        closeQuietly();
+      }
+    });
+  }
 
+  private void closeQuietly() {
     try {
       if(backendSession != null && !backendSession.isOpen()) {
         backendSession.close();
@@ -280,7 +254,5 @@ private void closeQuietly() {
     if(frontendSession != null && !frontendSession.isOpen()) {
       frontendSession.close();
     }
-
   }
-
 }