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

Accessing unset global variable cause master to hang

    XMLWordPrintable

    Details

    • Similar Issues:

      Description

      If I have a global lib file vars/config.groovy like this:

      def getFoo()  { return this.foo }
      
      def setFoo(String value) {
          if (this.foo == null) {
              this.foo = value
          } else {
              logImmutableWarning('foo', this.foo, value)
          }
      }
      

      and I call

      echo config.foo
      

      my Jenkins master hangs with 100% CPU.

      If I call

      config.foo = 'bar'
      echo config.foo
      

      it works fine.

      Note that it's impossible to kill the job properly and that CPU is still 100% and a thread is still available even after klicking links in build log.

      Aborted by N N
      Click here to forcibly terminate running steps
      Click here to forcibly kill entire build
      

      Only way I know to fix this is to restart master.

      I do not think the extra setter logic is responsible for this.

      I updated this issue because the problem was bigger than I understood at first.

      Original issue

      Reading global variable in own file cause stack overflow

      In have a file in the pipeline global functions (vars/) that has properties. I want to have a default value. But this implementation:

      def getSomething() { return this.something ?: 'somethingDefaultValue' }
      

      leads to Jenkins master taking 100% CPU and hanging in what I believe is a stack overflow error. But it's very hard to get any info from logs or thread dumps.

      Trying to fix this followng the standard groovy way and adding:

      String something
      

      does not help.

        Attachments

          Issue Links

            Activity

            Hide
            marcus_phi Marcus Philip added a comment -

            I think this is same or very similar as JENKINS-34416

            Show
            marcus_phi Marcus Philip added a comment - I think this is same or very similar as JENKINS-34416
            Hide
            jglick Jesse Glick added a comment -

            Try renaming your (implicit) field to be anything other than the name of the global variable.

            Show
            jglick Jesse Glick added a comment - Try renaming your (implicit) field to be anything other than the name of the global variable.
            Hide
            marcus_phi Marcus Philip added a comment -
            • So you confirm that this is current and intended behavior?
            • Have you reproduced this?

            I think this is insane, especially considering the grave consequence (hanging Jenkins master).

            If this is indded the case, I feel that there is strong lack of documentation here: https://github.com/jenkinsci/workflow-cps-global-lib-plugin/blob/master/README.md. It should state:

            Docs addition
            • If you access an unset global variable your Jenkins master will hang indefinitely and need to be restarted
            • This also means you cannot have the name on the implicit field be the same as the name of the global variable if you want to access it in getter, e.g. to have some checks/default value handling:
              def getSomething() { return this.something ?: 'somethingDefaultValue' }
              

              And from JENKINS-34416:

            • The global variables singletons are not accessible inside shared code (workflowLibs/src/...) unless you explicitly give it as argument. They will see another instance.
            • Basically, you should think twice before using this pattern.

            PS. I updated the title since I'm not actually sure it's a stack overflow error.

            Show
            marcus_phi Marcus Philip added a comment - So you confirm that this is current and intended behavior? Have you reproduced this? I think this is insane, especially considering the grave consequence (hanging Jenkins master). If this is indded the case, I feel that there is strong lack of documentation here: https://github.com/jenkinsci/workflow-cps-global-lib-plugin/blob/master/README.md . It should state: Docs addition If you access an unset global variable your Jenkins master will hang indefinitely and need to be restarted This also means you cannot have the name on the implicit field be the same as the name of the global variable if you want to access it in getter, e.g. to have some checks/default value handling: def getSomething() { return this .something ?: 'somethingDefaultValue' } And from JENKINS-34416 : The global variables singletons are not accessible inside shared code (workflowLibs/src/...) unless you explicitly give it as argument. They will see another instance. Basically, you should think twice before using this pattern. PS. I updated the title since I'm not actually sure it's a stack overflow error.
            Hide
            marcus_phi Marcus Philip added a comment -

            Even more strange behavior found...

            These two calls are not identical, the second does not call the setter as one would expected

            globalConfig.setFoo('bar')
            globalConfig.foo = 'bar'
            

            So it does not behave like groovy properties: http://groovy-lang.org/objectorientation.html#properties . And indeed globalConfig.properties.keySet() does not contain these so called 'properties'.

            This means that code like this could behave 'interesting' if you do something more than just set and return field in getter and setter:

            globalConfig.setFoo('bar')
            globalConfig.foo('beep')
            echo globalConfig.scmFoo
            
            Show
            marcus_phi Marcus Philip added a comment - Even more strange behavior found... These two calls are not identical, the second does not call the setter as one would expected globalConfig.setFoo( 'bar' ) globalConfig.foo = 'bar' So it does not behave like groovy properties: http://groovy-lang.org/objectorientation.html#properties . And indeed globalConfig.properties.keySet() does not contain these so called 'properties'. This means that code like this could behave 'interesting' if you do something more than just set and return field in getter and setter: globalConfig.setFoo( 'bar' ) globalConfig.foo( 'beep' ) echo globalConfig.scmFoo
            Hide
            jglick Jesse Glick added a comment -

            you confirm that this is current and intended behavior?

            No, I was just offering a suggestion to fix your issue. Probably there is a StackOverflowError that some code TBD could defend against.

            Have you reproduced this?

            I have not yet tried.

            Even more strange behavior found

            Offhand this looks unrelated, please keep it to a separate issue (with steps to reproduce from scratch as usual).

            Show
            jglick Jesse Glick added a comment - you confirm that this is current and intended behavior? No, I was just offering a suggestion to fix your issue. Probably there is a StackOverflowError that some code TBD could defend against. Have you reproduced this? I have not yet tried. Even more strange behavior found Offhand this looks unrelated, please keep it to a separate issue (with steps to reproduce from scratch as usual).
            Hide
            jglick Jesse Glick added a comment -

            I believe this is simply a duplicate of JENKINS-31484 but I want to do some more integration testing first to be sure, and also want to verify that JENKINS-25623 allows the build to be halted even without this particular fix.

            Show
            jglick Jesse Glick added a comment - I believe this is simply a duplicate of JENKINS-31484 but I want to do some more integration testing first to be sure, and also want to verify that JENKINS-25623 allows the build to be halted even without this particular fix.
            Hide
            jglick Jesse Glick added a comment -

            the second does not call the setter as one would [have] expected

            Reproduced but struggling to track down the cause, since it works correctly in a unit test in groovy-cps. Neither switching to Jenkins 2 and thus Groovy 2, nor turning off the sandbox, fixes this.

            BTW you get a stack overflow even on the command line if you neglect to declare String foo as a field.

            Show
            jglick Jesse Glick added a comment - the second does not call the setter as one would [have] expected Reproduced but struggling to track down the cause, since it works correctly in a unit test in groovy-cps . Neither switching to Jenkins 2 and thus Groovy 2, nor turning off the sandbox, fixes this. BTW you get a stack overflow even on the command line if you neglect to declare String foo as a field.
            Hide
            jglick Jesse Glick added a comment -

            In other words, while I can easily reproduce JENKINS-31484 generally and have prepared a fix for it, I cannot reproduce the issue you originally reported here, because the getter does not get called at all.

            Show
            jglick Jesse Glick added a comment - In other words, while I can easily reproduce JENKINS-31484 generally and have prepared a fix for it, I cannot reproduce the issue you originally reported here, because the getter does not get called at all.
            Hide
            jglick Jesse Glick added a comment -

            Oh, and when I explicitly call the getter and setter, I get a MissingFieldException but no hang, even without using the JENKINS-31484 fix.

            I think the problems stem from the distinction between a Java-style class, as in src, vs. an implicit Script subtype as is typical for vars. If you wrap in

            class config implements Serializable {…}
            

            then things work the way you would expect, including a stack overflow prior to the JENKINS-31484 fix. I will treat this as a documentation issue.

            The @groovy.transform.Field annotation does not seem to work, so better not use it.

            Show
            jglick Jesse Glick added a comment - Oh, and when I explicitly call the getter and setter, I get a MissingFieldException but no hang, even without using the JENKINS-31484 fix. I think the problems stem from the distinction between a Java-style class, as in src , vs. an implicit Script subtype as is typical for vars . If you wrap in class config implements Serializable {…} then things work the way you would expect, including a stack overflow prior to the JENKINS-31484 fix. I will treat this as a documentation issue. The @groovy.transform.Field annotation does not seem to work, so better not use it.
            Hide
            scm_issue_link SCM/JIRA link daemon added a comment -

            Code changed in jenkins
            User: R. Tyler Croy
            Path:
            content/doc/book/pipeline/shared-libraries.adoc
            http://jenkins-ci.org/commit/jenkins.io/d3664130c51f25dd37ebdf8497b8aeedef53e72a
            Log:
            Merge pull request #565 from jglick/global-lib-tips

            JENKINS-38021 More tips about global variables

            Compare: https://github.com/jenkins-infra/jenkins.io/compare/4a2fbc6f144d...d3664130c51f

            Show
            scm_issue_link SCM/JIRA link daemon added a comment - Code changed in jenkins User: R. Tyler Croy Path: content/doc/book/pipeline/shared-libraries.adoc http://jenkins-ci.org/commit/jenkins.io/d3664130c51f25dd37ebdf8497b8aeedef53e72a Log: Merge pull request #565 from jglick/global-lib-tips JENKINS-38021 More tips about global variables Compare: https://github.com/jenkins-infra/jenkins.io/compare/4a2fbc6f144d...d3664130c51f
            Hide
            jglick Jesse Glick added a comment -

            Under some conditions I get a hang, corrected by the JENKINS-31484 fix. The build is interruptible however.

            Show
            jglick Jesse Glick added a comment - Under some conditions I get a hang, corrected by the JENKINS-31484 fix. The build is interruptible however.
            Hide
            leedega Kevin Phillips added a comment - - edited

            Just to clarify, in an earlier comment it was noted that the setter for a field does not get called when accessing the field directly (ie: myclass.myfield = 10). Is this something being investigated / fixed as part of this particular defect or should a new one be created for this specific aspect of the problem?

            Show
            leedega Kevin Phillips added a comment - - edited Just to clarify, in an earlier comment it was noted that the setter for a field does not get called when accessing the field directly (ie: myclass.myfield = 10). Is this something being investigated / fixed as part of this particular defect or should a new one be created for this specific aspect of the problem?
            Hide
            jglick Jesse Glick added a comment -

            I am not aware of any such defect. If you can reproduce a divergence in this respect from command-line Groovy in a minimal, self-contained test case, please file a separate bug report.

            Show
            jglick Jesse Glick added a comment - I am not aware of any such defect. If you can reproduce a divergence in this respect from command-line Groovy in a minimal, self-contained test case, please file a separate bug report.

              People

              Assignee:
              jglick Jesse Glick
              Reporter:
              marcus_phi Marcus Philip
              Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved: