Camel
  1. Camel
  2. CAMEL-5235

Make sure to close file input stream when converting file to string to avoid locking file on windows

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.9.2
    • Fix Version/s: 2.9.3, 2.10.0
    • Component/s: camel-core
    • Labels:
      None
    • Estimated Complexity:
      Unknown

      Description

      See nabble
      http://camel.465427.n5.nabble.com/File-Processor-Not-deleting-the-files-tp5670301.html

      Need to explicit close the file input/output streams to avoid the files to be locked on windows.

        Activity

        Hide
        Babak Vahdat added a comment -

        Claus, one stupid question of mine:

        Why don't we simply remove these two static member classes EncodingFileReader as well as EncodingFileWriter and just make use of JDK's own InputStreamReader & OutputStreamWriter directly? To my understanding these two classes provide no added value other than trouble. And the JDK's own bridges (byte streams to character streams) are already encoding aware anyway!

        So the two usage methods would simply become:

            public static BufferedReader toReader(File file, String charset) throws IOException {
                FileInputStream in = new FileInputStream(file);
                return IOHelper.buffered(new InputStreamReader(in, charset));
            }
        
            public static BufferedWriter toWriter(File file, boolean append, String charset) throws IOException {
                FileOutputStream os = new FileOutputStream(file, append);
                return IOHelper.buffered(new OutputStreamWriter(os, charset));
            }
        

        They were both introduced by CAMEL-2056.

        Show
        Babak Vahdat added a comment - Claus, one stupid question of mine: Why don't we simply remove these two static member classes EncodingFileReader as well as EncodingFileWriter and just make use of JDK's own InputStreamReader & OutputStreamWriter directly? To my understanding these two classes provide no added value other than trouble. And the JDK's own bridges (byte streams to character streams) are already encoding aware anyway! So the two usage methods would simply become: public static BufferedReader toReader(File file, String charset) throws IOException { FileInputStream in = new FileInputStream(file); return IOHelper.buffered( new InputStreamReader(in, charset)); } public static BufferedWriter toWriter(File file, boolean append, String charset) throws IOException { FileOutputStream os = new FileOutputStream(file, append); return IOHelper.buffered( new OutputStreamWriter(os, charset)); } They were both introduced by CAMEL-2056 .
        Hide
        Claus Ibsen added a comment -

        Maybe but dont you still have the the issue that the File input/output streams do not get closed when the InputStreamWriter/OutputStreamWriter gets closed.

        With the current code we explicit close those now.

        Show
        Claus Ibsen added a comment - Maybe but dont you still have the the issue that the File input/output streams do not get closed when the InputStreamWriter/OutputStreamWriter gets closed. With the current code we explicit close those now.
        Hide
        Claus Ibsen added a comment -

        You are welcome to do some investigations and see if we can do as you suggests, but we need to make sure the file streams gets closed, to avoid locking issues with windows.

        Show
        Claus Ibsen added a comment - You are welcome to do some investigations and see if we can do as you suggests, but we need to make sure the file streams gets closed, to avoid locking issues with windows.
        Hide
        Babak Vahdat added a comment -

        Maybe but dont you still have the the issue that the File input/output streams do not get closed when the InputStreamWriter/OutputStreamWriter gets closed.

        No as that's done for "free" through the close() method magic, that's the call-chaining of the close() call by the underlying input/output streams. As an example when we look at BufferedReader.close() method we see that the close() call get's chained and the underlying stream (hopefully) does properly chain the call again and again, until the OS native stream gets properly closed. But that's the user's responsibility to
        do that:

        BufferedReader myReader = CamelContext.getTypeConverter().convertTo(BufferedReader.class, ...);
        …
        …
        myReader.close(); // user should close the stream properly after the usage
        

        The main "problem" is that we can't control if the user do properly close the Reader, Writer, In and Outputstream after the usage. And if they don't, then they will run into trouble and think something in Camel could be the reason for that.

        Show
        Babak Vahdat added a comment - Maybe but dont you still have the the issue that the File input/output streams do not get closed when the InputStreamWriter/OutputStreamWriter gets closed. No as that's done for "free" through the close() method magic, that's the call-chaining of the close() call by the underlying input/output streams. As an example when we look at BufferedReader.close() method we see that the close() call get's chained and the underlying stream (hopefully) does properly chain the call again and again, until the OS native stream gets properly closed. But that's the user's responsibility to do that: BufferedReader myReader = CamelContext.getTypeConverter().convertTo(BufferedReader.class, ...); … … myReader.close(); // user should close the stream properly after the usage The main "problem" is that we can't control if the user do properly close the Reader, Writer, In and Outputstream after the usage. And if they don't, then they will run into trouble and think something in Camel could be the reason for that.
        Hide
        Claus Ibsen added a comment -

        Yeah if the user gets a reader/stream etc then he may need to close it after use.

        But in this use-case it was converting to String or byte[] etc directly. And for that Camel must make sure to close the stream internally.

        Show
        Claus Ibsen added a comment - Yeah if the user gets a reader/stream etc then he may need to close it after use. But in this use-case it was converting to String or byte[] etc directly. And for that Camel must make sure to close the stream internally.
        Hide
        Babak Vahdat added a comment - - edited

        While trying to find some resources on the web describing what my poor english is talking about, I came along many of them but the best one was given through that already dissappered real vendor itself:

        Similarly, when closing chained streams, you only need to close the outermost stream class because the close() call is automatically trickled through all the chained classes; in the example above, you would simply call the close() method on the GZIPOutputStream class.

        When you look at the example there the inner most OutputStream is also a FileOutputStream, however there's no need of an explicit close call on that:

        http://java.sun.com/developer/technicalArticles/Streams/ProgIOStreams/

        There are plenty of other APIs having similar semantics (call-chaining) like when you close a JDBC Statement:

        Note:When a Statement object is closed, its current ResultSet object, if one exists, is also closed.

        http://docs.oracle.com/javase/6/docs/api/java/sql/Statement.html#close()

        So why I still ask for your approval to get rid of those two obsolete static member classes.

        Show
        Babak Vahdat added a comment - - edited While trying to find some resources on the web describing what my poor english is talking about, I came along many of them but the best one was given through that already dissappered real vendor itself: Similarly, when closing chained streams, you only need to close the outermost stream class because the close() call is automatically trickled through all the chained classes; in the example above, you would simply call the close() method on the GZIPOutputStream class. When you look at the example there the inner most OutputStream is also a FileOutputStream, however there's no need of an explicit close call on that: http://java.sun.com/developer/technicalArticles/Streams/ProgIOStreams/ There are plenty of other APIs having similar semantics (call-chaining) like when you close a JDBC Statement: Note:When a Statement object is closed, its current ResultSet object, if one exists, is also closed. http://docs.oracle.com/javase/6/docs/api/java/sql/Statement.html#close( ) So why I still ask for your approval to get rid of those two obsolete static member classes.
        Hide
        Claus Ibsen added a comment -

        Yes they can possible be removed, just make sure the FileInputStream gets closed, as otherwise it can lock file handles on Windows.

        Show
        Claus Ibsen added a comment - Yes they can possible be removed, just make sure the FileInputStream gets closed, as otherwise it can lock file handles on Windows.

          People

          • Assignee:
            Claus Ibsen
            Reporter:
            Claus Ibsen
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development