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

transient file handle leak in LargeText.GzipAwareSession.isGzipStream(File)

      One of the "reproduction" scenarios if JENKINS-45057 identified a separate transient file handle leak in LargeText.GzipAwareSession.isGzipStream(File)

          [JENKINS-45903] transient file handle leak in LargeText.GzipAwareSession.isGzipStream(File)

          Reproduction test:

          Run freestyle job with system groovy step:

          import hudson.model.*
          
          def thr = Thread.currentThread()
          def build = thr?.executable
          def jobName = build.parent.builds[0].properties.get("envVars").get("JOB_NAME")
          def jobNr = build.parent.builds[0].properties.get("envVars").get("BUILD_NUMBER")
          println "This is " + jobName + " running for the $jobNr:th time"
          

          Now the above script is not a correct script, but use of Groovy's magic "proprerties" property exposes the file leak when instantiating the Run.getLogText() "property", you get file handle leaks like:

          > java 19008 jenkins 587r REG 252,0 503 395865 /data/jenkins/jobs/automation/jobs/test-open-files/builds/7/log
          > java 19008 jenkins 589r REG 252,0 503 395865 /data/jenkins/jobs/automation/jobs/test-open-files/builds/7/log
          > java 19008 jenkins 590r REG 252,0 503 395865 /data/jenkins/jobs/automation/jobs/test-open-files/builds/7/log
          > java 19008 jenkins 592r REG 252,0 503 395865 /data/jenkins/jobs/automation/jobs/test-open-files/builds/7/log
          

          The differentiator from JENKINS-45057 is that these are READ locks not WRITE locks.

          Stephen Connolly added a comment - Reproduction test: Run freestyle job with system groovy step: import hudson.model.* def thr = Thread .currentThread() def build = thr?.executable def jobName = build.parent.builds[0].properties.get( "envVars" ).get( "JOB_NAME" ) def jobNr = build.parent.builds[0].properties.get( "envVars" ).get( "BUILD_NUMBER" ) println "This is " + jobName + " running for the $jobNr:th time" Now the above script is not a correct script, but use of Groovy's magic "proprerties" property exposes the file leak when instantiating the Run.getLogText() "property", you get file handle leaks like: > java 19008 jenkins 587r REG 252,0 503 395865 /data/jenkins/jobs/automation/jobs/test-open-files/builds/7/log > java 19008 jenkins 589r REG 252,0 503 395865 /data/jenkins/jobs/automation/jobs/test-open-files/builds/7/log > java 19008 jenkins 590r REG 252,0 503 395865 /data/jenkins/jobs/automation/jobs/test-open-files/builds/7/log > java 19008 jenkins 592r REG 252,0 503 395865 /data/jenkins/jobs/automation/jobs/test-open-files/builds/7/log The differentiator from JENKINS-45057 is that these are READ locks not WRITE locks.

          Joschua Grube added a comment - - edited

          We solved this by using this approach instead:

          def thr = Thread.currentThread()
          def build = thr?.executable
          def resolver = build.buildVariableResolver
          def envValue = resolver.resolve(envName)

          Checkout: https://wiki.jenkins.io/display/JENKINS/Parameterized+System+Groovy+script

          Note: This just works for system groovy scripts.

          Joschua Grube added a comment - - edited We solved this by using this approach instead: def thr = Thread .currentThread() def build = thr?.executable def resolver = build.buildVariableResolver def envValue = resolver.resolve(envName) Checkout:  https://wiki.jenkins.io/display/JENKINS/Parameterized+System+Groovy+script Note: This just works for system groovy scripts.

          joschua_grube yes there are many ways to fix the groovy script, the groovy script is just cited as a reproducer...

          Stephen Connolly added a comment - joschua_grube yes there are many ways to fix the groovy script, the groovy script is just cited as a reproducer...

          Oleg Nenashev added a comment -

          The fix has been integrated towards Jenkins 2.74. Marking it as LTS Candidate, but I am not sure we want to backport Stapler

          Oleg Nenashev added a comment - The fix has been integrated towards Jenkins 2.74. Marking it as LTS Candidate, but I am not sure we want to backport Stapler

          Jesse Glick added a comment -

          Backporting just this fix to a special Stapler release for 2.70.x LTS is an option, but first I would like to understand this issue better.

          First, the reproduction cases: they all claim to involve broken Groovy scripts which accidentally call Run.getLogText and thus use LargeText.GzipAwareSession.isGzipStream. But would the same thing not happen when you just access a build log any other way, say using a /jobs///console link?

          Second, the cause. isGzipStream did close its file handle already. As JDK-8080225 notes (cf. JENKINS-42934), this is not an issue of a file handle leak, just of an unnecessary burden on the GC system in the JRE. But the reported symptom is of file handle leaks. AFAICT the only code in LargeText (or AnnotatedLargeText) which could cause a leak if used improperly is the Session which must have its close method called. Since the session is private, and writeLogTo does so itself (albeit without using a finally block as it should), that leaves readAll, whose caller must close the Reader. But then who is calling readAll? Just calling getLogText does not.

          Until the mechanism of the bug, and the effectiveness of the fix, is explained, I would not want to risk a backport.

          Jesse Glick added a comment - Backporting just this fix to a special Stapler release for 2.70.x LTS is an option, but first I would like to understand this issue better. First, the reproduction cases: they all claim to involve broken Groovy scripts which accidentally call Run.getLogText and thus use LargeText.GzipAwareSession.isGzipStream . But would the same thing not happen when you just access a build log any other way, say using a /jobs/ / /console link? Second, the cause. isGzipStream   did  close its file handle already. As JDK-8080225 notes (cf. JENKINS-42934 ), this is not an issue of a file handle leak, just of an unnecessary burden on the GC system in the JRE. But the reported symptom is of file handle leaks. AFAICT the only code in LargeText (or AnnotatedLargeText ) which could cause a leak if used improperly is the Session which must have its close method called. Since the session is private , and writeLogTo does so itself (albeit without using a finally block as it should), that leaves readAll , whose caller must close  the Reader . But then who is calling readAll ? Just calling getLogText does not. Until the mechanism of the bug, and the effectiveness of the fix, is explained, I would not want to risk a backport.

          Mike Delaney added a comment -

          For us afflicted by this, is there a way to mitigate the issue?

          Mike Delaney added a comment - For us afflicted by this, is there a way to mitigate the issue?

          mdelaney, it is fixed in 2.74 weekly release.

          Oliver Gondža added a comment - mdelaney , it is fixed in 2.74 weekly release.

          Mike Delaney added a comment -

          olivergondza, awesome. Thanks for the confirmation.

          Mike Delaney added a comment - olivergondza , awesome. Thanks for the confirmation.

          Jesse Glick added a comment -

          As written above, I am not convinced that what was fixed is what people think they are reporting.

          Jesse Glick added a comment - As written above, I am not convinced that what was fixed is what people think they are reporting.

          Hugues Moreau added a comment - - edited

          As Jesse I think this is not fixed, I still reproduce this on 2.78 with a 1-line System Groovy Script:

          println build.properties.environment.SOME_GLOBAL_PROP

          This leaks 2 file descriptors on that build's log file.

          To make sure that it's not something else being wrong on my setup, I have also verified that println "Howdy" leaks nothing.

          (I thought this approach was OK to obtain Global Properties from a System Groovy Script, but if that's the wrong way I'm interested to hear about alternatives that do not leak 2 file descriptors at each execution – build.buildVariableResolver does not seem to resolve Global Variables – sorry if that's only tangentially on-topic)

          Hugues Moreau added a comment - - edited As Jesse I think this is not fixed, I still reproduce this on 2.78 with a 1-line System Groovy Script: println build.properties.environment.SOME_GLOBAL_PROP This leaks 2 file descriptors on that build's log file. To make sure that it's not something else being wrong on my setup, I have also verified that  println "Howdy" leaks nothing. (I thought this approach was OK to obtain Global Properties from a System Groovy Script, but if that's the wrong way I'm interested to hear about alternatives that do not leak 2 file descriptors at each execution – build.buildVariableResolver does not seem to resolve Global Variables – sorry if that's only tangentially on-topic)

          Daniel Beck added a comment -

          alternatives that do not leak 2 file descriptors at each execution

          println build.environment.SOME_VAR

          Imagine getProperties as returning a Map of all getters/fields of the object. Unless you're doing rather unusual things with Groovy (like trying to inspect a variable) there's not really a reason to getProperties.

          http://docs.groovy-lang.org/latest/html/api/org/codehaus/groovy/runtime/DefaultGroovyMethods.html#getProperties-java.lang.Object-

          So it's redundant in most use cases.

          Note that getEnvironment() (which is what environment translates to) has been deprecated ~8 years ago. Very little of what you could run in System Groovy would be considered "user" API, so if you're not using the core Javadoc, better don't do it at all.

          Daniel Beck added a comment - alternatives that do not leak 2 file descriptors at each execution println build.environment.SOME_VAR Imagine getProperties as returning a  Map of all getters/fields of the object. Unless you're doing rather unusual things with Groovy (like trying to inspect a variable) there's not really a reason to getProperties . http://docs.groovy-lang.org/latest/html/api/org/codehaus/groovy/runtime/DefaultGroovyMethods.html#getProperties-java.lang.Object- So it's redundant in most use cases. Note that getEnvironment() (which is what environment translates to) has been deprecated ~8 years ago. Very little of what you could run in System Groovy would be considered "user" API, so if you're not using the core Javadoc, better don't do it at all.

          Hugues Moreau added a comment -

          Ouch. Yes. Thanks a lot. I have long forgotten where I found this build.properties.environment.FOO construct, I should have thought of Groovy's default methods, and should have checked the javadoc.

          Anyway, this means my earlier comment is wrong about the bug not being fixed. Feel free to delete if you want to reduce noise in JIRA (or let me know if you want me to). Perhaps more importantly, you might want to decrement the counter for "notable issues" for 2.78 in the changelog page, as I also regrettably did that.

          Hugues Moreau added a comment - Ouch. Yes. Thanks a lot. I have long forgotten where I found this build.properties.environment.FOO construct, I should have thought of Groovy's default methods, and should have checked the javadoc. Anyway, this means my earlier comment is wrong about the bug not being fixed. Feel free to delete if you want to reduce noise in JIRA (or let me know if you want me to). Perhaps more importantly, you might want to decrement the counter for "notable issues" for 2.78 in the changelog  page, as I also regrettably did that.

          Daniel Beck added a comment -

          this means my earlier comment is wrong about the bug not being fixed

          Why? 2.78 has Stapler 1.252, that is supposed to have a fix for this issue (see first comment by stephenconnolly). If it happens with properties, that seems to indicate this (or a similar) bug is still present, it just has a simple workaround.

          It seems that jglick thinks the change in Stapler did not actually change anything wrt the issue observed here, which indicate this should be reopened.

          FWIW my reproduction case on 2.78 with Groovy Plugin 2.0 (non-sandboxed):

          println build.properties.keySet()

          Daniel Beck added a comment - this means my earlier comment is wrong about the bug not being fixed Why? 2.78 has Stapler 1.252, that is supposed to have a fix for this issue (see first comment by stephenconnolly ). If it happens with properties , that seems to indicate this (or a similar) bug is still present, it just has a simple workaround. It seems that jglick thinks the change in Stapler did not actually change anything wrt the issue observed here, which indicate this should be reopened. FWIW my reproduction case on 2.78 with Groovy Plugin 2.0 (non-sandboxed): println build.properties.keySet()

          Hugues Moreau added a comment -

          OK maybe not wrong, but I did not bring conclusive evidence. Yes it produces a leak of 2 FDs, and I initially thought "oh that's the same bug, I'll chime in with my repro, maybe that'll help". Now I don't know if that's the same issue. Maybe this is a different leak. Maybe it's the same. I simply don't know: given the nature of "getProperties()" method, using build.properties looks like a Bad Idea because of the nasty/uncontrollable side effects. And for a moment I thought this made my repro invalid & worthless. But if you know enough to think it's the same, good.

          I would not be too shocked, now that I understand what it does, if the official stance was "not supported, don't do that". But if you want to make the build "getProperties()-proof", that's great

          Hugues Moreau added a comment - OK maybe not wrong, but I did not bring conclusive evidence. Yes it produces a leak of 2 FDs, and I initially thought " oh that's the same bug, I'll chime in with my repro, maybe that'll help ". Now I don't know if that's the same issue. Maybe this is a different leak. Maybe it's the same. I simply don't know: given the nature of "getProperties()" method, using build.properties  looks like a Bad Idea because of the nasty/uncontrollable side effects. And for a moment I thought this made my repro invalid & worthless. But if you know enough to think it's the same, good. I would not be too shocked, now that I understand what it does, if the official stance was " not supported, don't do that ". But if you want to make the build "getProperties()-proof", that's great

          danielbeck this issue is just for the LargeText.GzipAwareSession.isGzipStream(File) leak, there are other leaks possible through the Groovy getProperties() method, but they would likely have shared the same file handle, e.g. getLogInputStream(), getLogReader(), etc. so they would not have shown up as separate from LargeText.GzipAwareSession.isGzipStream(File) which was the driver.

          Basically DO NOT USE build.properties as it will trigger multiple file-handle leaks until a full GC is forced.

          The LargeText.GzipAwareSession.isGzipStream(File) had other side-effects (such as causing a leak when browsing via the UI) which is why this was important to fix independently of the advice not to use build.properties

          HTH

          Stephen Connolly added a comment - danielbeck this issue is just for the LargeText.GzipAwareSession.isGzipStream(File) leak, there are other leaks possible through the Groovy getProperties() method, but they would likely have shared the same file handle, e.g. getLogInputStream() , getLogReader() , etc. so they would not have shown up as separate from LargeText.GzipAwareSession.isGzipStream(File) which was the driver. Basically DO NOT USE build.properties as it will trigger multiple file-handle leaks until a full GC is forced. The LargeText.GzipAwareSession.isGzipStream(File) had other side-effects (such as causing a leak when browsing via the UI) which is why this was important to fix independently of the advice not to use build.properties HTH

          I am removing this from LTS candidates for 2.73 as stapler version bump is required that would, among other things, introduce new API: https://github.com/stapler/stapler/pull/106.

          Oliver Gondža added a comment - I am removing this from LTS candidates for 2.73 as stapler version bump is required that would, among other things, introduce new API: https://github.com/stapler/stapler/pull/106 .

          Daniel Beck added a comment -

          olivergondza How about a Stapler 1.250.1, if feasible?

          Daniel Beck added a comment - olivergondza How about a Stapler 1.250.1, if feasible?

          danielbeck, I do not oppose if this is serious enough.

          Oliver Gondža added a comment - danielbeck , I do not oppose if this is serious enough.

          Daniel Beck added a comment -

          Restoring lts-candidate then. Popularity indicates this is relevant enough for a backport. (Even if it's unclear everyone watching would be helped by this fix.)

          oleg_nenashev olivergondza Do we need KK to release Stapler, or could someone else do this?

          Daniel Beck added a comment - Restoring  lts-candidate then. Popularity indicates this is relevant enough for a backport. (Even if it's unclear everyone watching would be helped by this fix.) oleg_nenashev olivergondza Do we need KK to release Stapler, or could someone else do this?

          Jesse Glick added a comment -

          I at least am able to release Stapler if this is under consideration for 2.73.3.

          Jesse Glick added a comment - I at least am able to release Stapler if this is under consideration for 2.73.3.

          Daniel Beck added a comment -

          olivergondza See above – we can add it if you think the backport to Stapler 1.250 is reasonable for 2.73.3 inclusion.

          Daniel Beck added a comment - olivergondza See above – we can add it if you think the backport to Stapler 1.250 is reasonable for 2.73.3 inclusion.

          Hubert Okon added a comment -

          Instead of using build.properties (which leaks FD) it is possible to use build..getActions() which does not leak FD.

          Hubert Okon added a comment - Instead of using  build.properties  (which leaks FD) it is possible to use  build. .getActions()  which does not leak FD.

            stephenconnolly Stephen Connolly
            stephenconnolly Stephen Connolly
            Votes:
            2 Vote for this issue
            Watchers:
            9 Start watching this issue

              Created:
              Updated:
              Resolved: