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

      When kohsuke introduced TimeDuration in 2012, there were mistakes in the handling of time units. The naming of the internal field millis implies that it is measured in milliseconds, as does the design of the getTimeInMillis and as methods. Yet fromString treats 3sec as new TimeDuration(3), treating the field as measured in seconds.

      Why did we not notice wildly incorrect scheduling times? Because the callers of this utility, currently ParametersDefinitionProperty and ParameterizedJobMixIn, also use it incorrectly. If the query parameter is set, they ask it for getTime (supposedly in milliseconds), yet pass it to scheduling methods which expect to be measured in seconds (as do quietPeriod fields). If it is not set, they construct it from quiet periods, passing in a value measured in seconds.

      The parsing code was also intended to support other units, but never did.

      The effect of all this mess is that .../build, .../build?delay=3sec, and .../build?delay=3 all behave as you expect, so there is not really a user-visible bug, but the code is very confusing.

      Since TimeDuration is not Serializable, there is no stored-settings issue here, so it would suffice to change <init>(long) to * 1000 for compatibility but deprecate it in favor of a constructor taking an explicit unit; change getTime to / 1000 and deprecate in favor of as; and change all call sites to use nondeprecated forms, passing in TimeUnit.SECONDS.

          [JENKINS-44052] Incorrect usage of TimeDuration

          Code changed in jenkins
          User: Baptiste Mathus
          Path:
          core/src/main/java/hudson/model/ParametersDefinitionProperty.java
          core/src/main/java/jenkins/model/ParameterizedJobMixIn.java
          core/src/main/java/jenkins/util/TimeDuration.java
          core/src/test/java/jenkins/util/TimeDurationTest.java
          http://jenkins-ci.org/commit/jenkins/ff36adf0d243e2c3461045615ee654eb33665acb
          Log:
          JENKINS-44052 Document & fix intended behaviour (#3043)

          The unit was actually ignored...

          • Add tests for getTimeInSeconds

          SCM/JIRA link daemon added a comment - Code changed in jenkins User: Baptiste Mathus Path: core/src/main/java/hudson/model/ParametersDefinitionProperty.java core/src/main/java/jenkins/model/ParameterizedJobMixIn.java core/src/main/java/jenkins/util/TimeDuration.java core/src/test/java/jenkins/util/TimeDurationTest.java http://jenkins-ci.org/commit/jenkins/ff36adf0d243e2c3461045615ee654eb33665acb Log: JENKINS-44052 Document & fix intended behaviour (#3043) JENKINS-44052 Document & fix intended behaviour The unit was actually ignored... Add tests for getTimeInSeconds

          Code changed in jenkins
          User: Baptiste Mathus
          Path:
          core/src/test/java/jenkins/util/TimeDurationTest.java
          http://jenkins-ci.org/commit/jenkins/61016e1a841c6ec39828e5fb7cdcd83dd426fcb7
          Log:
          Add @Issue to reference JENKINS-44052

          SCM/JIRA link daemon added a comment - Code changed in jenkins User: Baptiste Mathus Path: core/src/test/java/jenkins/util/TimeDurationTest.java http://jenkins-ci.org/commit/jenkins/61016e1a841c6ec39828e5fb7cdcd83dd426fcb7 Log: Add @Issue to reference JENKINS-44052

          Code changed in jenkins
          User: Baptiste Mathus
          Path:
          core/src/main/java/jenkins/util/TimeDuration.java
          core/src/test/java/jenkins/util/TimeDurationTest.java
          http://jenkins-ci.org/commit/jenkins/be7ac438e013e0ff31c032f33f3ed6929377ea0e
          Log:
          Merge pull request #3058 from batmat/JENKINS-44052-followup

          JENKINS-44052 Small followups on #3043

          Compare: https://github.com/jenkinsci/jenkins/compare/ff36adf0d243...be7ac438e013

          SCM/JIRA link daemon added a comment - Code changed in jenkins User: Baptiste Mathus Path: core/src/main/java/jenkins/util/TimeDuration.java core/src/test/java/jenkins/util/TimeDurationTest.java http://jenkins-ci.org/commit/jenkins/be7ac438e013e0ff31c032f33f3ed6929377ea0e Log: Merge pull request #3058 from batmat/ JENKINS-44052 -followup JENKINS-44052 Small followups on #3043 Compare: https://github.com/jenkinsci/jenkins/compare/ff36adf0d243...be7ac438e013

          Daniel Beck added a comment -

          Fixed in 2.82.

          Daniel Beck added a comment - Fixed in 2.82.

            batmat Baptiste Mathus
            jglick Jesse Glick
            Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

              Created:
              Updated:
              Resolved: