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

Incorrect usage of TimeDuration

    XMLWordPrintable

    Details

    • Type: Bug
    • Status: Resolved (View Workflow)
    • Priority: Minor
    • Resolution: Fixed
    • Component/s: core
    • Labels:
    • Similar Issues:

      Description

      When Kohsuke Kawaguchi 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.

        Attachments

          Issue Links

            Activity

            Hide
            scm_issue_link 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)

            The unit was actually ignored...

            • Add tests for getTimeInSeconds
            Show
            scm_issue_link 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
            Hide
            scm_issue_link 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

            Show
            scm_issue_link 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
            Hide
            scm_issue_link 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

            Show
            scm_issue_link 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
            Hide
            danielbeck Daniel Beck added a comment -

            Fixed in 2.82.

            Show
            danielbeck Daniel Beck added a comment - Fixed in 2.82.

              People

              • Assignee:
                batmat Baptiste Mathus
                Reporter:
                jglick Jesse Glick
              • Votes:
                0 Vote for this issue
                Watchers:
                4 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved: