File-Iterator leaves Reader open

[Jörg Staudemeyer]

Hi,

there seems to be a problem with iterators on files not closing their readers. consider this script: 

f = new File('ExampleIterator.txt')
f << 'This is the first line\n'
f << 'This is the second line\n'
f.each { println it }
deleted = f.delete()
println "File is deleted: $deleted" 

prints: 

This is the first line
This is the second line
File is deleted: false 

Obviously, the f.delete() does not work because the file is not closed. When I enclose the each()-call in f.withReader{…} the file will be closed properly. 

Or did I miss something?

Jörg Staudemeyer

[glaforge]

Could you file a JIRA issue about that?

You might have a look at the code in the class DefaultGroovyMethods
which contain those methods (each, eachLine, leftShift and so on) to
see whether it's fixable, and provide us with a patch?

Thanks in advance.

On 5/19/07, Jörg Staudemeyer <joerg@...> wrote:

>
>
>
> Hi,
>
> there seems to be a problem with iterators on files not closing their
> readers. consider this script:
>
> f = new File('ExampleIterator.txt')
>  f << 'This is the first line\n'
>  f << 'This is the second line\n'
>  f.each { println it }
>  deleted = f.delete()
>  println "File is deleted: $deleted"
>
> prints:
>
> This is the first line
>  This is the second line
>  File is deleted: false
>
>
> Obviously, the f.delete() does not work because the file is not closed. When
> I enclose the each()-call in f.withReader{…} the file will be closed
> properly.
>
> Or did I miss something?
>
> Jörg Staudemeyer

--
Guillaume Laforge
Groovy Project Manager
http://glaforge.free.fr/blog/groovy

---------------------------------------------------------------------
To unsubscribe from this list please visit:

    http://xircles.codehaus.org/manage_email

[Joachim Baumann]

Hi,

what happens is the following: .each() works on an iterator, and an
iterator is created for a Reader instantiated for the file instance (in
DefaultGroovyMethods).

Sometime last year I implemented this iterator and did not close the
reader on purpose. If I create an iterator for a Reader then I do not
assume that when I have iterated through the contents that the Reader is
closed. If this assumption is wrong, then the fix is quite simple (I
already have a version that can be tested).

Since e.g., eachLine() does already close the reader closing the reader
might be the way of least surprise ...

So, what is it to be?

Cheers, Joachim
PS: Only two more weeks until I have, again, some spare time for Groovy :-)

Guillaume Laforge schrieb:

> Could you file a JIRA issue about that?
>
> You might have a look at the code in the class DefaultGroovyMethods
> which contain those methods (each, eachLine, leftShift and so on) to
> see whether it's fixable, and provide us with a patch?
>
> Thanks in advance.
>
> On 5/19/07, Jörg Staudemeyer <joerg@...> wrote:
>>
>>
>>
>> Hi,
>>
>> there seems to be a problem with iterators on files not closing their
>> readers. consider this script:
>>
>> f = new File('ExampleIterator.txt')
>>  f << 'This is the first line\n'
>>  f << 'This is the second line\n'
>>  f.each { println it }
>>  deleted = f.delete()
>>  println "File is deleted: $deleted"
>>
>> prints:
>>
>> This is the first line
>>  This is the second line
>>  File is deleted: false
>>
>>
>> Obviously, the f.delete() does not work because the file is not
>> closed. When
>> I enclose the each()-call in f.withReader{…} the file will be closed
>> properly.
>>
>> Or did I miss something?
>>
>> Jörg Staudemeyer
>
>

--
Xinaris GmbH
Flüsselweg 10, 76646 Bruchsal
Tel.: +(49)7251/39264-21    | Sitz der Gesellschaft: Bruchsal
Fax:  +(49)7251/39264-29    | Registergericht: AG Mannheim HRB 232791
WWW:  http://www.xinaris.de | Geschäftsführer: Dr. Joachim Baumann
Mail: info@...       | USt-ID DE 242 975 278


---------------------------------------------------------------------
To unsubscribe from this list please visit:

    http://xircles.codehaus.org/manage_email

[Jörg Staudemeyer]

As always, things get more complicated the more one thinks about.

Joachim Baumann wrote:

>Hi,
>
>what happens is the following: .each() works on an iterator, and an
>iterator is created for a Reader instantiated for the file instance (in
>DefaultGroovyMethods).
>
>Sometime last year I implemented this iterator and did not close the
>reader on purpose. If I create an iterator for a Reader then I do not
>assume that when I have iterated through the contents that the Reader is
>closed. If this assumption is wrong, then the fix is quite simple (I
>already have a version that can be tested).
>
>Since e.g., eachLine() does already close the reader closing the reader
>might be the way of least surprise ...
>
>  
>

I think this is definitely the case. It's a must for File#iterator()
because there is no other chance to close the Reader, and
Reader#iterator() should not behave differently.

But there is another problem, and that cannot be solved as easily. What
happens when an Exception occurs inside the closure? eachLine() and all
the other closure-related GDK methods on files, readers, writers,
streams etc. close their streams no matter what happens. The same
behaviour should be expected from a each(), but each() cannot know about
readers or streams that have to be closed.

The following might be a possible solution:

1. Invent a new interface ClosableIterator or so that subclasses the
simple Iterator interface and defines an additional method called
close(). This ClosableIterator is to be implemented by our embedded
Reader iterator. When this close() method is called, the Reader will be
closed.

public interface ClosableIterator extends Iterator {  void close() }

2. Enhance the each(Object,Closure) GDK method in a way, that it catches
all exceptions inside the closure. In case of an exception it checks if
the iterator is an instance of ClosableIterator. If yes, it calls the
the its close() method. Afterwords the exception is rethrown.

public static void each(Object self, Closure closure) {
    try {
        for (Iterator iter = InvokerHelper.asIterator(self);
iter.hasNext();) {
            closure.call(iter.next());
        }
    } catch (Throwable t) {
        if (iter implements ClosableIterator) {
            ((ClosableIterator)iter).close();
        }
    }
 }

This solution may even be helpful in other cases where some action has
to be taken when an iterative closure aborts prematurely.

You could also write specialized each() methods, but the solution above
looks a bit more nifty, doesn't it?

>Guillaume Laforge schrieb:
>  
>
>>Could you file a JIRA issue about that?
>>
>>You might have a look at the code in the class DefaultGroovyMethods
>>which contain those methods (each, eachLine, leftShift and so on) to
>>see whether it's fixable, and provide us with a patch?
>>    
>>

This should be left to Joachim Baumann, seems to be his child. But let
me know, if I can help.
Have a good weekend! Jörg Staudemeyer

>>Thanks in advance.
>>
>>On 5/19/07, Jörg Staudemeyer <joerg@...> wrote:
>>    
>>
>>>
>>>Hi,
>>>
>>>there seems to be a problem with iterators on files not closing their
>>>readers. consider this script:
>>>
>>>f = new File('ExampleIterator.txt')
>>> f << 'This is the first line\n'
>>> f << 'This is the second line\n'
>>> f.each { println it }
>>> deleted = f.delete()
>>> println "File is deleted: $deleted"
>>>
>>>prints:
>>>
>>>This is the first line
>>> This is the second line
>>> File is deleted: false
>>>
>>>
>>>Obviously, the f.delete() does not work because the file is not
>>>closed. When
>>>I enclose the each()-call in f.withReader{…} the file will be closed
>>>properly.
>>>
>>>Or did I miss something?
>>>
>>>Jörg Staudemeyer
>>>      
>>>
>>    
>>
>
>
>  
>

---------------------------------------------------------------------
To unsubscribe from this list please visit:

    http://xircles.codehaus.org/manage_email

[Jochen Theodorou]

Joerg Staudemeyer wrote:
[...]

> 2. Enhance the each(Object,Closure) GDK method in a way, that it catches
> all exceptions inside the closure. In case of an exception it checks if
> the iterator is an instance of ClosableIterator. If yes, it calls the
> the its close() method. Afterwords the exception is rethrown.
>
> public static void each(Object self, Closure closure) {
>    try {
>        for (Iterator iter = InvokerHelper.asIterator(self);
> iter.hasNext();) {
>            closure.call(iter.next());
>        }
>    } catch (Throwable t) {
>        if (iter implements ClosableIterator) {
>            ((ClosableIterator)iter).close();
>        }
>    }
> }
>
> This solution may even be helpful in other cases where some action has
> to be taken when an iterative closure aborts prematurely.
>
> You could also write specialized each() methods, but the solution above
> looks a bit more nifty, doesn't it?

but a specialiced each method for File, InputStream and OutputStream
would allow us to operate without the ClosableIterator interface. and do
not forget that it does not solve the case of

for (l in file) {

}

How about letting the Iterator close the file/stream if next or hasNext
is failing?

bye blackdrag


---------------------------------------------------------------------
To unsubscribe from this list please visit:

    http://xircles.codehaus.org/manage_email

[Jörg Staudemeyer-2]

Jochen Theodorou wrote:

> Joerg Staudemeyer wrote:
> [...]
>
>> 2. Enhance the each(Object,Closure) GDK method in a way, that it catches all exceptions inside the closure. In case of an exception it checks if the iterator is an instance of ClosableIterator. If yes, it calls the the its close() method. Afterwords the exception is rethrown.
>>
>> public static void each(Object self, Closure closure) {
>>    try {
>>        for (Iterator iter = InvokerHelper.asIterator(self); iter.hasNext();) {
>>            closure.call(iter.next());
>>        }
>>    } catch (Throwable t) {
>>        if (iter implements ClosableIterator) {
>>            ((ClosableIterator)iter).close();
>>        }
>>    }
>> }
>>
>> This solution may even be helpful in other cases where some action has to be taken when an iterative closure aborts prematurely.
>>
>> You could also write specialized each() methods, but the solution above looks a bit more nifty, doesn't it?
>
>
> but a specialiced each method for File, InputStream and OutputStream would allow us to operate without the ClosableIterator interface. and do not forget that it does not solve the case of
>
> for (l in file) {
>
> }
>
> How about letting the Iterator close the file/stream if next or hasNext is failing?

Thanks for your comment. Of course, the latter should be done anyway. This was the starting point of the discussion. The more challenging problem is, what happens if an exception occurs inside the loop. All similar loop methods in GDK, e.g eachRow(), eachByte(), eachObject(), ... close their resources in case of an interruption, and so each() should do.

The proposed ClosableIterator is a general solution to this issue. I would prefer it, since it doesn't leave the responsibility to provide an each() loop to any programmer of an iterable object that must close its resources after the loop is done. In contrast, providing a ClosabeIterator is almost no extra work.

The for-loop you mention is another reason for providing a general solution, because there are no specialized for-loops. This problem is not so easily to be solved. I dared to dig a little into the compiled code and found out, that for-loops are directly compiled into bytecode. Here I see no other way but to change the generated code in a way that it checks for exceptions occurring inside the loop and does something similar to my enhanced each() method above - presumedly delegating ScriptBytecodeAdapter.

Or are there other ideas? I think leaving ressouces simply open is a no-no, particularly if the user has no chance to handle it himself.
Let me know, please.

Jörg Staudemeyer

---------------------------------------------------------------------
To unsubscribe from this list please visit:

    http://xircles.codehaus.org/manage_email

[Joachim Baumann]

Hi Jochen,
> but a specialiced each method for File, InputStream and OutputStream
> would allow us to operate without the ClosableIterator interface. and
> do not forget that it does not solve the case of
>
> for (l in file) {
>
> }
I like that idea very much. It makes what happens in
DefaultGroovyMethods much clearer and allows a more precise control.

Regarding the ClosableIterator-Interface: Starting with 1.5 a
Closable-Interface exists in java.io, which would be perfectly suited.
And this is where I stop before starting another 1.4 vs. 1.5 war :-)
Nonetheless, the ClosableIterator-Interface does not seem like a bad
idea at all. Jochen, do you have objections to such an interface?

>
> How about letting the Iterator close the file/stream if next or
> hasNext is failing?
>
This is exactly what my correction would do, for the Reader iterator as
well as for the DataInputStream iterator.

The underlying question still is: Is this the Right Thing (TM) to do for
an iterator that takes a Reader? Important here is the differentiation.
We do not talk about a File, but a Reader (the File case would be
handled by the solution above). In this case we have a reference to the
reader and can either reset it or close it ourselves.

Jochen, your answer seems to be yes, close it. Jörgs answer definitely
is yes.I'm happy either way.

Are there other comments?

If not, then I would commit the first change to the iterators at once
and would write the File- and Stream-specific implementations as well as
the ClosableIterator as soon as my current project is done (beginning of
June). Alternatively Jörg could do this :-)

Cheers, Joachim

---------------------------------------------------------------------
To unsubscribe from this list please visit:

    http://xircles.codehaus.org/manage_email

[Jochen Theodorou]

Joerg Staudemeyer wrote:
[...]
> The more challenging
> problem is, what happens if an exception occurs inside the loop. All
> similar loop methods in GDK, e.g eachRow(), eachByte(), eachObject(),
> ... close their resources in case of an interruption, and so each()
> should do.

ahh.. looks like I misunderstood the problem. I thought about closing
due to an IOException, not about closing when there is any exception in
the closure block. That is a problem, indeed. In case of the for loop
the closure is inlined, so there is normally no closure body, that we
can supervise. hmm.. I need to put any loop into a try-finally-block and
test the iterator if the iterator is closable, and change each iterator
to be closeable... and other iterators may not be affected... somehow I
am not happy.

> The proposed ClosableIterator is a general solution to this issue.

General, but not very automatic... it requires lots of explicit work.

> I
> would prefer it, since it doesn't leave the responsibility to provide
> an each() loop to any programmer of an iterable object that must
> close its resources after the loop is done. In contrast, providing a
> ClosabeIterator is almost no extra work.

I am not so much concenred about more work, I am more concerned about
hidden logic used somewhere inside, that applies to some iterators and
not to others wihtout insight by the user which does and which does not.

> The for-loop you mention is another reason for providing a general
> solution, because there are no specialized for-loops. This problem is
> not so easily to be solved.

I agree

> I dared to dig a little into the compiled
> code and found out, that for-loops are directly compiled into
> bytecode.

yes, we get an Iterator and then use it to do a java5 style loop

> Here I see no other way but to change the generated code in
> a way that it checks for exceptions occurring inside the loop and
> does something similar to my enhanced each() method above -
> presumedly delegating ScriptBytecodeAdapter.

maybe, yes.

> Or are there other ideas? I think leaving ressouces simply open is a
> no-no, particularly if the user has no chance to handle it himself.
> Let me know, please.

I need to think a bit more about the problem first.

bye blackdrag

---------------------------------------------------------------------
To unsubscribe from this list please visit:

    http://xircles.codehaus.org/manage_email

[Jörg Staudemeyer]

Hi,

2007/5/20, Jochen Theodorou <blackdrag@...>:

I need to think a bit more about the problem first.

To not leave the whole thing to oblivion, I entered it in Jira (GROOVY-1904) as suggested by Guillaume. I added a testcase with a bunch of tests to clarify the issue.

Meanwhile (while you are thinking) we (Joachim or I) could implement the closing of the streams as a partial solution.


off topic:

Joachim,
when writing the testcase I noticed that the iterator for ObjectInputStream returns bytes. I can hardly imagine what for. In Groovy everything is an object and the normally it's objects that will be serialized and deserialized. Or am I missing something?

Danke und Grüße
Jörg Staudemeyer

[Jochen Theodorou]

Jörg Staudemeyer schrieb:
> Hi,
> 2007/5/20, Jochen Theodorou <blackdrag@... <mailto:blackdrag@...>>:
>
>
>     I need to think a bit more about the problem first.
>
> To not leave the whole thing to oblivion, I entered it in Jira
> (GROOVY-1904) as suggested by Guillaume. I added a testcase with a bunch
> of tests to clarify the issue.

thx... I came to the conclusion the the iterator should be responsible
for closing the stream in hasNext() and next(). And the iteration method
(like each) is responsible for closing in case of an exception in the
closure. In case of the for-loop... Maybe we should remove some iterator
methods to keep only the save ways. I mean that would only be
File#iterator, or not? In other cases it is still possible to close it.
By removing I mean to let the method throw an DeprecatedException.

thoughts?

bye blackdrag

--
Jochen "blackdrag" Theodorou
Groovy Tech Lead (http://groovy.codehaus.org)
http://blackdragsview.blogspot.com/

---------------------------------------------------------------------
To unsubscribe from this list please visit:

    http://xircles.codehaus.org/manage_email

[Russel Winder]

On Mon, 2007-05-21 at 17:19 +0200, Jochen Theodorou wrote:

> thx... I came to the conclusion the the iterator should be responsible
> for closing the stream in hasNext() and next(). And the iteration method
> (like each) is responsible for closing in case of an exception in the
> closure. In case of the for-loop... Maybe we should remove some iterator
> methods to keep only the save ways. I mean that would only be
> File#iterator, or not? In other cases it is still possible to close it.
> By removing I mean to let the method throw an DeprecatedException.
>
> thoughts?

There is a pattern/idiom from C++ called Resource Acquisition Is
Initialization (RAII), that is now considered the standard paradigm of
resource management.  The point is that programmers should never have to
explicitly close files and free resources, these things should happen
automatically when variables holding the resources go out of scope.

Ruby and Groovy support a variation on this when using the iteration
methods and closures.

        file.each { }

should open the file, do the iteration and then close the file.
Actually I thought this is what Groovy actually did.  Certainly Ruby
does.

--
Russel.
====================================================
Dr Russel Winder                [phone]
41 Buckmaster Road              [phone]
London SW11 1EN, UK             russel@...

[Jochen Theodorou]

Russel Winder schrieb:

> On Mon, 2007-05-21 at 17:19 +0200, Jochen Theodorou wrote:
>
>> thx... I came to the conclusion the the iterator should be responsible
>> for closing the stream in hasNext() and next(). And the iteration method
>> (like each) is responsible for closing in case of an exception in the
>> closure. In case of the for-loop... Maybe we should remove some iterator
>> methods to keep only the save ways. I mean that would only be
>> File#iterator, or not? In other cases it is still possible to close it.
>> By removing I mean to let the method throw an DeprecatedException.
>>
>> thoughts?
>
> There is a pattern/idiom from C++ called Resource Acquisition Is
> Initialization (RAII), that is now considered the standard paradigm of
> resource management.  The point is that programmers should never have to
> explicitly close files and free resources, these things should happen
> automatically when variables holding the resources go out of scope.
>
> Ruby and Groovy support a variation on this when using the iteration
> methods and closures.
>
> file.each { }
>
> should open the file, do the iteration and then close the file.
> Actually I thought this is what Groovy actually did.  Certainly Ruby
> does.

If it does not we can make it doing so, that is not the problem. The
method has full control and can be packed with custom logic to
accomplish the goal. The problem is

for (l in file) {
...
}

There is no method that could take control and I see no logic that would
work with every iterator too. So we can ignore the case here, "remove"
the iterator from file or add an interface to the iterator (as suggested).

removing the file iterator would solve the problem somehow ;) Adding the
interface needs prominent documentation for the api writers. But adding
an interfae that basically already exists in java5 gives me a headache.

As for RAII... there is no ressource in scope. I mean the file is opened
somewhere else, the reader must live longer then the method that opened
the reader and so on. It is closed when the reader is collected.. so I
don't see RAII violated, it dos not fully apply as there is no variable
holding the resource. Only problem is that it might not be collected
before the system looses the ability to give more file handles.

bye blackdrag

--
Jochen "blackdrag" Theodorou
Groovy Tech Lead (http://groovy.codehaus.org)
http://blackdragsview.blogspot.com/

---------------------------------------------------------------------
To unsubscribe from this list please visit:

    http://xircles.codehaus.org/manage_email

[Jörg Staudemeyer]

2007/5/21, Jochen Theodorou <blackdrag@...>:

Russel Winder schrieb:

> On Mon, 2007-05-21 at 17:19 +0200, Jochen Theodorou wrote:

>
>> thx... I came to the conclusion the the iterator should be responsible
>> for closing the stream in hasNext() and next(). And the iteration method
>> (like each) is responsible for closing in case of an exception in the
>> closure. In case of the for-loop... Maybe we should remove some iterator
>> methods to keep only the save ways. I mean that would only be
>> File#iterator, or not? In other cases it is still possible to close it.
>> By removing I mean to let the method throw an DeprecatedException.
>>
>> thoughts?
>
> There is a pattern/idiom from C++ called Resource Acquisition Is
> Initialization (RAII), that is now considered the standard paradigm of
> resource management.  The point is that programmers should never have to
> explicitly close files and free resources, these things should happen
> automatically when variables holding the resources go out of scope.
>
> Ruby and Groovy support a variation on this when using the iteration
> methods and closures.
>
>       file.each { }
>
> should open the file, do the iteration and then close the file.
> Actually I thought this is what Groovy actually did.  Certainly Ruby
> does.

If it does not we can make it doing so, that is not the problem. The
method has full control and can be packed with custom logic to
accomplish the goal. The problem is

for (l in file) {
...
}

There is no method that could take control and I see no logic that would
work with every iterator too. So we can ignore the case here, "remove"
the iterator from file or add an interface to the iterator (as suggested).

removing the file iterator would solve the problem somehow ;) Adding the
interface needs prominent documentation for the api writers. But adding
an interfae that basically already exists in java5 gives me a headache.

Groovians will be experiencing this headache more often in future. What do you think will happen when Java adopts closures, as is discussed widely? Nonetheless, I'm convinced by your reasoning; simply "removing" the iterator(File) method and enhancing the each() methods is simple und easy to realize. One can't do for-loop with files any more, but for-loops are more or less candy for Groovy-newbies, aren't they?

As for RAII... there is no ressource in scope. I mean the file is opened
somewhere else, the reader must live longer then the method that opened
the reader and so on. It is closed when the reader is collected.. so I
don't see RAII violated, it dos not fully apply as there is no variable
holding the resource. Only problem is that it might not be collected
before the system looses the ability to give more file handles.

The other problem is - the resource may be locked until garbage collection. I stumbled on this because I could not close a file that I had written shortly befor in an each-loop. For that reason all each-loops must catch all exceptions occuring inside the loop and close the resource explicitly.

If you agree, I will add a remark to GROOVY-1904 according to your (Blackdrag's) statement and than that's the way it goes.

Jörg Staudemeyer

[Jochen Theodorou]

Jörg Staudemeyer schrieb:
>
> 2007/5/21, Jochen Theodorou <blackdrag@... <mailto:blackdrag@...>>:
[...]
> If you agree, I will add a remark to GROOVY-1904 according to your
> (Blackdrag's) statement and than that's the way it goes.

that is make File#iterator throw an DeprecatedException and let the
iteration method handle the closing? Then yes, I agree.

bye blackdrag

--
Jochen "blackdrag" Theodorou
Groovy Tech Lead (http://groovy.codehaus.org)
http://blackdragsview.blogspot.com/

---------------------------------------------------------------------
To unsubscribe from this list please visit:

    http://xircles.codehaus.org/manage_email

[Jörg Staudemeyer]

2007/5/22, Jochen Theodorou <blackdrag@...>:

Jörg Staudemeyer schrieb:
>
> 2007/5/21, Jochen Theodorou <blackdrag@... <mailto:blackdrag@...>>:
[...]
> If you agree, I will add a remark to GROOVY-1904 according to your
> (Blackdrag's) statement and than that's the way it goes.

that is make File#iterator throw an DeprecatedException and let the
iteration method handle the closing? Then yes, I agree.

OK. wrote a quite comprehensive comment stating all details that have to be changed. Implemention won't be challenging, I suppose.

Since I'm relatively new to all this - what is the further procedure?

Jörg

[Jochen Theodorou]

Jörg Staudemeyer schrieb:

> 2007/5/22, Jochen Theodorou <blackdrag@... <mailto:blackdrag@...>>:
>
>     Jörg Staudemeyer schrieb:
>      >
>      > 2007/5/21, Jochen Theodorou <blackdrag@...
>     <mailto:blackdrag@...> <mailto:blackdrag@...
>     <mailto:blackdrag@...>>>:
>     [...]
>      > If you agree, I will add a remark to GROOVY-1904 according to your
>      > (Blackdrag's) statement and than that's the way it goes.
>
>     that is make File#iterator throw an DeprecatedException and let the
>     iteration method handle the closing? Then yes, I agree.
>
>
> OK. wrote a quite comprehensive comment stating all details that have to
> be changed. Implemention won't be challenging, I suppose.
>
> Since I'm relatively new to all this - what is the further procedure?

If you want to, you can write a patch and attach it to the issue.

bye blackdrag

--
Jochen "blackdrag" Theodorou
Groovy Tech Lead (http://groovy.codehaus.org)
http://blackdragsview.blogspot.com/

---------------------------------------------------------------------
To unsubscribe from this list please visit:

    http://xircles.codehaus.org/manage_email

[Chris vanBuskirk]

I have had this problem also.  I always just use deleteOnExit() instead (but that isn't a proper fix ;)

 

---

From: joerg@... [mailto:joerg@...]
Sent: Saturday, May 19, 2007 6:03 AM
To: user@...
Subject: [groovy-user] File-Iterator leaves Reader open

Hi,

there seems to be a problem with iterators on files not closing their readers. consider this script: 

f = new File('ExampleIterator.txt')
f << 'This is the first line\n'
f << 'This is the second line\n'
f.each { println it }
deleted = f.delete()
println "File is deleted: $deleted" 

prints: 

This is the first line
This is the second line
File is deleted: false 

Obviously, the f.delete() does not work because the file is not closed. When I enclose the each()-call in f.withReader{…} the file will be closed properly. 

Or did I miss something?

Jörg Staudemeyer

[Jörg Staudemeyer]

Hi Chris,
the correct and durable solution is to use the specialized iterative methods with files, streams, and readers, and no each() and for-loops. These methods, eachLine(), eachByte etc., close their underlying ressources properly and even make your code more expressive.
Jörg

2007/5/22, Chris vanBuskirk <Chris.vanBuskirk@...>:

I have had this problem also.  I always just use deleteOnExit() instead (but that isn't a proper fix ;)

 

---

From: joerg@... [mailto:joerg@...]
Sent: Saturday, May 19, 2007 6:03 AM
To: user@...
Subject: [groovy-user] File-Iterator leaves Reader open

Hi,

there seems to be a problem with iterators on files not closing their readers. consider this script: 

f = new File('ExampleIterator.txt' )
f << 'This is the first line\n'
f << 'This is the second line\n'
f. each { println it }
deleted = f.delete()
println "File is deleted: $deleted" 

prints: 

This is the first line
This is the second line
File is deleted: false 

Obviously, the f.delete() does not work because the file is not closed. When I enclose the each()-call in f.withReader{…} the file will be closed properly. 

Or did I miss something?

Jörg Staudemeyer

[Jörg Staudemeyer]

Hi Jochen

2007/5/22, Jochen Theodorou <blackdrag@...>:

Jörg Staudemeyer schrieb:

> 2007/5/22, Jochen Theodorou <blackdrag@... <mailto:blackdrag@...>>:
>
>     Jörg Staudemeyer schrieb:
>      >
>      > 2007/5/21, Jochen Theodorou <blackdrag@...
>     <mailto:blackdrag@...> <mailto: blackdrag@...
>     <mailto:blackdrag@...>>>:
>     [...]
>      > If you agree, I will add a remark to GROOVY-1904 according to your
>      > (Blackdrag's) statement and than that's the way it goes.
>
>     that is make File#iterator throw an DeprecatedException and let the
>     iteration method handle the closing? Then yes, I agree.
>
>
> OK. wrote a quite comprehensive comment stating all details that have to
> be changed. Implemention won't be challenging, I suppose.
>
> Since I'm relatively new to all this - what is the further procedure?

If you want to, you can write a patch and attach it to the issue.

Wrote a patch and attached it to the issue. Also an updated testcase in Groovy.
- Jörg