groovy
  1. groovy
  2. GROOVY-5049

File.getText() should close the streams

    Details

    • Type: Bug Bug
    • Status: Closed Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.9-beta-4, 1.8.4
    • Component/s: None
    • Labels:
      None
    • Number of attachments :
      1

      Description

      Performing a lot of system calls causes Groovy to die on Linux with an IOException for "Too many open files". The cause is streams which are left open by system calls:

      (1..n).each

      { "ls".execute().text }

      This method should be changed to close all streams after it gets the text from them.

        Issue Links

          Activity

          Show
          Uri Moszkowicz added a comment - Discussed here: http://stackoverflow.com/questions/7570353/why-do-i-get-too-many-open-files-errors
          Hide
          blackdrag blackdrag added a comment -

          Uri, what version of Groovy are you talking about? File.text will use BuffereReader.text and the code for that looks like this:

              public static String getText(BufferedReader reader) throws IOException {
                  StringBuilder answer = new StringBuilder();
                  // reading the content of the file within a char buffer
                  // allow to keep the correct line endings
                  char[] charBuffer = new char[8192];
                  int nbCharRead /* = 0*/;
                  try {
                      while ((nbCharRead = reader.read(charBuffer)) != -1) {
                          // appends buffer
                          answer.append(charBuffer, 0, nbCharRead);
                      }
                      Reader temp = reader;
                      reader = null;
                      temp.close();
                  } finally {
                      closeWithWarning(reader);
                  }
                  return 
              }
          

          I know we had problems with that kind of thing quite some time back, but these are supposed to be solved. And the code above does close the Reader.

          Show
          blackdrag blackdrag added a comment - Uri, what version of Groovy are you talking about? File.text will use BuffereReader.text and the code for that looks like this: public static String getText(BufferedReader reader) throws IOException { StringBuilder answer = new StringBuilder(); // reading the content of the file within a char buffer // allow to keep the correct line endings char [] charBuffer = new char [8192]; int nbCharRead /* = 0*/; try { while ((nbCharRead = reader.read(charBuffer)) != -1) { // appends buffer answer.append(charBuffer, 0, nbCharRead); } Reader temp = reader; reader = null ; temp.close(); } finally { closeWithWarning(reader); } return } I know we had problems with that kind of thing quite some time back, but these are supposed to be solved. And the code above does close the Reader.
          Hide
          Uri Moszkowicz added a comment -

          Groovy Version: 1.8.0 JVM: 1.6.0_14

          Show
          Uri Moszkowicz added a comment - Groovy Version: 1.8.0 JVM: 1.6.0_14
          Hide
          blackdrag blackdrag added a comment -

          have you a program showing that problem?

          Show
          blackdrag blackdrag added a comment - have you a program showing that problem?
          Hide
          Uri Moszkowicz added a comment -

          Yes please see the snippet in the stackoverflow thread linked above. I think the input, output, and error streams need to be closed.

          Show
          Uri Moszkowicz added a comment - Yes please see the snippet in the stackoverflow thread linked above. I think the input, output, and error streams need to be closed.
          Hide
          blackdrag blackdrag added a comment -

          Ok, so the problem is on Process, not on File. I haven't seen that, sorry. Process#getText closes the InputStream. The question is if it makes sense to also close error and output streams through this method. I guess that is something we can do, but we would potentially break some code. Also if we go this route we have to think of the case of process error and/or process input stream buffers being full and thus blocking the process from finnishing what it had to tell us.

          Show
          blackdrag blackdrag added a comment - Ok, so the problem is on Process, not on File. I haven't seen that, sorry. Process#getText closes the InputStream. The question is if it makes sense to also close error and output streams through this method. I guess that is something we can do, but we would potentially break some code. Also if we go this route we have to think of the case of process error and/or process input stream buffers being full and thus blocking the process from finnishing what it had to tell us.
          Hide
          blackdrag blackdrag added a comment -

          Paul, isn't that something for you?

          Show
          blackdrag blackdrag added a comment - Paul, isn't that something for you?
          Hide
          Uri Moszkowicz added a comment -

          Hmm yeah maybe it should just close the STDOUT stream. It will break some legacy code but in those cases the stream would have been empty anyway. If my proposal for new mergedText and close methods are accepted, then we'll be able to concicesly close all the streams in all cases. We'll just need to write some notes documenting the pitfalls of executing commands and how to avoid them using the new methods.

          Show
          Uri Moszkowicz added a comment - Hmm yeah maybe it should just close the STDOUT stream. It will break some legacy code but in those cases the stream would have been empty anyway. If my proposal for new mergedText and close methods are accepted, then we'll be able to concicesly close all the streams in all cases. We'll just need to write some notes documenting the pitfalls of executing commands and how to avoid them using the new methods.
          Hide
          Paul King added a comment -

          I see a stacktrace on stackoverflow but no code. Perhaps I am missing something. Do you have some code which illustrates the problem you are talking about? Thanks.

          Show
          Paul King added a comment - I see a stacktrace on stackoverflow but no code. Perhaps I am missing something. Do you have some code which illustrates the problem you are talking about? Thanks.
          Hide
          Uri Moszkowicz added a comment - - edited

          Attached file to bug report. You can uncomment the "No error" code to see that closing the streams does fix the error.

          Show
          Uri Moszkowicz added a comment - - edited Attached file to bug report. You can uncomment the "No error" code to see that closing the streams does fix the error.
          Hide
          Uri Moszkowicz added a comment - - edited

          ...
          <removed dir>
          <removed dir>
          <removed dir>
          Caught: java.util.concurrent.ExecutionException: org.codehaus.groovy.runtime.InvokerInvocationException: java.io.IOException: Cannot run program "ls": java.io.IOException: error=24, Too many open files
          at groovyx.gpars.GParsPool.runForkJoin(GParsPool.groovy:305)
          at UsageAnalyzer2$_run_closure2_closure6.doCall(UsageAnalyzer2.groovy:36)
          at groovyx.gpars.GParsPool$_withExistingPool_closure1.doCall(GParsPool.groovy:170)
          at groovyx.gpars.GParsPool$_withExistingPool_closure1.doCall(GParsPool.groovy)
          at groovyx.gpars.GParsPool.withExistingPool(GParsPool.groovy:169)
          at groovyx.gpars.GParsPool.withPool(GParsPool.groovy:141)
          at groovyx.gpars.GParsPool.withPool(GParsPool.groovy:117)
          at UsageAnalyzer2$_run_closure2.doCall(UsageAnalyzer2.groovy:35)

          Show
          Uri Moszkowicz added a comment - - edited ... <removed dir> <removed dir> <removed dir> Caught: java.util.concurrent.ExecutionException: org.codehaus.groovy.runtime.InvokerInvocationException: java.io.IOException: Cannot run program "ls": java.io.IOException: error=24, Too many open files at groovyx.gpars.GParsPool.runForkJoin(GParsPool.groovy:305) at UsageAnalyzer2$_run_closure2_closure6.doCall(UsageAnalyzer2.groovy:36) at groovyx.gpars.GParsPool$_withExistingPool_closure1.doCall(GParsPool.groovy:170) at groovyx.gpars.GParsPool$_withExistingPool_closure1.doCall(GParsPool.groovy) at groovyx.gpars.GParsPool.withExistingPool(GParsPool.groovy:169) at groovyx.gpars.GParsPool.withPool(GParsPool.groovy:141) at groovyx.gpars.GParsPool.withPool(GParsPool.groovy:117) at UsageAnalyzer2$_run_closure2.doCall(UsageAnalyzer2.groovy:35)
          Hide
          blackdrag blackdrag added a comment -

          @Paul, the example basically is (1..n).each

          { "ls".execute().text }

          @Uri, getText does close STDOUT. Since you are not happy with that, it is obviously not enough.

          Show
          blackdrag blackdrag added a comment - @Paul, the example basically is (1..n).each { "ls".execute().text } @Uri, getText does close STDOUT. Since you are not happy with that, it is obviously not enough.
          Hide
          Uri Moszkowicz added a comment -

          It seems that the output stream can be closed safely once the process has exited. It makes sense that text would only close STDOUT since it only reads STDOUT. What's missing is the ability to read both STDOUT and STDERR and close both concisely.

          Show
          Uri Moszkowicz added a comment - It seems that the output stream can be closed safely once the process has exited. It makes sense that text would only close STDOUT since it only reads STDOUT. What's missing is the ability to read both STDOUT and STDERR and close both concisely.
          Hide
          Paul King added a comment -

          Changing the behavior of the DGM process methods is certainly a possibility. However, the current "design" of many of these methods is very much a mix and match toolkit style. The getText() method has never been designed to be an all-in-one solution, rather a light-weight layer over the existing process methods to make them a little bit nicer.

          A common existing usage pattern is:

          p = "ls = build.gradle".execute()
          p.waitFor()
          println "O: " + p.text      // O: build.gradle
          println "E: " + p.err.text  // E: ls: =: No such file or directory
          

          If getText() closed all the streams, then obviously the second println above would fail.

          The existing methods allow closing of the error stream directly if desired:

          def p = "ls = build.gradle".execute()
          p.waitFor()
          p.err.close()
          println "O:\n" + p.text
          

          Trying to change the existing methods is possible but might not be what people expect given the current mix and match style.

          At the moment, the closest we have to an all in one solution is the waitForProcessOutput() methods. At the moment, they don't close the output and error streams but probably should. In the first instance, that is what I am going to fix. That will then allow the following solutions to the above problem:

          p = "ls = build.gradle".execute()
          def sout = new StringBuilder()
          def serr = new StringBuilder()
          p.waitForProcessOutput(sout, serr)
          println "O:\n" + sout
          println "E:\n" + serr
          

          Or if merging of the streams is desired:

          p = "ls = build.gradle".execute()
          def sb = new StringBuffer()
          p.waitForProcessOutput(sb, sb)
          println "OE:\n" + sb
          

          This should stop the "Too many open files" error albeit not in as compact a syntax as might be desired.

          I think a more compact solution needs further discussion. It might need to be separate from the existing solutions or we might have to bite the bullet and do some wrapping of Process objects.

          Show
          Paul King added a comment - Changing the behavior of the DGM process methods is certainly a possibility. However, the current "design" of many of these methods is very much a mix and match toolkit style. The getText() method has never been designed to be an all-in-one solution, rather a light-weight layer over the existing process methods to make them a little bit nicer. A common existing usage pattern is: p = "ls = build.gradle" .execute() p.waitFor() println "O: " + p.text // O: build.gradle println "E: " + p.err.text // E: ls: =: No such file or directory If getText() closed all the streams, then obviously the second println above would fail. The existing methods allow closing of the error stream directly if desired: def p = "ls = build.gradle" .execute() p.waitFor() p.err.close() println "O:\n" + p.text Trying to change the existing methods is possible but might not be what people expect given the current mix and match style. At the moment, the closest we have to an all in one solution is the waitForProcessOutput() methods. At the moment, they don't close the output and error streams but probably should. In the first instance, that is what I am going to fix. That will then allow the following solutions to the above problem: p = "ls = build.gradle" .execute() def sout = new StringBuilder() def serr = new StringBuilder() p.waitForProcessOutput(sout, serr) println "O:\n" + sout println "E:\n" + serr Or if merging of the streams is desired: p = "ls = build.gradle" .execute() def sb = new StringBuffer () p.waitForProcessOutput(sb, sb) println "OE:\n" + sb This should stop the "Too many open files" error albeit not in as compact a syntax as might be desired. I think a more compact solution needs further discussion. It might need to be separate from the existing solutions or we might have to bite the bullet and do some wrapping of Process objects.
          Hide
          Paul King added a comment -

          OK, change made. If you are in a position to test your example with a snapshot jar and alter your code to use the slightly longer version usign waitForProcessOutput(), that would be great. It would be interesting to ensure that the errors go away. At present I am not closing the input stream as none should be in use but it would be useful to see what results you get.

          Show
          Paul King added a comment - OK, change made. If you are in a position to test your example with a snapshot jar and alter your code to use the slightly longer version usign waitForProcessOutput() , that would be great. It would be interesting to ensure that the errors go away. At present I am not closing the input stream as none should be in use but it would be useful to see what results you get.
          Hide
          Uri Moszkowicz added a comment -

          Sure I could test it. Just send me the file and instructions on how to install it.

          Show
          Uri Moszkowicz added a comment - Sure I could test it. Just send me the file and instructions on how to install it.
          Hide
          Uri Moszkowicz added a comment -

          It might be useful to have an example for evaluating proposals. I'll suggest the following snippet, written in Perl:

          if (`ls *.ext1` =~ /\S+/) {
            ...
          } elsif (`ls *.ext2` =~ /\S+/) {
            ...
          }
          

          Right now, it would look like this.

          def sb1 = new StringBuffer()
          def process1 = "ls *.ext1".execute()
          process1.waitForProcessOutput(sb1, sb1)
          
          def sb2 = new StringBuffer()
          def process2 = "ls *.ext2".execute()
          process2.waitForProcessOutput(sb2, sb2)
          
          if (sb1 =~ /\S+/) {
            ...
          else if (sb2 =~ /\S+/) {
            ...
          }
          
          Show
          Uri Moszkowicz added a comment - It might be useful to have an example for evaluating proposals. I'll suggest the following snippet, written in Perl: if (`ls *.ext1` =~ /\S+/) { ... } elsif (`ls *.ext2` =~ /\S+/) { ... } Right now, it would look like this. def sb1 = new StringBuffer () def process1 = "ls *.ext1" .execute() process1.waitForProcessOutput(sb1, sb1) def sb2 = new StringBuffer () def process2 = "ls *.ext2" .execute() process2.waitForProcessOutput(sb2, sb2) if (sb1 =~ /\S+/) { ... else if (sb2 =~ /\S+/) { ... }
          Hide
          Paul King added a comment -

          If you grab a snapshot jar from our CI server and just replace your existing groovy or groovy-all jar.

          http://snapshots.repository.codehaus.org/org/codehaus/groovy/
          

          You should grab the one closest to your current versions, pick groovy/groovy-all and pick the latest 1.7.x or 1.8.x jar depending on what you are currently using.

          Show
          Paul King added a comment - If you grab a snapshot jar from our CI server and just replace your existing groovy or groovy-all jar. http://snapshots.repository.codehaus.org/org/codehaus/groovy/ You should grab the one closest to your current versions, pick groovy/groovy-all and pick the latest 1.7.x or 1.8.x jar depending on what you are currently using.
          Hide
          Uri Moszkowicz added a comment -

          I grabbed groovy-all-1.8.3-SNAPSHOT.jar and placed it into my groovy embeddable directory and renamed it to groovy-all-1.8.2.jar. I then changed the script to use waitForProcessOutput() and ran it in the case showing the error and it still occurs. Did I test it right?

          Show
          Uri Moszkowicz added a comment - I grabbed groovy-all-1.8.3-SNAPSHOT.jar and placed it into my groovy embeddable directory and renamed it to groovy-all-1.8.2.jar. I then changed the script to use waitForProcessOutput() and ran it in the case showing the error and it still occurs. Did I test it right?
          Hide
          Paul King added a comment -

          It depends on how your are running groovy. If you are referencing the groovy-all jar directly (e.g. in an IDE) then you have done the correct steps. If you are using groovy from the commandline or groovyConsole in is likely using the non-all jar in the lib directory.

          Show
          Paul King added a comment - It depends on how your are running groovy. If you are referencing the groovy-all jar directly (e.g. in an IDE) then you have done the correct steps. If you are using groovy from the commandline or groovyConsole in is likely using the non-all jar in the lib directory.
          Hide
          Uri Moszkowicz added a comment -

          The change seems to prevent the exception now.

          Show
          Uri Moszkowicz added a comment - The change seems to prevent the exception now.
          Hide
          Paul King added a comment -

          Cool. Thanks for trying that out. Next we'll have to think about further shorthands! Though perhaps that could be a separate issue given the title of this issue?

          Show
          Paul King added a comment - Cool. Thanks for trying that out. Next we'll have to think about further shorthands! Though perhaps that could be a separate issue given the title of this issue?
          Hide
          Paul King added a comment -

          ok, closing this issue but see GROOVY-5058 for future enhancements

          Show
          Paul King added a comment - ok, closing this issue but see GROOVY-5058 for future enhancements

            People

            • Assignee:
              Paul King
              Reporter:
              Uri Moszkowicz
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: