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

JenkinsAbstractProjectListener ::onNotifyCommit() totally fails if a runtime exception happens in one item

    • Icon: Bug Bug
    • Resolution: Unresolved
    • Icon: Major Major
    • git-plugin

      JenkinsAbstractProjectListener::onNotifyCommit() checks all projects with SCM triggers sequentially. Then it calls getSCMs() of the project and processes SCMs. If the method somehow fails (e.g. NPE), the method exists => one corrupted project may cause a total outage of Git SCM triggers

      Example of the issue: JENKINS-26761 (caused by workflow-plugin)

          [JENKINS-28598] JenkinsAbstractProjectListener ::onNotifyCommit() totally fails if a runtime exception happens in one item

          Li Ke added a comment -

          oleg_nenashev Would you mind me to refactor this method? Seems like it's really big and complex..

          Li Ke added a comment - oleg_nenashev Would you mind me to refactor this method? Seems like it's really big and complex..

          Oleg Nenashev added a comment -

          liketic fine for me, but I would recommend syncing up with markewaite before it

          Oleg Nenashev added a comment - liketic fine for me, but I would recommend syncing up with markewaite before it

          Mark Waite added a comment - - edited

          liketic which method are you intending to refactor?

          If it is inside the git plugin (as in GitStatus or GitSCMSource), then your refactoring must include many tests to show current behavior.  Once you have tests that cover current behavior, then the refactoring can be attempted with the benefit of the tests to catch unexpected breaks.  With 120 000+ installations, there are very few branches in the git plugin which are not required for someone.

          If it is inside core Jenkins, others will need to decide if it needs tests first, or already has sufficient tests.  I've consistently found that there are not enough tests in the git plugin (and the git client plugin).

          I'm not especially persuaded that a null pointer exception in one of the triggers should be ignored.  That seems like we've detected undefined behavior, but will silently ignore it and continue operation.

          Mark Waite added a comment - - edited liketic which method are you intending to refactor? If it is inside the git plugin (as in GitStatus or GitSCMSource ), then your refactoring must include many tests to show current behavior.  Once you have tests that cover current behavior, then the refactoring can be attempted with the benefit of the tests to catch unexpected breaks.  With 120 000+ installations, there are very few branches in the git plugin which are not required for someone. If it is inside core Jenkins, others will need to decide if it needs tests first, or already has sufficient tests.  I've consistently found that there are not enough tests in the git plugin (and the git client plugin). I'm not especially persuaded that a null pointer exception in one of the triggers should be ignored.  That seems like we've detected undefined behavior, but will silently ignore it and continue operation.

          Li Ke added a comment -

          oleg_nenashevThanks! markewaite I agree with you. I think it's low priority to fix this issue unless we know what's the best resolution for it. I think what we can do now is add more correct tests for this plugin for changes in the future. Thanks. 

          Li Ke added a comment - oleg_nenashev Thanks! markewaite I agree with you. I think it's low priority to fix this issue unless we know what's the best resolution for it. I think what we can do now is add more correct tests for this plugin for changes in the future. Thanks. 

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

              Created:
              Updated: