Repository: gocd/gocd
Issue: Double-Checked Locking might be incorrect in PipelineGroups

https://github.com/gocd/gocd/blob/86b555e57daccec6b53b246f0dcdd4d0090566ef/config/config-api/src/main/java/com/thoughtworks/go/domain/PipelineGroups.java#L183-L187

Double-Checked Locking is widely cited and used as an efficient method for implementing lazy initialization in a multithreaded environment.
Unfortunately, it will not work reliably in a platform independent way when implemented in Java, without additional synchronization.

Modify the variable ‘packageToPipelineMap’ with volatile to tackle the problem.

Changed files:
domain/src/main/java/com/thoughtworks/go/domain/materials/tfs/TfsSDKCommandBuilder.java
config/config-api/src/main/java/com/thoughtworks/go/domain/PipelineGroups.java

diff --git a/config/config-api/src/main/java/com/thoughtworks/go/domain/PipelineGroups.java b/config/config-api/src/main/java/com/thoughtworks/go/domain/PipelineGroups.java
index 02747ee396..abaa4e98d8 100644
--- a/config/config-api/src/main/java/com/thoughtworks/go/domain/PipelineGroups.java
+++ b/config/config-api/src/main/java/com/thoughtworks/go/domain/PipelineGroups.java
@@ -21,8 +21,6 @@ import com.thoughtworks.go.config.exceptions.RecordNotFoundException;
 import com.thoughtworks.go.config.exceptions.UnprocessableEntityException;
 import com.thoughtworks.go.config.materials.PackageMaterialConfig;
 import com.thoughtworks.go.config.materials.PluggableSCMMaterialConfig;
-import com.thoughtworks.go.config.materials.ScmMaterialConfig;
-import com.thoughtworks.go.domain.materials.MaterialConfig;
 import com.thoughtworks.go.domain.packagerepository.PackageDefinition;
 import com.thoughtworks.go.domain.packagerepository.PackageRepository;
 import com.thoughtworks.go.domain.scm.SCM;
@@ -33,8 +31,8 @@ import java.util.*;
 @ConfigCollection(value = BasicPipelineConfigs.class)
 public class PipelineGroups extends BaseCollection<PipelineConfigs> implements Validatable {
     private final ConfigErrors configErrors = new ConfigErrors();
-    private Map<String, List<Pair<PipelineConfig, PipelineConfigs>>> packageToPipelineMap;
-    private Map<String, List<Pair<PipelineConfig, PipelineConfigs>>> pluggableSCMMaterialToPipelineMap;
+    private volatile Map<String, List<Pair<PipelineConfig, PipelineConfigs>>> packageToPipelineMap;
+    private volatile Map<String, List<Pair<PipelineConfig, PipelineConfigs>>> pluggableSCMMaterialToPipelineMap;
 
     public PipelineGroups() {
     }
@@ -235,10 +233,7 @@ public class PipelineGroups extends BaseCollection<PipelineConfigs> implements V
 
     public boolean canDeletePluggableSCMMaterial(SCM scmConfig) {
         Map<String, List<Pair<PipelineConfig, PipelineConfigs>>> packageUsageInPipelines = getPluggableSCMMaterialUsageInPipelines();
-        if (packageUsageInPipelines.containsKey(scmConfig.getId())) {
-            return false;
-        }
-        return true;
+        return !packageUsageInPipelines.containsKey(scmConfig.getId());
     }
 
     public PipelineGroups getLocal() {
diff --git a/domain/src/main/java/com/thoughtworks/go/domain/materials/tfs/TfsSDKCommandBuilder.java b/domain/src/main/java/com/thoughtworks/go/domain/materials/tfs/TfsSDKCommandBuilder.java
index 553ea6019b..93b5f1315f 100644
--- a/domain/src/main/java/com/thoughtworks/go/domain/materials/tfs/TfsSDKCommandBuilder.java
+++ b/domain/src/main/java/com/thoughtworks/go/domain/materials/tfs/TfsSDKCommandBuilder.java
@@ -22,6 +22,7 @@ import com.thoughtworks.go.util.command.CommandArgument;
 import com.thoughtworks.go.util.command.UrlArgument;
 import org.apache.commons.io.FileUtils;
 import org.apache.commons.io.IOUtils;
+import org.jetbrains.annotations.VisibleForTesting;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -30,7 +31,6 @@ import java.io.FileOutputStream;
 import java.io.IOException;
 import java.io.OutputStream;
 import java.lang.reflect.Constructor;
-import java.net.URISyntaxException;
 import java.net.URL;
 import java.nio.file.Paths;
 import java.util.jar.JarEntry;
@@ -44,16 +44,13 @@ class TfsSDKCommandBuilder {
     private static final Logger LOGGER = LoggerFactory.getLogger(TfsSDKCommandBuilder.class);
     private final File tempFolder = new File("data/tfs-sdk");
     private final ClassLoader sdkLoader;
-    private static TfsSDKCommandBuilder ME;
+    private static volatile TfsSDKCommandBuilder ME;
 
-    private TfsSDKCommandBuilder() throws IOException, URISyntaxException {
+    private TfsSDKCommandBuilder() throws IOException {
         this.sdkLoader = initSdkLoader();
     }
 
-    /*
-     * Used in tests
-     */
-    @Deprecated
+    @VisibleForTesting
     TfsSDKCommandBuilder(ClassLoader sdkLoader) {
         this.sdkLoader = sdkLoader;
     }
@@ -61,7 +58,7 @@ class TfsSDKCommandBuilder {
     TfsCommand buildTFSSDKCommand(String materialFingerPrint, UrlArgument url, String domain, String userName,
                                   String password, final String computedWorkspaceName, String projectPath) {
         try {
-            return instantitateAdapter(materialFingerPrint, url, domain, userName, password, computedWorkspaceName, projectPath);
+            return instantiateAdapter(materialFingerPrint, url, domain, userName, password, computedWorkspaceName, projectPath);
         } catch (Exception e) {
             String message = "[TFS SDK] Could not create TFS SDK Command ";
             LOGGER.error(message, e);
@@ -69,7 +66,7 @@ class TfsSDKCommandBuilder {
         }
     }
 
-    private TfsCommand instantitateAdapter(String materialFingerPrint, UrlArgument url, String domain, String userName, String password, String computedWorkspaceName, String projectPath) throws ReflectiveOperationException {
+    private TfsCommand instantiateAdapter(String materialFingerPrint, UrlArgument url, String domain, String userName, String password, String computedWorkspaceName, String projectPath) throws ReflectiveOperationException {
         Class<?> adapterClass = Class.forName(tfsSdkCommandTCLAdapterClassName(), true, sdkLoader);
         Constructor<?> constructor = adapterClass.getConstructor(String.class, CommandArgument.class, String.class, String.class, String.class, String.class, String.class);
         return (TfsCommand) constructor.newInstance(materialFingerPrint, url, domain, userName, password, computedWorkspaceName, projectPath);
@@ -79,7 +76,7 @@ class TfsSDKCommandBuilder {
         return "com.thoughtworks.go.tfssdk.TfsSDKCommandTCLAdapter";
     }
 
-    static TfsSDKCommandBuilder getBuilder() throws IOException, URISyntaxException {
+    static TfsSDKCommandBuilder getBuilder() throws IOException {
         if (ME == null) {
             synchronized (TfsSDKCommandBuilder.class) {
                 if (ME == null) {