Uploaded image for project: 'Jenkins'
  1. Jenkins
  2. JENKINS-45737

User mapping should be stored in a per-Jenkins field and getAll should be called just once per session unless we reload

    XMLWordPrintable

    Details

    • Similar Issues:

      Description

      Placeholder for https://github.com/jenkinsci/jenkins/pull/2928  created by Jesse Glick . Description from him:

      While looking into jenkinsci/git-plugin#498 I realized that the main problem in that thread dump was not that User.get was being called, per se, as that User.getAll was doing disk I/O. In general it needs to do disk I/O once per session, but there is no good reason to throw away the results until either Jenkins is restarted or an explicit configuration reload is requested. Rechecking disk every 10s just seems wasteful.

      (In principle we could try to optimize away ever loading of stored configurations but I suspect {{getAll}}calls are more or less unavoidable in practice, so it seems simpler to just move it into the startup sequence.)

      While there, I noticed that the storage of User.byName was quite wrong. Generally, no state should be kept in a static field other than that reachable from Jenkins.theInstance.

      I fixed this, then encountered User.reload, which was more or less legitimately being called from Jenkins.reload—really it ought to be automatic, but Jenkins.reload does not seem to rerun initializers reliably (unclear to me how this is intended to work)—yet was also being called from a bunch of functional tests with hazy intent, especially those coming from #831. Probably the idea was to simulate what would happen if Jenkins were restarted, but the test code held on to User objects across what would have been restarts, which does not correspond to any actual sequence of steps a Jenkins user could take; I fixed all such tests (whose purpose I could divine) to call Jenkins.reload}}and not carry over any objects between “sessions”. In general {{RestartableJenkinsRule is the preferred way to test such things, unless you are actually attempting to simulate Reload Configuration from Disk.

      And then User.clear appeared, which is not ever called in Jenkins production code. It has historically been called by JenkinsRule, apparently to work around the “stickiness” of the static byName field, which is no longer an issue; and, again, from some tests which seem to have been using it in lieu of actually performing a restart.

        Attachments

          Issue Links

            Activity

            Hide
            scm_issue_link SCM/JIRA link daemon added a comment -

            Code changed in jenkins
            User: Jesse Glick
            Path:
            core/src/main/java/hudson/init/InitMilestone.java
            core/src/main/java/hudson/model/User.java
            test/src/test/java/hudson/cli/ReloadConfigurationCommandTest.java
            test/src/test/java/hudson/model/MyViewsPropertyTest.java
            test/src/test/java/hudson/model/UserRestartTest.java
            test/src/test/java/hudson/model/UserTest.java
            test/src/test/java/jenkins/model/JenkinsReloadConfigurationTest.java
            http://jenkins-ci.org/commit/jenkins/f4d5c764212df6e287e4db69c6c5b9e65207d4ad
            Log:
            JENKINS-45737 - User mapping should be stored in a per-Jenkins field and getAll should be called just once per session unless we reload (#2928)

            JENKINS-45737 - User mapping should be stored in a per-Jenkins field and getAll should be called just once per session unless we reload

            Show
            scm_issue_link SCM/JIRA link daemon added a comment - Code changed in jenkins User: Jesse Glick Path: core/src/main/java/hudson/init/InitMilestone.java core/src/main/java/hudson/model/User.java test/src/test/java/hudson/cli/ReloadConfigurationCommandTest.java test/src/test/java/hudson/model/MyViewsPropertyTest.java test/src/test/java/hudson/model/UserRestartTest.java test/src/test/java/hudson/model/UserTest.java test/src/test/java/jenkins/model/JenkinsReloadConfigurationTest.java http://jenkins-ci.org/commit/jenkins/f4d5c764212df6e287e4db69c6c5b9e65207d4ad Log: JENKINS-45737 - User mapping should be stored in a per-Jenkins field and getAll should be called just once per session unless we reload (#2928) JENKINS-45737 - User mapping should be stored in a per-Jenkins field and getAll should be called just once per session unless we reload
            Hide
            oleg_nenashev Oleg Nenashev added a comment -

            It has been released in Jenkins 2.71

            Show
            oleg_nenashev Oleg Nenashev added a comment - It has been released in Jenkins 2.71
            Hide
            jglick Jesse Glick added a comment -

            I do not think this is a good lts-candidate.

            Show
            jglick Jesse Glick added a comment - I do not think this is a good lts-candidate .

              People

              • Assignee:
                jglick Jesse Glick
                Reporter:
                oleg_nenashev Oleg Nenashev
              • Votes:
                0 Vote for this issue
                Watchers:
                3 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved: