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

PipelineTriggersJobProperty#startTriggers() throws NPE when the property is not assigned to job

XMLWordPrintable

    • Icon: Bug Bug
    • Resolution: Incomplete
    • Icon: Minor Minor
    • workflow-job-plugin
    • None

      When somebody creates a property in API and occasionally triggers the startTriggers() method, Trigger#start(owner, bool) gets invoked with null argument, which violates the contract and causes NPE.

      java.lang.NullPointerException
 at hudson.triggers.Trigger.start(Trigger.java:94)
 at org.jenkinsci.plugins.workflow.job.properties.PipelineTriggersJobProperty.startTriggers(PipelineTriggersJobProperty.java:103)
 at .... (whatever code)

      I think this NPE should be prevented, because such behavior is not documented in the class Javadoc, and hence API users may expect a robust behavior.

       

      I have hit it in one of the proprietary Job templating engines, but actually it is a valid case reproducible via a simple test (see below). 

      public void triggerMethodsShouldNotThrowNPEWhenNotAssigned() {
  MockTrigger t = new MockTrigger();
  PipelineTriggersJobProperty prop = new PipelineTriggersJobProperty(Arrays.asList(t));

  prop.startTriggers(true);
}

          [JENKINS-45067] PipelineTriggersJobProperty#startTriggers() throws NPE when the property is not assigned to job

          Oleg Nenashev created issue -
          Oleg Nenashev made changes -
          Description Original: When somebody creates a property in API and occasionally triggers the startTriggers() method, Trigger#start(owner, bool) gets invoked with null argument, which violates the contract and causes NPE.
          java.lang.NullPointerException
          at hudson.triggers.Trigger.start(Trigger.java:94)
          at org.jenkinsci.plugins.workflow.job.properties.PipelineTriggersJobProperty.startTriggers(PipelineTriggersJobProperty.java:103)
                  at .... (whatever code)
          I think this NPE should be prevented, because such behavior is not documented in the class Javadoc, and hence API users may expect a robust behavior.

           

          I have hit it in one of the proprietary Job templating engines, but actually it is a valid case reproducible via a simple test (see below). 
          {code:java}
          public void triggerMethodsShouldNotThrowNPEWhenNotAssigned() {
            MockTrigger t = new MockTrigger();
            PipelineTriggersJobProperty prop = new PipelineTriggersJobProperty(Arrays.asList(t));

            prop.startTriggers(true);
          }{code}           		New: When somebody creates a property in API and occasionally triggers the startTriggers() method, Trigger#start(owner, bool) gets invoked with null argument, which violates the contract and causes NPE.
          {code:java}
          java.lang.NullPointerException
           at hudson.triggers.Trigger.start(Trigger.java:94)
           at org.jenkinsci.plugins.workflow.job.properties.PipelineTriggersJobProperty.startTriggers(PipelineTriggersJobProperty.java:103)
           at .... (whatever code){code}

           I think this NPE should be prevented, because such behavior is not documented in the class Javadoc, and hence API users may expect a robust behavior.

           

          I have hit it in one of the proprietary Job templating engines, but actually it is a valid case reproducible via a simple test (see below). 
          {code:java}
          public void triggerMethodsShouldNotThrowNPEWhenNotAssigned() {
            MockTrigger t = new MockTrigger();
            PipelineTriggersJobProperty prop = new PipelineTriggersJobProperty(Arrays.asList(t));

            prop.startTriggers(true);
          }{code}
          Oleg Nenashev made changes -
          Status Original: Open [ 1 ] New: In Progress [ 3 ]
          Oleg Nenashev made changes -
          Assignee New: Oleg Nenashev [ oleg_nenashev ]

          Jesse Glick added a comment -

          The “simple test” is illegitimate since the property must be assigned to the job if it is used.

          Jesse Glick added a comment - The “simple test” is illegitimate since the property must be assigned to the job if it is used.

          Jesse Glick added a comment -

          Possibly just a symptom of JENKINS-42446. Hard to tell without knowing how to reproduce the problem in a realistic situation.

          Jesse Glick added a comment - Possibly just a symptom of  JENKINS-42446 . Hard to tell without knowing how to reproduce the problem in a realistic situation.
          Jesse Glick made changes -
          Link New: This issue duplicates JENKINS-42446 [ JENKINS-42446 ]

          Oleg Nenashev added a comment -

          jglick

          > The “simple test” is illegitimate since the property must be assigned to the job if it is used.

          I agree, but I am not sure if it is actually documented anywhere (e.g. methods have no Javadoc). Please provide a link. If it is not documented, I assume the snippet is something possibly valid.

          Anyway, the pull request adds diagnostics and Javadoc, which explicitly notify about invalid state, and hence Jenkins admins will get notified about a bug in whatever calling logic. The proposed change is better than the original NPE, because we fail fast and prevent whatever undefined behavior in triggers.

           

           

           

          Oleg Nenashev added a comment - jglick > The “simple test” is illegitimate since the property must be assigned to the job if it is used. I agree, but I am not sure if it is actually documented anywhere (e.g. methods have no Javadoc). Please provide a link. If it is not documented, I assume the snippet is something possibly valid. Anyway, the pull request adds diagnostics and Javadoc, which explicitly notify about invalid state, and hence Jenkins admins will get notified about a bug in whatever calling logic. The proposed change is better than the original NPE, because we fail fast and prevent whatever undefined behavior in triggers.      
          Jesse Glick made changes -
          Remote Link New: This issue links to "PR 58 (Web Link)" [ 17213 ]
          Jesse Glick made changes -
          Link New: This issue relates to JENKINS-38454 [ JENKINS-38454 ]

            oleg_nenashev Oleg Nenashev
            oleg_nenashev Oleg Nenashev
            Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

              Created:
              Updated:
              Resolved: