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)

          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: