[FIXED HUDSON-4202] in 1.320. Test case confirmed my hypothesis.

git-svn-id: https://hudson.dev.java.net/svn/hudson/trunk/hudson/main@20630 71c3de6d-444a-0410-be80-ed276b4c234a

core/src/main/java/hudson/matrix/MatrixRun.java

@@ -129,7 +129,7 @@ public class MatrixRun extends Build<MatrixConfiguration,MatrixRun> {
 
     protected class RunnerImpl extends Build<MatrixConfiguration,MatrixRun>.RunnerImpl {
         @Override
-        protected FilePath decideWorkspace(Node n, WorkspaceList wsl) {
+        protected FilePath decideWorkspace(Node n, WorkspaceList wsl) throws InterruptedException, IOException {
             Node node = getBuiltOn();
             FilePath ws = node.getWorkspaceFor(getParent().getParent());
             if(useShortWorkspaceName)

core/src/main/java/hudson/model/AbstractBuild.java

@@ -316,7 +316,7 @@ public abstract class AbstractBuild<P extends AbstractProject<P,R>,R extends Abs
          * @param wsl
          *      Passed in for the convenience. The returned path must be registered to this object.
          */
-        protected FilePath decideWorkspace(Node n, WorkspaceList wsl) {
+        protected FilePath decideWorkspace(Node n, WorkspaceList wsl) throws InterruptedException, IOException {
             // TODO: this cast is indicative of abstraction problem
             return wsl.allocate(n.getWorkspaceFor((TopLevelItem)getProject()));
         }

core/src/main/java/hudson/model/AbstractProject.java

@@ -945,7 +945,13 @@ public abstract class AbstractProject<P extends AbstractProject<P,R>,R extends A
                     return true;
                 } else {
                     WorkspaceList l = lb.getBuiltOn().toComputer().getWorkspaceList();
-                    l.acquire(ws);
+                    // if doing non-concurrent build, acquite a workspace in a way that causes builds to block for this workspace.
+                    // this prevents multiple workspaces of the same job --- the behavior of Hudson < 1.319.
+                    //
+                    // OTOH, if a concurrent build is chosen, the user is willing to create a multiple workspace,
+                    // so better throughput is achieved over time (modulo the initial cost of creating that many workspaces)
+                    // by having multiple workspaces
+                    l.acquire(ws,!concurrentBuild);
                     try {
                         LOGGER.fine("Polling SCM changes of " + getName());
                         return scm.pollChanges(this, ws.createLauncher(listener), ws, listener);

core/src/main/java/hudson/model/FreeStyleBuild.java

@@ -48,7 +48,7 @@ public class FreeStyleBuild extends Build<FreeStyleProject,FreeStyleBuild> {
 
     protected class RunnerImpl extends Build<FreeStyleProject,FreeStyleBuild>.RunnerImpl {
         @Override
-        protected FilePath decideWorkspace(Node n, WorkspaceList wsl) {
+        protected FilePath decideWorkspace(Node n, WorkspaceList wsl) throws IOException, InterruptedException {
             String customWorkspace = getProject().getCustomWorkspace();
             if (customWorkspace != null)
                 return n.createPath(customWorkspace);

core/src/main/java/hudson/slaves/WorkspaceList.java

@@ -24,10 +24,15 @@
 package hudson.slaves;
 
 import hudson.FilePath;
+import hudson.Util;
+import hudson.Functions;
 import hudson.model.Computer;
 
 import java.util.HashSet;
 import java.util.Set;
+import java.util.Map;
+import java.util.HashMap;
+import java.util.Date;
 import java.util.logging.Logger;
 import java.util.logging.Level;
 
@@ -42,22 +47,62 @@ import java.util.logging.Level;
  * @see Computer#getWorkspaceList()
  */
 public final class WorkspaceList {
-    private final Set<FilePath> inUse = new HashSet<FilePath>();
+    /**
+     * Book keeping for workspace allocation.
+     */
+    public static final class Entry {
+        /**
+         * Who acquired this workspace?
+         */
+        public final Thread holder = Thread.currentThread();
+
+        /**
+         * When?
+         */
+        public final long time = System.currentTimeMillis();
+
+        /**
+         * From where?
+         */
+        public final Exception source = new Exception();
+
+        /**
+         * True makes the caller of {@link WorkspaceList#allocate(FilePath)} wait
+         * for this workspace.
+         */
+        public final boolean quick;
+
+        public final FilePath path;
+
+        private Entry(FilePath path, boolean quick) {
+            this.path = path;
+            this.quick = quick;
+        }
+
+        @Override
+        public String toString() {
+            String s = path+" owned by "+holder.getName()+" from "+new Date(time);
+            if(quick) s+=" (quick)";
+            s+="\n"+Functions.printThrowable(source);
+            return s;
+        }
+    }
+
+    private final Map<FilePath,Entry> inUse = new HashMap<FilePath,Entry>();
 
     public WorkspaceList() {
     }
 
     /**
-     * Allocates some workspace by adding some variation to the given base if necessary.
+     * Allocates a workspace by adding some variation to the given base to make it unique.
      */
-    public synchronized FilePath allocate(FilePath base) {
+    public synchronized FilePath allocate(FilePath base) throws InterruptedException {
         for (int i=1; ; i++) {
             FilePath candidate = i==1 ? base : base.withSuffix("@"+i);
-            if(inUse.contains(candidate))
+            Entry e = inUse.get(candidate);
+            if(e!=null && !e.quick)
                 continue;
-            inUse.add(candidate);
-            log("allocated  " + candidate);
-            return candidate;
+            return acquire(candidate);
         }
     }
 
@@ -66,8 +111,9 @@ public final class WorkspaceList {
      */
     public synchronized FilePath record(FilePath p) {
         log("recorded  "+p);
-        if (!inUse.add(p))
-            throw new AssertionError("Tried to record a workspace already owned");
+        Entry old = inUse.put(p, new Entry(p, false));
+        if (old!=null)
+            throw new AssertionError("Tried to record a workspace already owned: "+old);
         return p;
     }
 
@@ -75,7 +121,8 @@ public final class WorkspaceList {
      * Releases an allocated or acquired workspace.
      */
     public synchronized void release(FilePath p) {
-        if (!inUse.remove(p))
+        Entry old = inUse.remove(p);
+        if (old==null)
             throw new AssertionError("Releasing unallocated workspace "+p);
         notifyAll();
     }
@@ -87,10 +134,21 @@ public final class WorkspaceList {
      *      The same {@link FilePath} as given to this method.
      */
     public synchronized FilePath acquire(FilePath p) throws InterruptedException {
-        while (inUse.contains(p))
+        return acquire(p,false);
+    }
+
+    /**
+     * See {@link #acquire(FilePath)}
+     *
+     * @param quick
+     *      If true, indicates that the acquired workspace will be returned quickly.
+     *      This makes other calls to {@link #allocate(FilePath)} to wait for the release of this workspace.
+     */
+    public synchronized FilePath acquire(FilePath p, boolean quick) throws InterruptedException {
+        while (inUse.containsKey(p))
             wait();
         log("acquired "+p);
-        inUse.add(p);
+        inUse.put(p,new Entry(p,quick));
         return p;
     }
 

maven-plugin/src/main/java/hudson/maven/MavenBuild.java

@@ -477,7 +477,7 @@ public class MavenBuild extends AbstractMavenBuild<MavenModule,MavenBuild> {
         private List<MavenReporter> reporters;
 
         @Override
-        protected FilePath decideWorkspace(Node n, WorkspaceList wsl) {
+        protected FilePath decideWorkspace(Node n, WorkspaceList wsl) throws InterruptedException, IOException {
             return wsl.allocate(getParentBuild().getModuleRoot().child(getProject().getRelativePath()));
         }
 

test/src/test/java/hudson/model/AbstractProjectTest.java

@@ -29,10 +29,22 @@ import com.gargoylesoftware.htmlunit.html.HtmlForm;
 import com.gargoylesoftware.htmlunit.html.HtmlPage;
 import hudson.security.GlobalMatrixAuthorizationStrategy;
 import hudson.tasks.Shell;
+import hudson.scm.SCM;
+import hudson.scm.ChangeLogParser;
+import hudson.scm.NullSCM;
+import hudson.Launcher;
+import hudson.FilePath;
+import hudson.util.StreamTaskListener;
+import hudson.util.OneShotEvent;
 import org.jvnet.hudson.test.HudsonTestCase;
+import org.jvnet.hudson.test.Bug;
 import org.jvnet.hudson.test.recipes.PresetData;
 import org.jvnet.hudson.test.recipes.PresetData.DataSet;
 
+import java.io.IOException;
+import java.io.File;
+import java.util.concurrent.Future;
+
 /**
  * @author Kohsuke Kawaguchi
  */
@@ -117,4 +129,54 @@ public class AbstractProjectTest extends HudsonTestCase {
             assertEquals(b,p.isConcurrentBuild());
         }
     }
+
+    /**
+     * Unless the concurrent build option is enabled, polling and build should be mutually exclusive
+     * to avoid allocating unnecessary workspaces.
+     */
+    @Bug(4202)
+    public void testPollingAndBuildExclusion() throws Exception {
+        final OneShotEvent sync = new OneShotEvent();
+
+        final FreeStyleProject p = createFreeStyleProject();
+        FreeStyleBuild b1 = assertBuildStatusSuccess(p.scheduleBuild2(0).get());
+
+        p.setScm(new NullSCM() {
+            @Override
+            public boolean pollChanges(AbstractProject project, Launcher launcher, FilePath workspace, TaskListener listener) {
+                try {
+                    sync.block();
+                } catch (InterruptedException e) {
+                    e.printStackTrace();
+                }
+                return true;
+            }
+
+            public boolean requiresWorkspaceForPolling() {
+                return true;
+            }
+        });
+        Thread t = new Thread() {
+            public void run() {
+                p.pollSCMChanges(new StreamTaskListener(System.out));
+            }
+        };
+        try {
+            t.start();
+            Future<FreeStyleBuild> f = p.scheduleBuild2(0);
+
+            // add a bit of delay to make sure that the blockage is happening
+            Thread.sleep(3000);
+
+            // release the polling
+            sync.signal();
+
+            FreeStyleBuild b2 = assertBuildStatusSuccess(f.get());
+
+            // they should have used the same workspace.
+            assertEquals(b1.getWorkspace(), b2.getWorkspace());
+        } finally {
+            t.interrupt();
+        }
+    }
 }