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

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

    Details

    • Similar Issues:

      Description

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

        Attachments

          Issue Links

            Activity

            stephenconnolly Stephen Connolly created issue -
            Hide
            stephenconnolly 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.

            Show
            stephenconnolly 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.
            stephenconnolly Stephen Connolly made changes -
            Field Original Value New Value
            Remote Link This issue links to "stapler#124 (Web Link)" [ 17380 ]
            stephenconnolly Stephen Connolly made changes -
            Link This issue is related to JENKINS-45057 [ JENKINS-45057 ]
            stephenconnolly Stephen Connolly made changes -
            Link This issue is related to JENKINS-42934 [ JENKINS-42934 ]
            stephenconnolly Stephen Connolly made changes -
            Assignee Stephen Connolly [ stephenconnolly ]
            Hide
            joschua_grube 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.

            Show
            joschua_grube 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.
            Hide
            stephenconnolly 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...

            Show
            stephenconnolly 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 Oleg Nenashev made changes -
            Labels stapler
            Hide
            oleg_nenashev 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

            Show
            oleg_nenashev 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 Oleg Nenashev made changes -
            Status Open [ 1 ] Resolved [ 5 ]
            Resolution Fixed [ 1 ]
            oleg_nenashev Oleg Nenashev made changes -
            Labels stapler lts-candidate stapler
            Hide
            jglick 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.

            Show
            jglick 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.
            Hide
            mdelaney Mike Delaney added a comment -

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

            Show
            mdelaney Mike Delaney added a comment - For us afflicted by this, is there a way to mitigate the issue?
            Hide
            olivergondza Oliver Gondža added a comment -

            Mike Delaney, it is fixed in 2.74 weekly release.

            Show
            olivergondza Oliver Gondža added a comment - Mike Delaney , it is fixed in 2.74 weekly release.
            olivergondza Oliver Gondža made changes -
            Labels lts-candidate stapler 2.73.1-rejected lts-candidate stapler
            Hide
            mdelaney Mike Delaney added a comment -

            Oliver Gondža, awesome. Thanks for the confirmation.

            Show
            mdelaney Mike Delaney added a comment - Oliver Gondža , awesome. Thanks for the confirmation.
            Hide
            jglick Jesse Glick added a comment -

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

            Show
            jglick Jesse Glick added a comment - As written above, I am not convinced that what was fixed is what people think they are reporting.
            Hide
            gumoro 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)

            Show
            gumoro 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)
            Hide
            danielbeck 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.

            Show
            danielbeck 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.
            Hide
            gumoro 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.

            Show
            gumoro 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.
            Hide
            danielbeck 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 Stephen Connolly). 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 Jesse Glick 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()
            Show
            danielbeck 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 Stephen Connolly ). 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 Jesse Glick 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()
            Hide
            gumoro 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

            Show
            gumoro 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
            Hide
            stephenconnolly Stephen Connolly added a comment -

            Daniel Beck 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

            Show
            stephenconnolly Stephen Connolly added a comment - Daniel Beck 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
            Hide
            olivergondza 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.

            Show
            olivergondza 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 .
            olivergondza Oliver Gondža made changes -
            Labels 2.73.1-rejected lts-candidate stapler 2.73.1-rejected stapler
            Hide
            danielbeck Daniel Beck added a comment -

            Oliver Gondža How about a Stapler 1.250.1, if feasible?

            Show
            danielbeck Daniel Beck added a comment - Oliver Gondža How about a Stapler 1.250.1, if feasible?
            Hide
            olivergondza Oliver Gondža added a comment -

            Daniel Beck, I do not oppose if this is serious enough.

            Show
            olivergondza Oliver Gondža added a comment - Daniel Beck , I do not oppose if this is serious enough.
            danielbeck Daniel Beck made changes -
            Labels 2.73.1-rejected stapler 2.73.1-rejected lts-candidate stapler
            Hide
            danielbeck 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 Oliver Gondža Do we need KK to release Stapler, or could someone else do this?

            Show
            danielbeck 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 Oliver Gondža Do we need KK to release Stapler, or could someone else do this?
            Hide
            jglick Jesse Glick added a comment -

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

            Show
            jglick Jesse Glick added a comment - I at least am able to release Stapler if this is under consideration for 2.73.3.
            Hide
            danielbeck Daniel Beck added a comment -

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

            Show
            danielbeck Daniel Beck added a comment - Oliver Gondža See above – we can add it if you think the backport to Stapler 1.250 is reasonable for 2.73.3 inclusion.
            olivergondza Oliver Gondža made changes -
            Labels 2.73.1-rejected lts-candidate stapler 2.73.1-rejected 2.73.2-rejected 2.73.3-rejected stapler

              People

              • Assignee:
                stephenconnolly Stephen Connolly
                Reporter:
                stephenconnolly Stephen Connolly
              • Votes:
                2 Vote for this issue
                Watchers:
                8 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved: