[JENKINS-22395]

I'm not sure if I understand how the original fix in PR #1190 (f1430a2) addresses the problem. My understanding is of the problem is as follows: - someone deletes 'b2' but holds on to its reference (could be different threads) - someone calls b2.getPreviousBuild() - if b2.previousBuildR is null, then this triggers the loading of b1, and that process sets up a bi-di link via b1<->b2 via b1.nextBuildR <-> b2.previousBuildR - this makes b1.getNextBuild() incorrectly return b2 Presumably f1430a2 addresses this somehow, but I think I can induce this situation in other ways, which is what dropLinksAfterGC2() does. In this test, initial state: b1 <-> b2 <-> b3 we load everyone and connect them all up after deleting b2: b1 <--------> b3 ^ ^ +---- b2 ----+ b1 and b3 points each other, and b2 still refers to each side after dropping b1: b2 --> b3 now, here, when I do b2.getPreviousBuild(), it loads b1a and it sets b1a.nextBuildR to b2. b1a <-> b2 --> b3 So I claim this is a proof that the fix is incomplete, even for the problem JENKINS-22395 has discovered. I don't think that the problem is for the dropLinks call to fail to patch up references. The problem is that b2.getPreviousBuild() forcing b1 to point to b2, because if b2 is deleted and assumed to be invalid, then no matter what bX this method will find you never want bX.nextBuildR to point to b2. (cherry picked from commit 53e95ee)
jenkinsci · Aug 1, 2014 · 2baa7ca · 2baa7ca
1 parent 58e6c7b
commit 2baa7ca
Showing 1 changed file with 23 additions and 1 deletion.
diff --git a/test/src/test/java/jenkins/model/lazy/LazyBuildMixInTest.java b/test/src/test/java/jenkins/model/lazy/LazyBuildMixInTest.java
@@ -33,6 +33,7 @@
 import org.junit.Rule;
 import org.junit.Test;
 import org.jvnet.hudson.test.Bug;
+import org.jvnet.hudson.test.Issue;
 import org.jvnet.hudson.test.JenkinsRule;
 import org.jvnet.hudson.test.SleepBuilder;
 import org.jvnet.hudson.test.TestExtension;
@@ -60,10 +61,31 @@ public class LazyBuildMixInTest {
         assertEquals(1, b1a.getNumber());
         assertEquals(b3, b1a.getNextBuild());
     }
+
+    @Issue("JENKINS-22395")
+    @Test public void dropLinksAfterGC2() throws Exception {
+        FreeStyleProject p = r.createFreeStyleProject();
+        FreeStyleBuild b1 = r.buildAndAssertSuccess(p);
+        FreeStyleBuild b2 = r.buildAndAssertSuccess(p);
+        FreeStyleBuild b3 = r.buildAndAssertSuccess(p);
+        assertEquals(b2, b1.getNextBuild());
+        assertEquals(b3, b2.getNextBuild());
+        assertEquals(null, b3.getNextBuild());
+        assertEquals(null, b1.getPreviousBuild());
+        assertEquals(b1, b2.getPreviousBuild());
+        assertEquals(b2, b3.getPreviousBuild());
+        b2.delete();
+        assertEquals(1, BRHF.drop(b1));
+        FreeStyleBuild b1a = b2.getPreviousBuild();
+        assertNotSame(b1, b1a);
+        assertEquals(1, b1a.getNumber());
+        assertEquals(b3, b1a.getNextBuild());
+    }
+
     /**
      * Unlike the standard {@link SoftReference} this lets us simulate a referent disappearing at a specific time.
      */
-    @TestExtension("dropLinksAfterGC") public static final class BRHF implements BuildReference.HolderFactory {
+    @TestExtension public static final class BRHF implements BuildReference.HolderFactory {
         private static final List<BRH<?>> refs = new ArrayList<BRH<?>>();
         private static final class BRH<R> implements BuildReference.Holder<R> {
             R r;