Details

    • Type: Bug Bug
    • Status: Open Open
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: JRuby 1.5.1
    • Fix Version/s: JRuby 1.6
    • Component/s: Core Classes/Modules
    • Labels:
      None
    • Environment:
      Windows
    • Number of attachments :
      0

      Description

      I can't move a folder onto a UNC path. So a

      mv src,dest, :verbose => true, :force => true

      where dest is a windows share doesn't do anything. Mapping the share doesn't help, and mounting the share in linux doesn't do anything either. It doesn't throw any errors, but no files are moved. I can reproduce this easy enough with a gem generate_index -d <share_name> where <share_name> can be a UNC path, a mounted drive pointing to a windows share, or a linux mount point which mounts the windows share.

      The RubyFile.java class does throw the "throw runtime.newErrnoEACCESError(oldNameJavaString + " or " + newNameJavaString);" exception, but FuleUtils.rb seems to ignore it.

      This could be related to http://jira.codehaus.org/browse/JRUBY-2724

        Issue Links

          Activity

          Hide
          George added a comment -

          The issue appears to be creating the folder on a windows share. I've been doing some digging, and the RubyDir.java mkdir function does correctly throw the exception, but FileUtils.mv doesn't catch it correctly. In mv the Rename exception is caught, but in the rescue a copy then delete is tried. The copy also fails, with an exception, but since it is never caught, it fails silently. This is all the info I have on the exception part, as to why it can't create the folder, I'm still not sure, but I think it's in the getPosix().mkdir call.

          Show
          George added a comment - The issue appears to be creating the folder on a windows share. I've been doing some digging, and the RubyDir.java mkdir function does correctly throw the exception, but FileUtils.mv doesn't catch it correctly. In mv the Rename exception is caught, but in the rescue a copy then delete is tried. The copy also fails, with an exception, but since it is never caught, it fails silently. This is all the info I have on the exception part, as to why it can't create the folder, I'm still not sure, but I think it's in the getPosix().mkdir call.
          Hide
          Jay McGaffigan added a comment - - edited

          I've sent this to wayne meissner but the problem is in the jnr-posix JavaSecuredFile class. It uses java.io.File#renameTo which is pretty fragile in the face of windows filesystem to filesystem moves.

          I've sent the following patch to Wayne. It preserves teh renameTo semantics but if that fails it uses a FileChannel style copy.

          index f8abb6b..1bd1a80 100644
          --- a/src/org/jruby/ext/posix/JavaSecuredFile.java
          +++ b/src/org/jruby/ext/posix/JavaSecuredFile.java
          @@ -32,6 +32,10 @@ import java.io.FileFilter;
           import java.io.FilenameFilter;
           import java.io.IOException;
           import java.net.URI;
          +import java.io.FileInputStream;
          +import java.io.FileOutputStream;
          +import java.nio.channels.FileChannel;
          +import java.io.Closeable;
           
           /**
            * <p>This file catches any SecurityExceptions raised when access
          @@ -251,7 +255,10 @@ public class JavaSecuredFile extends File {
               @Override
               public boolean renameTo(File dest) {
                   try {
          -            return super.renameTo(dest);
          +            if (super.renameTo(dest)) {
          +                return true;
          +            }            
          +            return moveIt(this, dest);
                   } catch (SecurityException ex) {
                       return false;
                   }
          @@ -274,5 +281,166 @@ public class JavaSecuredFile extends File {
                       return false;
                   }
               }
          +    
          +    
          +    /** Move a File
          +    * The renameTo method does not allow action across NFS mounted filesystems
          +    * this method is the workaround
          +    *
          +    * @param fromFile The existing File
          +    * @param toFile The new File
          +    * @return  <code>true</code> if and only if the renaming succeeded;
          +    *          <code>false</code> otherwise
          +    */
          +    private static final boolean moveIt(File fromFile, File toFile) {
          +
          +        // delete if copy was successful, otherwise move will fail
          +        if (copyIt(fromFile, toFile)) {
          +            return removeIt(fromFile);
          +        }
          +        return false;
          +    }
          +    /** Copy a Directory
          +    * if renameTo has failed and our source is a directory we have to start delving into
          +    * copying all the directories.
          +    * One thing to note is that I need to "preserve" the mv semantics.  This means:
          +    *  if the destination is a directory that exists we add the source directory under it.
          +    *    so: mv stuff /Volumes/share  would become /Volumes/share/stuff  if /Volumes/share exists
          +    *  if the distination directory does not exist.  We create that and add all the files in the 
          +    *    source directory UNDER this new directory.
          +    *    so: mv stuff /Volumes/share/stuff3 would put the contents of stuff into stuff3 if stuff3 does not exist.
          +    * 
          +    * @param fromDirectory the existing directory
          +    * @param toDirectory  the destination directory
          +    * @return <code>true</code> if the copy succeeded
          +    *         <code>false</code> if there was a failure
          +    * @throws IOException if we encounter issues
          +    */    
          +    private static final boolean copyDirectory (File fromDirectory, File toDirectory) throws IOException {
          +        File destDirectory = null;
          +        if (toDirectory.exists()) {
          +            destDirectory = new File(toDirectory.getPath(), fromDirectory.getName());
          +        } else {
          +            destDirectory = toDirectory;
          +        }
          +        
          +        if (destDirectory.mkdirs() == false){
          +            throw new IOException("Could not create destination directory");
          +        }
          +        
          +        File[] files = fromDirectory.listFiles();
          +        boolean success = false;
          +        for (File file:files){
          +            File toFile = new File(destDirectory, file.getName());
          +            if (file.isDirectory()) {
          +                copyDirectory(file, toFile);
          +            } else {
          +                if (copyFile(file, toFile) == false) {
          +                    throw new IOException("Could not copy file");
          +                }
          +            }
          +        }
          +        return true;
          +    }
          +
          +    /** Copy a File
          +    * The renameTo method does not allow action across NFS mounted filesystems
          +    * this method is the workaround
          +    *
          +    * @param fromFile The existing File
          +    * @param toFile The new File
          +    * @return  <code>true</code> if and only if the renaming succeeded;
          +    *          <code>false</code> otherwise
          +    */
          +    private static final boolean copyFile (File fromFile, File toFile) {
          +        FileInputStream in = null;
          +        FileOutputStream out = null;
          +        try {
          +            in = new FileInputStream(fromFile);
          +            out = new FileOutputStream(toFile);
          +            
          +
          +            FileChannel src = in.getChannel();
          +            FileChannel dst = out.getChannel();
          +
          +            long pos = 0;
          +            long transmitted = -1;
          +            //Transfer bytes until we get 0. (need to cover the last chunk)
          +            while (transmitted != 0) {
          +                transmitted = src.transferTo(pos, 8192, dst);
          +                pos += transmitted;
          +            }
          +            
          +            // cleanupif files are not the same length
          +            if (fromFile.length() != toFile.length()) {
          +                toFile.delete();
          +                return false;
          +            }
          +
          +            return true;
          +        } catch (IOException e) {
          +            return false;
          +        } finally {
          +            closeIt(out);               
          +            closeIt(in);            
          +        }
          +    }
          +    
          +    private static final void closeIt(Closeable closeable){
          +        try {
          +            if (closeable != null) {
          +                closeable.close();
          +            }
          +        } catch (IOException e) {
          +            //Do nothing
          +        }
          +    }
          +
          +    /**
          +    * copy a file check if directories are involved and handle appropriately
          +    * 
          +    * @param fromFile from 
          +    * @param toFile to
          +    * @return <code>true</code> success
          +    *         <code>false</code> failure somewhere
          +    */
          +    private static final boolean copyIt(File fromFile, File toFile) {
          +        try {
          +            if (fromFile.isDirectory()) {
          +                return copyDirectory(fromFile, toFile);
          +            } else {
          +                return copyFile(fromFile, toFile);
          +            }            
          +        } catch (IOException e) {
          +            return false;
          +        }
          +    }
          +    
          +    /** Remove a file.  If it's a directory recursively remove it
          +    *
          +    * @param fromFile
          +    * @return <code>true</code> success
          +    *         <code>false</code> could not remove files
          +    */
          +    private static final boolean removeIt(File fromFile){
          +        boolean success = false;
          +        if (fromFile.isDirectory()) {
          +            File[] files = fromFile.listFiles();
          +            for (File file : files){
          +                if (file.isDirectory()) {
          +                    success = removeIt(file);
          +                } else {
          +                    success = file.delete();
          +                }
          +                if (success == false) {
          +                    //Break out of the loop on failure means we don't need to go on.
          +                    return false;
          +                }
          +            }
          +        }
          +        //AFter deleting all the children we can delete this one.
          +        return fromFile.delete(); 
          +    }
          +
           }
          
           
          
          Show
          Jay McGaffigan added a comment - - edited I've sent this to wayne meissner but the problem is in the jnr-posix JavaSecuredFile class. It uses java.io.File#renameTo which is pretty fragile in the face of windows filesystem to filesystem moves. I've sent the following patch to Wayne. It preserves teh renameTo semantics but if that fails it uses a FileChannel style copy. index f8abb6b..1bd1a80 100644 --- a/src/org/jruby/ext/posix/JavaSecuredFile.java +++ b/src/org/jruby/ext/posix/JavaSecuredFile.java @@ -32,6 +32,10 @@ import java.io.FileFilter; import java.io.FilenameFilter; import java.io.IOException; import java.net.URI; +import java.io.FileInputStream; +import java.io.FileOutputStream; +import java.nio.channels.FileChannel; +import java.io.Closeable; /** * <p>This file catches any SecurityExceptions raised when access @@ -251,7 +255,10 @@ public class JavaSecuredFile extends File { @Override public boolean renameTo(File dest) { try { - return super.renameTo(dest); + if (super.renameTo(dest)) { + return true; + } + return moveIt(this, dest); } catch (SecurityException ex) { return false; } @@ -274,5 +281,166 @@ public class JavaSecuredFile extends File { return false; } } + + + /** Move a File + * The renameTo method does not allow action across NFS mounted filesystems + * this method is the workaround + * + * @param fromFile The existing File + * @param toFile The new File + * @return <code>true</code> if and only if the renaming succeeded; + * <code>false</code> otherwise + */ + private static final boolean moveIt(File fromFile, File toFile) { + + // delete if copy was successful, otherwise move will fail + if (copyIt(fromFile, toFile)) { + return removeIt(fromFile); + } + return false; + } + /** Copy a Directory + * if renameTo has failed and our source is a directory we have to start delving into + * copying all the directories. + * One thing to note is that I need to "preserve" the mv semantics. This means: + * if the destination is a directory that exists we add the source directory under it. + * so: mv stuff /Volumes/share would become /Volumes/share/stuff if /Volumes/share exists + * if the distination directory does not exist. We create that and add all the files in the + * source directory UNDER this new directory. + * so: mv stuff /Volumes/share/stuff3 would put the contents of stuff into stuff3 if stuff3 does not exist. + * + * @param fromDirectory the existing directory + * @param toDirectory the destination directory + * @return <code>true</code> if the copy succeeded + * <code>false</code> if there was a failure + * @throws IOException if we encounter issues + */ + private static final boolean copyDirectory (File fromDirectory, File toDirectory) throws IOException { + File destDirectory = null; + if (toDirectory.exists()) { + destDirectory = new File(toDirectory.getPath(), fromDirectory.getName()); + } else { + destDirectory = toDirectory; + } + + if (destDirectory.mkdirs() == false){ + throw new IOException("Could not create destination directory"); + } + + File[] files = fromDirectory.listFiles(); + boolean success = false; + for (File file:files){ + File toFile = new File(destDirectory, file.getName()); + if (file.isDirectory()) { + copyDirectory(file, toFile); + } else { + if (copyFile(file, toFile) == false) { + throw new IOException("Could not copy file"); + } + } + } + return true; + } + + /** Copy a File + * The renameTo method does not allow action across NFS mounted filesystems + * this method is the workaround + * + * @param fromFile The existing File + * @param toFile The new File + * @return <code>true</code> if and only if the renaming succeeded; + * <code>false</code> otherwise + */ + private static final boolean copyFile (File fromFile, File toFile) { + FileInputStream in = null; + FileOutputStream out = null; + try { + in = new FileInputStream(fromFile); + out = new FileOutputStream(toFile); + + + FileChannel src = in.getChannel(); + FileChannel dst = out.getChannel(); + + long pos = 0; + long transmitted = -1; + //Transfer bytes until we get 0. (need to cover the last chunk) + while (transmitted != 0) { + transmitted = src.transferTo(pos, 8192, dst); + pos += transmitted; + } + + // cleanupif files are not the same length + if (fromFile.length() != toFile.length()) { + toFile.delete(); + return false; + } + + return true; + } catch (IOException e) { + return false; + } finally { + closeIt(out); + closeIt(in); + } + } + + private static final void closeIt(Closeable closeable){ + try { + if (closeable != null) { + closeable.close(); + } + } catch (IOException e) { + //Do nothing + } + } + + /** + * copy a file check if directories are involved and handle appropriately + * + * @param fromFile from + * @param toFile to + * @return <code>true</code> success + * <code>false</code> failure somewhere + */ + private static final boolean copyIt(File fromFile, File toFile) { + try { + if (fromFile.isDirectory()) { + return copyDirectory(fromFile, toFile); + } else { + return copyFile(fromFile, toFile); + } + } catch (IOException e) { + return false; + } + } + + /** Remove a file. If it's a directory recursively remove it + * + * @param fromFile + * @return <code>true</code> success + * <code>false</code> could not remove files + */ + private static final boolean removeIt(File fromFile){ + boolean success = false; + if (fromFile.isDirectory()) { + File[] files = fromFile.listFiles(); + for (File file : files){ + if (file.isDirectory()) { + success = removeIt(file); + } else { + success = file.delete(); + } + if (success == false) { + //Break out of the loop on failure means we don't need to go on. + return false; + } + } + } + //AFter deleting all the children we can delete this one. + return fromFile.delete(); + } + }
          Hide
          Charles Oliver Nutter added a comment -

          Patch provided, but requires updating jnr-posix release. Marking for 1.6.

          Show
          Charles Oliver Nutter added a comment - Patch provided, but requires updating jnr-posix release. Marking for 1.6.
          Hide
          Jay McGaffigan added a comment -

          I had to update the patch to support directories too

          Show
          Jay McGaffigan added a comment - I had to update the patch to support directories too
          Hide
          Hiro Asari added a comment -

          Jay,

          Can you attach your patch as a git patch? git diff --format-patch

          That way we can give you the credit for the fix.

          Show
          Hiro Asari added a comment - Jay, Can you attach your patch as a git patch? git diff --format-patch That way we can give you the credit for the fix.
          Hide
          Charles Oliver Nutter added a comment -

          Getting a bit late in 1.6 cycle for this. Has it been integrated in the offending library?

          Show
          Charles Oliver Nutter added a comment - Getting a bit late in 1.6 cycle for this. Has it been integrated in the offending library?
          Hide
          Thomas E Enebo added a comment -

          Hoping to land this when I land real Windows exec behavior (hoping for 1.6.0 still).

          Show
          Thomas E Enebo added a comment - Hoping to land this when I land real Windows exec behavior (hoping for 1.6.0 still).
          Hide
          George added a comment - - edited

          Not sure if this was fixed using Jay's patch, or http://jira.codehaus.org/browse/JRUBY-2724 fixed it, but it appears to be fixed in 1.7.2

          Show
          George added a comment - - edited Not sure if this was fixed using Jay's patch, or http://jira.codehaus.org/browse/JRUBY-2724 fixed it, but it appears to be fixed in 1.7.2
          Hide
          George added a comment -

          Correction, the move seems fixed, but a mkdir -p still fails. Using Jay's patch, I can still get this to work, but 1.7.2 hasn't fixed it.

          Show
          George added a comment - Correction, the move seems fixed, but a mkdir -p still fails. Using Jay's patch, I can still get this to work, but 1.7.2 hasn't fixed it.

            People

            • Assignee:
              Unassigned
              Reporter:
              George
            • Votes:
              1 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated: