groovy

Iterators do not close streams

Details

  • Type: Bug Bug
  • Status: Closed Closed
  • Priority: Major Major
  • Resolution: Fixed
  • Affects Version/s: 1.0, 1.1-beta-1
  • Fix Version/s: 1.1-beta-2
  • Component/s: groovy-jdk
  • Labels:
    None
  • Testcase included:
    yes
  • Number of attachments :
    3

Description

iterator() methods for File, Reader, InputStream, and DataInputStream return iterators that do not close their underlying streams. They should do this, to be consistent with iterative closure methods like eachLine() etc. A solution must take in account that a loop can be broken by an exception inside the loop, that is not caused by a resource read operation. Also in this case the resource must be closed.

See discussion at http://www.nabble.com/File-Iterator-leaves-Reader-open-tf3782079.html.

To clarify problem a JUnit test case will is attached.

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Jörg Staudemeyer added a comment -

According to the results of the mentioned discussion following modifications shall be applied to DefaultGroovyMethods:

  • New overloaded each() methods on File, Reader, InputStream, and ObjectInputStream close their resources on EOF and when an exception occurs. In some cases they may simple delegate to specialized each-methods (like eachLine()).
  • The iterator() methods on Reader, InputStream, and ObjectInputStream return iterators that close their underlying resource on EOF or IOException.
  • The iterator() method on File throws an groovy.lang.DeprecationException stating "Iterators on files are not supported any more. Use File.eachLine() instead. Alternatively you can use FileReader.iterator() and provide your own exception handling."

This results in the following consequences for application programmers:

  • each() loops on streams and readers can be used safely, streams and readers will be closed, no matter what happens inside the loops.
  • For-loops on streams and readers close their streams and readers on normal execution but not necessarily when an exception occurs. So such loops must always be enclosed in try blocks with a finally block that closes the resource.
  • For-loops on files do not work anymore, since there is no way to handle exceptions. They can be easily replaced by File.each() or - more expressive - by File.eachLine().

All test methods that relate to handling of exceptions inside the loop can be removed from the attached JUnit file.

Show
Jörg Staudemeyer added a comment - According to the results of the mentioned discussion following modifications shall be applied to DefaultGroovyMethods:
  • New overloaded each() methods on File, Reader, InputStream, and ObjectInputStream close their resources on EOF and when an exception occurs. In some cases they may simple delegate to specialized each-methods (like eachLine()).
  • The iterator() methods on Reader, InputStream, and ObjectInputStream return iterators that close their underlying resource on EOF or IOException.
  • The iterator() method on File throws an groovy.lang.DeprecationException stating "Iterators on files are not supported any more. Use File.eachLine() instead. Alternatively you can use FileReader.iterator() and provide your own exception handling."
This results in the following consequences for application programmers:
  • each() loops on streams and readers can be used safely, streams and readers will be closed, no matter what happens inside the loops.
  • For-loops on streams and readers close their streams and readers on normal execution but not necessarily when an exception occurs. So such loops must always be enclosed in try blocks with a finally block that closes the resource.
  • For-loops on files do not work anymore, since there is no way to handle exceptions. They can be easily replaced by File.each() or - more expressive - by File.eachLine().
All test methods that relate to handling of exceptions inside the loop can be removed from the attached JUnit file.
Hide
Permalink
Jörg Staudemeyer added a comment -

Attached patch file for DefaultGroovyMethods. Further directives are written directly to the file.

Attached also an updated unit testcase reflecting the actual requirements.

Show
Jörg Staudemeyer added a comment - Attached patch file for DefaultGroovyMethods. Further directives are written directly to the file. Attached also an updated unit testcase reflecting the actual requirements.
Hide
Permalink
blackdrag blackdrag added a comment -

I close this issue now because I have deprecated File#iterator.

The solution with the Iterator was discussed, but not approved... it is a pattern too hidden from the user - it is difficult to know then if it will close or not.

I will also not change/add the "each" Methods on File, Reader and InputStream... One good example here is DataInputStream where people usually expect mixed, not only raw bytes. And using eachLine as "each" for File... We already have eachLine, eachObject and eachByte and they fit exactly the needs here.. I think it is not of much help to provide here an alias to existing methods that might do what they expect for one, but not for the other.

That means Object#each will be used which tries not to make them into arrays, lists or other collections and just iterators over a one element list containing the object we operate on. That might be not of much use, but it is also not harmful

Show
blackdrag blackdrag added a comment - I close this issue now because I have deprecated File#iterator. The solution with the Iterator was discussed, but not approved... it is a pattern too hidden from the user - it is difficult to know then if it will close or not. I will also not change/add the "each" Methods on File, Reader and InputStream... One good example here is DataInputStream where people usually expect mixed, not only raw bytes. And using eachLine as "each" for File... We already have eachLine, eachObject and eachByte and they fit exactly the needs here.. I think it is not of much help to provide here an alias to existing methods that might do what they expect for one, but not for the other. That means Object#each will be used which tries not to make them into arrays, lists or other collections and just iterators over a one element list containing the object we operate on. That might be not of much use, but it is also not harmful

People

Vote (0)
Watch (0)

Dates

  • Created:
    Updated:
    Resolved:
Atlassian JIRA (v4.4.3#663-r165197) | Report a problem
Powered by a free Atlassian JIRA open source license for Codehaus. Try JIRA - bug tracking software for your team.