Fix issue JENKINS-17550, make DisplayAllocator singleton so it can ke…

…ep track of allocated displays. Also fixes JENKINS-17280.
jenkinsci · Apr 10, 2013 · 6bdd609 · olivergondza · Sep 19, 2014 · 6bdd609
1 parent 0e0f867
commit 6bdd609
Show file tree

Hide file tree

Showing 3 changed files with 55 additions and 52 deletions.
diff --git a/src/main/java/hudson/plugins/xvnc/DisplayAllocator.java b/src/main/java/hudson/plugins/xvnc/DisplayAllocator.java
@@ -15,24 +15,20 @@ final class DisplayAllocator {
      */
     private final Set<Integer> allocatedNumbers = new HashSet<Integer>();
     private final Set<Integer> blacklistedNumbers = new HashSet<Integer>();
-    private final int minDisplayNumber;
-    private final int maxDisplayNumber;
 
-    public DisplayAllocator(final int minDisplayNumber, final int maxDisplayNumber) {
-        this.minDisplayNumber = minDisplayNumber;
-        this.maxDisplayNumber = maxDisplayNumber;
+    public DisplayAllocator() {
     }
 
-    private final int getRandomValue() {
-        return minDisplayNumber + (new Random().nextInt(getRange()));
+    private final int getRandomValue(final int min, final int max) {
+        return min + (new Random().nextInt(getRange(min, max)));
     }
 
-    private int getRange() {
-        return (maxDisplayNumber + 1) - minDisplayNumber;
+    private int getRange(final int min, final int max) {
+        return (max + 1) - min;
     }
 
-    public synchronized int allocate() {
-        if (noDisplayNumbersLeft()) {
+    public synchronized int allocate(final int minDisplayNumber, final int maxDisplayNumber) {
+        if (noDisplayNumbersLeft(minDisplayNumber, maxDisplayNumber)) {
             if (!blacklistedNumbers.isEmpty()) {
                 blacklistedNumbers.clear();
             } else {
@@ -43,7 +39,7 @@ public synchronized int allocate() {
         }
         int displayNumber;
         do {
-            displayNumber = getRandomValue();
+            displayNumber = getRandomValue(minDisplayNumber, maxDisplayNumber);
         } while(isNotAvailable(displayNumber));
         allocatedNumbers.add(displayNumber);
         return displayNumber;
@@ -53,8 +49,8 @@ private boolean isNotAvailable(int number) {
         return allocatedNumbers.contains(number) || blacklistedNumbers.contains(number);
     }
 
-    private boolean noDisplayNumbersLeft() {
-        return allocatedNumbers.size() + blacklistedNumbers.size() >= getRange();
+    private boolean noDisplayNumbersLeft(final int min, final int max) {
+        return allocatedNumbers.size() + blacklistedNumbers.size() >= getRange(min, max);
     }
 
     public synchronized void free(int n) {

diff --git a/src/main/java/hudson/plugins/xvnc/Xvnc.java b/src/main/java/hudson/plugins/xvnc/Xvnc.java
@@ -30,7 +30,7 @@
 
 /**
  * {@link BuildWrapper} that runs <tt>xvnc</tt>.
- * 
+ *
  * @author Kohsuke Kawaguchi
  */
 public class Xvnc extends BuildWrapper {
@@ -39,11 +39,6 @@ public class Xvnc extends BuildWrapper {
      */
     public boolean takeScreenshot;
 
-    /**
-     * Manages display numbers in use.
-     */
-    private DisplayAllocator allocator;
-
     private static final String FILENAME_SCREENSHOT = "screenshot.jpg";
 
     @DataBoundConstructor
@@ -52,7 +47,7 @@ public Xvnc(boolean takeScreenshot) {
     }
 
     @Override
-    public Environment setUp(AbstractBuild build, final Launcher launcher, BuildListener listener) 
+    public Environment setUp(AbstractBuild build, final Launcher launcher, BuildListener listener)
             throws IOException, InterruptedException {
         final PrintStream logger = listener.getLogger();
         DescriptorImpl DESCRIPTOR = Hudson.getInstance().getDescriptorByType(DescriptorImpl.class);
@@ -62,28 +57,29 @@ public Environment setUp(AbstractBuild build, final Launcher launcher, BuildList
                 || build.getBuiltOn().getNodeProperties().get(NodePropertyImpl.class) != null) {
             return new Environment(){};
         }
-        
+
         if (DESCRIPTOR.skipOnWindows && !launcher.isUnix()) {
             return new Environment(){};
         }
-        
+
         if (DESCRIPTOR.cleanUp) {
             maybeCleanUp(launcher, listener);
         }
 
         String cmd = Util.nullify(DESCRIPTOR.xvnc);
-        allocator = new DisplayAllocator(DESCRIPTOR.minDisplayNumber, DESCRIPTOR.maxDisplayNumber);
         if (cmd == null) {
             cmd = "vncserver :$DISPLAY_NUMBER -localhost -nolisten tcp";
         }
 
-        return doSetUp(build, launcher, logger, cmd, 10);
+        return doSetUp(build, launcher, logger, cmd, 10, DESCRIPTOR.minDisplayNumber,
+                DESCRIPTOR.maxDisplayNumber);
     }
 
     private Environment doSetUp(AbstractBuild build, final Launcher launcher, final PrintStream logger,
-            String cmd, int retries) throws IOException, InterruptedException {
+            String cmd, int retries, int minDisplayNumber, int maxDisplayNumber)
+                    throws IOException, InterruptedException {
 
-        final int displayNumber = allocator.allocate();
+        final int displayNumber = allocator.allocate(minDisplayNumber, maxDisplayNumber);
         final String actualCmd = Util.replaceMacro(cmd, Collections.singletonMap("DISPLAY_NUMBER",String.valueOf(displayNumber)));
 
         logger.println(Messages.Xvnc_STARTING());
@@ -105,7 +101,8 @@ private Environment doSetUp(AbstractBuild build, final Launcher launcher, final
                 //allocator.free(displayNumber);
                 allocator.blacklist(displayNumber);
                 if (retries > 0) {
-                    return doSetUp(build, launcher, logger, cmd, retries - 1);
+                    return doSetUp(build, launcher, logger, cmd, retries - 1,
+                            minDisplayNumber, maxDisplayNumber);
                 } else {
                     throw new IOException(message);
                 }
@@ -147,11 +144,16 @@ public boolean tearDown(AbstractBuild build, BuildListener listener) throws IOEx
         };
     }
 
+    /**
+     * Manages display numbers in use.
+     */
+    private static final DisplayAllocator allocator = new DisplayAllocator();
+
     /**
      * Whether {@link #maybeCleanUp} has already been run on a given node.
      */
     private static final Map<Node,Boolean> cleanedUpOn = new WeakHashMap<Node,Boolean>();
-    
+
     // XXX I18N
     private static synchronized void maybeCleanUp(Launcher launcher, BuildListener listener) throws IOException, InterruptedException {
         Node node = Computer.currentComputer().getNode();
@@ -168,10 +170,10 @@ private static synchronized void maybeCleanUp(Launcher launcher, BuildListener l
         launcher.launch().stdout(logger).cmds("pkill", "Xrealvnc").join();
         launcher.launch().stdout(logger).cmds("sh", "-c", "rm -f /tmp/.X*-lock /tmp/.X11-unix/X*").join();
     }
-    
+
     @Extension
     public static final class DescriptorImpl extends BuildWrapperDescriptor {
-        
+
         /**
          * xvnc command line. This can include macro.
          *
@@ -180,20 +182,20 @@ public static final class DescriptorImpl extends BuildWrapperDescriptor {
         public String xvnc;
 
         /*
-         * Base X display number. 
+         * Base X display number.
          */
         public int minDisplayNumber = 10;
 
         /*
-         * Maximum X display number. 
+         * Maximum X display number.
          */
         public int maxDisplayNumber = 99;
 
         /**
          * If true, skip xvnc launch on all Windows slaves.
          */
         public boolean skipOnWindows = true;
-        
+
         /**
          * If true, try to clean up old processes and locks when first run.
          */

diff --git a/src/test/java/hudson/plugins/xvnc/DisplayAllocatorTest.java b/src/test/java/hudson/plugins/xvnc/DisplayAllocatorTest.java
@@ -8,88 +8,93 @@
 
 import java.util.Arrays;
 
+import org.junit.Before;
 import org.junit.Test;
 
 public class DisplayAllocatorTest {
 
     private static final int MAX = 3;
     private static final int MIN = 0;
+    private DisplayAllocator allocator;
+
+    @Before
+    public void setup() {
+        allocator = new DisplayAllocator();
+    }
 
     @Test
     public void throwsExceptionWhenAllDisplaysTaken() {
-        DisplayAllocator allocator = new DisplayAllocator(MIN, MAX);
         int range = MAX - MIN + 1;
         for (int i=0; i<range; i++) {
-            allocator.allocate();
+            allocate();
         }
         try {
-            allocator.allocate();
+            allocate();
             fail("Expected exception because all displays are allocated, none received");
         } catch(RuntimeException e) {
         }
     }
 
+    private int allocate() {
+        return allocator.allocate(MIN, MAX);
+    }
+
     @Test
     public void doesNotAllocateBlacklistedDisplays() {
-        DisplayAllocator allocator = new DisplayAllocator(MIN, MAX);
         int badDisplay = MIN + 1;
         int range = MAX - MIN + 1 - 1; // one bad display
         allocator.blacklist(badDisplay);
         for (int i=0; i<range; i++) {
-            assertThat(allocator.allocate(), not(equalTo(badDisplay)));
+            assertThat(allocate(), not(equalTo(badDisplay)));
         }
     }
 
     @Test
     public void clearsBlacklistWhenAllDisplaysTaken() {
-        DisplayAllocator allocator = new DisplayAllocator(MIN, MAX);
         int badDisplay = MIN + 1;
         allocator.blacklist(badDisplay);
         int range = MAX - MIN + 1 - 1;
         for (int i=0; i<range; i++) {
-            assertThat(allocator.allocate(), not(equalTo(badDisplay)));
+            assertThat(allocate(), not(equalTo(badDisplay)));
         }
-        assertThat(allocator.allocate(), equalTo(badDisplay));
+        assertThat(allocate(), equalTo(badDisplay));
         // now all displays are taken
         try {
-            allocator.allocate();
+            allocate();
             fail("Expected exception because all displays are allocated, none received");
         } catch(RuntimeException e) {
         }
     }
 
     @Test
     public void freesPortWhenBlacklisted() {
-        DisplayAllocator allocator = new DisplayAllocator(0, 1);
-        int first = allocator.allocate();
-        int second = allocator.allocate();
+        allocator.allocate(0, 1);
+        int second = allocator.allocate(0, 1);
         allocator.blacklist(second);
-        assertThat(allocator.allocate(), equalTo(second));
+        assertThat(allocator.allocate(0, 1), equalTo(second));
     }
 
     @Test
     public void canHandleWhenAllDisplaysAreBlacklisted() {
-        DisplayAllocator allocator = new DisplayAllocator(MIN, MAX);
         int range = MAX - MIN + 1;
         for (int i=0; i<range; i++) {
-            allocator.allocate();
+            allocate();
             allocator.blacklist(i);
         }
         try {
-            allocator.allocate();
+            allocate();
         } catch(RuntimeException e) {
             fail("Blacklist should have been cleared.");
         }
     }
 
     @Test
     public void doesReturnAllNumbersInRangeInclusive() {
-        DisplayAllocator allocator = new DisplayAllocator(MIN, MAX);
         int range = MAX - MIN + 1;
         int[] displays = new int[range];
         int[] expected = new int[range];
         for (int i=0; i<range; i++) {
-            displays[i] = allocator.allocate();
+            displays[i] = allocate();
             expected[i] = i;
         }
         Arrays.sort(displays);