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


    • Icon: Bug Bug
    • Resolution: Fixed
    • Icon: Minor Minor
    • core

      Placeholder for https://github.com/jenkinsci/jenkins/pull/2928  created by jglick . 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.

            jglick Jesse Glick
            oleg_nenashev Oleg Nenashev
            0 Vote for this issue
            3 Start watching this issue