Merely calling job.getBuilds().isEmpty() breaks lazy-loading:

      	at hudson.model.Run.onLoad(Run.java:319)
      	at hudson.model.RunMap.retrieve(RunMap.java:226)
      	at hudson.model.RunMap.retrieve(RunMap.java:59)
      	at jenkins.model.lazy.AbstractLazyLoadRunMap.load(AbstractLazyLoadRunMap.java:667)
      	at jenkins.model.lazy.AbstractLazyLoadRunMap.load(AbstractLazyLoadRunMap.java:650)
      	at jenkins.model.lazy.AbstractLazyLoadRunMap.all(AbstractLazyLoadRunMap.java:602)
      	at jenkins.model.lazy.AbstractLazyLoadRunMap.entrySet(AbstractLazyLoadRunMap.java:264)
      	at java.util.AbstractMap$2$1.<init>(AbstractMap.java:378)
      	at java.util.AbstractMap$2.iterator(AbstractMap.java:377)
      	at hudson.util.RunList$2.iterator(RunList.java:213)
      	at hudson.util.RunList.iterator(RunList.java:103)
      	at hudson.util.RunList.isEmpty(RunList.java:173)
      

      You would reasonably expect this method, and anything simply iterating builds, to be lazy, but it is eager.

          [JENKINS-18065] AbstractLazyLoadRunMap.iterator() calls .all()

          Code changed in jenkins
          User: Jesse Glick
          Path:
          core/src/test/java/jenkins/model/lazy/AbstractLazyLoadRunMapTest.java
          core/src/test/java/jenkins/model/lazy/FakeMap.java
          http://jenkins-ci.org/commit/jenkins/c3e73296b5af0a4aaa06a3bdf867ac50fe211888
          Log:
          JENKINS-18065 Uncommenting original test.
          The actual implementation does not behave as nicely as one would hope, for a few reasons:
          1. isEmpty actually tries to load the newestBuild, rather than simply checking whether there is some idOnDisk.
          Arguably this is necessary, since there could be an unloadable build record, in which case it would be technically correct for the map to be considered empty.
          2. Loading AbstractLazyLoadRunMap.newestBuild calls search(MAX_VALUE, DESC), which behaves oddly, by doing a binary search, and thus perhaps loading lg(|map|) entries.
          You would think that it would suffice to check for the last member of idOnDisk in index.byId.
          3. The iterator eagerly loads the next value before hasNext has even been called.
          Looks bad in a test, though it probably has little practical impact since most callers would be calling hasNext soon afterward anyway.
          Might cause one extra build record to be loaded unnecessarily from a limited RunList.

          (cherry picked from commit 2f58ceb1a66c8ed621fa2562c5ed3445b29ea200)

          SCM/JIRA link daemon added a comment - Code changed in jenkins User: Jesse Glick Path: core/src/test/java/jenkins/model/lazy/AbstractLazyLoadRunMapTest.java core/src/test/java/jenkins/model/lazy/FakeMap.java http://jenkins-ci.org/commit/jenkins/c3e73296b5af0a4aaa06a3bdf867ac50fe211888 Log: JENKINS-18065 Uncommenting original test. The actual implementation does not behave as nicely as one would hope, for a few reasons: 1. isEmpty actually tries to load the newestBuild, rather than simply checking whether there is some idOnDisk. Arguably this is necessary, since there could be an unloadable build record, in which case it would be technically correct for the map to be considered empty. 2. Loading AbstractLazyLoadRunMap.newestBuild calls search(MAX_VALUE, DESC), which behaves oddly, by doing a binary search, and thus perhaps loading lg(|map|) entries. You would think that it would suffice to check for the last member of idOnDisk in index.byId. 3. The iterator eagerly loads the next value before hasNext has even been called. Looks bad in a test, though it probably has little practical impact since most callers would be calling hasNext soon afterward anyway. Might cause one extra build record to be loaded unnecessarily from a limited RunList. (cherry picked from commit 2f58ceb1a66c8ed621fa2562c5ed3445b29ea200)

          dogfood added a comment -

          Integrated in jenkins_main_trunk #3512
          JENKINS-18065 54c0846 amendment: presumably RunMap.entrySet().iterator().next().setValue(…) should be illegal. (Revision da57ad5c30b1c9ca8365f869638abd29c8fb5896)

          Result = SUCCESS
          Jesse Glick : da57ad5c30b1c9ca8365f869638abd29c8fb5896
          Files :

          • core/src/main/java/jenkins/model/lazy/LazyLoadRunMapEntrySet.java

          dogfood added a comment - Integrated in jenkins_main_trunk #3512 JENKINS-18065 54c0846 amendment: presumably RunMap.entrySet().iterator().next().setValue(…) should be illegal. (Revision da57ad5c30b1c9ca8365f869638abd29c8fb5896) Result = SUCCESS Jesse Glick : da57ad5c30b1c9ca8365f869638abd29c8fb5896 Files : core/src/main/java/jenkins/model/lazy/LazyLoadRunMapEntrySet.java

          Code changed in jenkins
          User: Jesse Glick
          Path:
          core/src/test/java/jenkins/model/lazy/AbstractLazyLoadRunMapTest.java
          core/src/test/java/jenkins/model/lazy/FakeMap.java
          http://jenkins-ci.org/commit/jenkins/2f58ceb1a66c8ed621fa2562c5ed3445b29ea200
          Log:
          JENKINS-18065 Uncommenting original test.
          The actual implementation does not behave as nicely as one would hope, for a few reasons:
          1. isEmpty actually tries to load the newestBuild, rather than simply checking whether there is some idOnDisk.
          Arguably this is necessary, since there could be an unloadable build record, in which case it would be technically correct for the map to be considered empty.
          2. Loading AbstractLazyLoadRunMap.newestBuild calls search(MAX_VALUE, DESC), which behaves oddly, by doing a binary search, and thus perhaps loading lg(|map|) entries.
          You would think that it would suffice to check for the last member of idOnDisk in index.byId.
          3. The iterator eagerly loads the next value before hasNext has even been called.
          Looks bad in a test, though it probably has little practical impact since most callers would be calling hasNext soon afterward anyway.
          Might cause one extra build record to be loaded unnecessarily from a limited RunList.

          SCM/JIRA link daemon added a comment - Code changed in jenkins User: Jesse Glick Path: core/src/test/java/jenkins/model/lazy/AbstractLazyLoadRunMapTest.java core/src/test/java/jenkins/model/lazy/FakeMap.java http://jenkins-ci.org/commit/jenkins/2f58ceb1a66c8ed621fa2562c5ed3445b29ea200 Log: JENKINS-18065 Uncommenting original test. The actual implementation does not behave as nicely as one would hope, for a few reasons: 1. isEmpty actually tries to load the newestBuild, rather than simply checking whether there is some idOnDisk. Arguably this is necessary, since there could be an unloadable build record, in which case it would be technically correct for the map to be considered empty. 2. Loading AbstractLazyLoadRunMap.newestBuild calls search(MAX_VALUE, DESC), which behaves oddly, by doing a binary search, and thus perhaps loading lg(|map|) entries. You would think that it would suffice to check for the last member of idOnDisk in index.byId. 3. The iterator eagerly loads the next value before hasNext has even been called. Looks bad in a test, though it probably has little practical impact since most callers would be calling hasNext soon afterward anyway. Might cause one extra build record to be loaded unnecessarily from a limited RunList.

          Jesse Glick added a comment - Deployed https://github.com/jenkinsci/jenkins/compare/jenkins-1.554.3...JENKINS-18065-ALLRM-all to: http://repo.jenkins-ci.org/public/org/jenkins-ci/main/jenkins-war/1.554.3.JENKINS-18065-ALLRM-all/jenkins-war-1.554.3.JENKINS-18065-ALLRM-all.war

          dogfood added a comment -

          Integrated in jenkins_main_trunk #3514
          JENKINS-18065 Uncommenting original test. (Revision 2f58ceb1a66c8ed621fa2562c5ed3445b29ea200)

          Result = SUCCESS
          Jesse Glick : 2f58ceb1a66c8ed621fa2562c5ed3445b29ea200
          Files :

          • core/src/test/java/jenkins/model/lazy/AbstractLazyLoadRunMapTest.java
          • core/src/test/java/jenkins/model/lazy/FakeMap.java

          dogfood added a comment - Integrated in jenkins_main_trunk #3514 JENKINS-18065 Uncommenting original test. (Revision 2f58ceb1a66c8ed621fa2562c5ed3445b29ea200) Result = SUCCESS Jesse Glick : 2f58ceb1a66c8ed621fa2562c5ed3445b29ea200 Files : core/src/test/java/jenkins/model/lazy/AbstractLazyLoadRunMapTest.java core/src/test/java/jenkins/model/lazy/FakeMap.java

          Code changed in jenkins
          User: Daniel Beck
          Path:
          changelog.html
          http://jenkins-ci.org/commit/jenkins/fdc0b5c5650b3ee849f02e3c5d94d23c12886adc
          Log:
          Noting #1314, #1316, #1308, JENKINS-17667, JENKINS-22395, JENKINS-18065

          SCM/JIRA link daemon added a comment - Code changed in jenkins User: Daniel Beck Path: changelog.html http://jenkins-ci.org/commit/jenkins/fdc0b5c5650b3ee849f02e3c5d94d23c12886adc Log: Noting #1314, #1316, #1308, JENKINS-17667 , JENKINS-22395 , JENKINS-18065

          Code changed in jenkins
          User: Daniel Beck
          Path:
          changelog.html
          http://jenkins-ci.org/commit/jenkins/72f37e414533c02624f9deffc85a2b50f03905e6
          Log:
          Remove JENKINS-22395, JENKINS-18065 from 1.572 section

          SCM/JIRA link daemon added a comment - Code changed in jenkins User: Daniel Beck Path: changelog.html http://jenkins-ci.org/commit/jenkins/72f37e414533c02624f9deffc85a2b50f03905e6 Log: Remove JENKINS-22395 , JENKINS-18065 from 1.572 section

          Code changed in jenkins
          User: Kohsuke Kawaguchi
          Path:
          core/src/main/java/hudson/util/RunList.java
          core/src/main/java/jenkins/model/lazy/AbstractLazyLoadRunMap.java
          core/src/main/java/jenkins/model/lazy/LazyLoadRunMapEntrySet.java
          core/src/test/java/jenkins/model/lazy/AbstractLazyLoadRunMapTest.java
          http://jenkins-ci.org/commit/jenkins/632d7fe0d01d492e0803495691f4397980cf44e0
          Log:
          [FIXED JENKINS-18065] made ALLRM.entrySet() smarter

          (cherry picked from commit 54c084613f83f9b7a02957eb98dbd8dc661d1c9c)

          Conflicts:
          changelog.html

          SCM/JIRA link daemon added a comment - Code changed in jenkins User: Kohsuke Kawaguchi Path: core/src/main/java/hudson/util/RunList.java core/src/main/java/jenkins/model/lazy/AbstractLazyLoadRunMap.java core/src/main/java/jenkins/model/lazy/LazyLoadRunMapEntrySet.java core/src/test/java/jenkins/model/lazy/AbstractLazyLoadRunMapTest.java http://jenkins-ci.org/commit/jenkins/632d7fe0d01d492e0803495691f4397980cf44e0 Log: [FIXED JENKINS-18065] made ALLRM.entrySet() smarter (cherry picked from commit 54c084613f83f9b7a02957eb98dbd8dc661d1c9c) Conflicts: changelog.html

          Code changed in jenkins
          User: Jesse Glick
          Path:
          core/src/main/java/jenkins/model/lazy/LazyLoadRunMapEntrySet.java
          http://jenkins-ci.org/commit/jenkins/f1b0779c6d356db8fb0de6c89bd5bc03483d6b34
          Log:
          JENKINS-18065 54c0846 amendment: presumably RunMap.entrySet().iterator().next().setValue(…) should be illegal.
          (cherry picked from commit da57ad5c30b1c9ca8365f869638abd29c8fb5896)

          SCM/JIRA link daemon added a comment - Code changed in jenkins User: Jesse Glick Path: core/src/main/java/jenkins/model/lazy/LazyLoadRunMapEntrySet.java http://jenkins-ci.org/commit/jenkins/f1b0779c6d356db8fb0de6c89bd5bc03483d6b34 Log: JENKINS-18065 54c0846 amendment: presumably RunMap.entrySet().iterator().next().setValue(…) should be illegal. (cherry picked from commit da57ad5c30b1c9ca8365f869638abd29c8fb5896)

          Code changed in jenkins
          User: Jesse Glick
          Path:
          core/src/test/java/jenkins/model/lazy/AbstractLazyLoadRunMapTest.java
          core/src/test/java/jenkins/model/lazy/FakeMap.java
          http://jenkins-ci.org/commit/jenkins/8c70a2b76875d2ca6a114c45fd54c0b1e9ecb7d2
          Log:
          JENKINS-18065 Uncommenting original test.
          The actual implementation does not behave as nicely as one would hope, for a few reasons:
          1. isEmpty actually tries to load the newestBuild, rather than simply checking whether there is some idOnDisk.
          Arguably this is necessary, since there could be an unloadable build record, in which case it would be technically correct for the map to be considered empty.
          2. Loading AbstractLazyLoadRunMap.newestBuild calls search(MAX_VALUE, DESC), which behaves oddly, by doing a binary search, and thus perhaps loading lg(|map|) entries.
          You would think that it would suffice to check for the last member of idOnDisk in index.byId.
          3. The iterator eagerly loads the next value before hasNext has even been called.
          Looks bad in a test, though it probably has little practical impact since most callers would be calling hasNext soon afterward anyway.
          Might cause one extra build record to be loaded unnecessarily from a limited RunList.

          (cherry picked from commit 2f58ceb1a66c8ed621fa2562c5ed3445b29ea200)

          SCM/JIRA link daemon added a comment - Code changed in jenkins User: Jesse Glick Path: core/src/test/java/jenkins/model/lazy/AbstractLazyLoadRunMapTest.java core/src/test/java/jenkins/model/lazy/FakeMap.java http://jenkins-ci.org/commit/jenkins/8c70a2b76875d2ca6a114c45fd54c0b1e9ecb7d2 Log: JENKINS-18065 Uncommenting original test. The actual implementation does not behave as nicely as one would hope, for a few reasons: 1. isEmpty actually tries to load the newestBuild, rather than simply checking whether there is some idOnDisk. Arguably this is necessary, since there could be an unloadable build record, in which case it would be technically correct for the map to be considered empty. 2. Loading AbstractLazyLoadRunMap.newestBuild calls search(MAX_VALUE, DESC), which behaves oddly, by doing a binary search, and thus perhaps loading lg(|map|) entries. You would think that it would suffice to check for the last member of idOnDisk in index.byId. 3. The iterator eagerly loads the next value before hasNext has even been called. Looks bad in a test, though it probably has little practical impact since most callers would be calling hasNext soon afterward anyway. Might cause one extra build record to be loaded unnecessarily from a limited RunList. (cherry picked from commit 2f58ceb1a66c8ed621fa2562c5ed3445b29ea200)

            kohsuke Kohsuke Kawaguchi
            jglick Jesse Glick
            Votes:
            0 Vote for this issue
            Watchers:
            7 Start watching this issue

              Created:
              Updated:
              Resolved: