File.getText() should close the streams

Discussed here:
http://stackoverflow.com/questions/7570353/why-do-i-get-too-many-open-files-errors

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.

Groovy Version: 1.8.0 JVM: 1.6.0_14

have you a program showing that problem?

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

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.

Paul, isn't that something for you?

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.

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.

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

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

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

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

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.

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.

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.

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

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+/) {
  ...
}

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.

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?

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.

The change seems to prevent the exception now.

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?

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