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.

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...

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

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.

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

mdelaney, it is fixed in 2.74 weekly release.

olivergondza, awesome. Thanks for the confirmation.

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

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)}

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.

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.

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()

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

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.

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

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?

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

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

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