xadisk
  1. xadisk
  2. XADISK-98

Safe close FileInput/OutputStreams

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 1.2
    • Fix Version/s: 1.2.2
    • Component/s: None
    • Labels:
      None

      Description

      FileIOUtility uses unsafe close operation. Any exceptions lead to leaks file descriptors, for example:

          public static void copyFile(File src, File dest, boolean append) throws IOException {
              FileChannel srcChannel = new FileInputStream(src).getChannel();
              FileChannel destChannel = new FileOutputStream(dest, append).getChannel();
      
              // Any exceptions lead to leaks file descriptors!
              long contentLength = srcChannel.size();
              long num = 0;
              while (num < contentLength) {
                  num += srcChannel.transferTo(num, NativeXAFileSystem.maxTransferToChannel(contentLength - num), destChannel);
              }
      
              destChannel.force(false);
              srcChannel.close();
              destChannel.close();
          }
      

      Correct code:

          public static void copyFile(File src, File dest, boolean append) throws IOException {
              FileInputStream srcIn = null;
              FileOutputStream destOut = null;
              try {
                srcIn = new FileInputStream(src);
                destOut = new FileOutputStream(dest, append);
                FileChannel srcChannel = srcIn.getChannel();
                FileChannel destChannel = destOut.getChannel();
                long contentLength = srcChannel.size();
                long num = 0;
                while (num < contentLength) {
                    num += srcChannel.transferTo(num, NativeXAFileSystem.maxTransferToChannel(contentLength - num), destChannel);
                }
      
                destChannel.force(false);
              } finally {
                close(srcIn);
                close(destOut);
              }
          }
          static void close(Closeable c) {
            if (c != null) {
              try {
                c.close();
              } catch (Throwable t) {
                 logger.warn("Unable to close: " + t);
              }
            }
          }
      

        Activity

        romaerzhuk created issue -
        Hide
        romaerzhuk added a comment -

        This patch solves the issue

        Show
        romaerzhuk added a comment - This patch solves the issue
        romaerzhuk made changes -
        Field Original Value New Value
        Attachment Safe-close-streams.patch [ 48865 ]
        Hide
        Nitin Verma added a comment -

        Many Thanks for identifying this, romaerzhuk. [sorry, don't know your name, I wish I did :)]

        I will fix it by the next release.

        Just wanted to add an FYI here...

        One basic premise when XADisk is used to interact with a set of files is that, (at a broad level) no other software is simultaneously working over the same set of files. If someone else is also accessing those files which are being worked on by XADisk, (say XADisk delete operation is going on), the application runs into the risk of inconsistency.

        When XADisk finds that it is not able to do certain operations (say delete a file, because someone else is writing to it), it makes a guess that someone else is simultaneously tempering with the same file, and disables itself from doing any other operation; applications will then start receiving XASystemNoMoreAvailableException.

        Now, the above copyFile code is one such place where, if XADisk receives an exception, it is very likely that someone else doing something, and with current implementation, XADisk will call it "end of the world" and will disable itself. In practice, this would require a more serious remedy by the application administrator. But I agree that doing channel.close() is always better, even in these cases.

        Thanks Again...
        Nitin

        Show
        Nitin Verma added a comment - Many Thanks for identifying this, romaerzhuk. [sorry, don't know your name, I wish I did :)] I will fix it by the next release. Just wanted to add an FYI here... One basic premise when XADisk is used to interact with a set of files is that, (at a broad level) no other software is simultaneously working over the same set of files. If someone else is also accessing those files which are being worked on by XADisk, (say XADisk delete operation is going on), the application runs into the risk of inconsistency. When XADisk finds that it is not able to do certain operations (say delete a file, because someone else is writing to it), it makes a guess that someone else is simultaneously tempering with the same file, and disables itself from doing any other operation; applications will then start receiving XASystemNoMoreAvailableException . Now, the above copyFile code is one such place where, if XADisk receives an exception, it is very likely that someone else doing something, and with current implementation, XADisk will call it "end of the world" and will disable itself. In practice, this would require a more serious remedy by the application administrator. But I agree that doing channel.close() is always better, even in these cases. Thanks Again... Nitin
        Nitin Verma made changes -
        Priority Major [ 3 ] Minor [ 4 ]
        Hide
        Nitin Verma added a comment -

        Checked-in the changes to trunk.

        Just as an additional information: the xadisk issue #76 is now fixed, and we have a mechanism to leave transactions in incomplete failed state without bringing the xadisk instance down. The user may be doing manual management of the files, which were probably worked upon inside the transaction prior to failure. So, it becomes more important to take care that we always close the fileChannels (specially to the application files operated over by the transaction, if not xadisk system transaction-logs).

        Show
        Nitin Verma added a comment - Checked-in the changes to trunk. Just as an additional information: the xadisk issue #76 is now fixed, and we have a mechanism to leave transactions in incomplete failed state without bringing the xadisk instance down. The user may be doing manual management of the files, which were probably worked upon inside the transaction prior to failure. So, it becomes more important to take care that we always close the fileChannels (specially to the application files operated over by the transaction, if not xadisk system transaction-logs).
        Nitin Verma made changes -
        Status Open [ 1 ] Resolved [ 5 ]
        Fix Version/s current [ 13583 ]
        Resolution Fixed [ 1 ]
        Nitin Verma made changes -
        Fix Version/s 1.2.2 [ 16644 ]
        Fix Version/s current [ 13583 ]

          People

          • Assignee:
            Nitin Verma
            Reporter:
            romaerzhuk
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: