Issue Details (XML | Word | Printable)

Key: XADISK-98
Type: Bug Bug
Status: Resolved Resolved
Resolution: Fixed
Priority: Minor Minor
Assignee: Nitin Verma
Reporter: romaerzhuk
Votes: 0
Watchers: 0
Operations

If you were logged in you would be able to see more operations.
xadisk

Safe close FileInput/OutputStreams

Created: 07/Jan/12 09:09 AM   Updated: 27/Aug/13 07:22 AM   Resolved: 19/Aug/13 06:14 PM
Component/s: None
Affects Version/s: 1.2
Fix Version/s: 1.2.2

Time Tracking:
Not Specified

File Attachments: 1. Text File Safe-close-streams.patch (13 kB) 07/Jan/12 05:16 PM - romaerzhuk


Tags:
Participants: Nitin Verma and romaerzhuk


 Description  « Hide

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);
        }
      }
    }


romaerzhuk added a comment - 07/Jan/12 05:16 PM

This patch solves the issue


Nitin Verma added a comment - 21/Jan/12 01:53 PM

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 added a comment - 19/Aug/13 06:14 PM

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