Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Process is preventing itself from copying files #80

Closed
tm-ms opened this issue Feb 19, 2020 · 7 comments
Closed

Process is preventing itself from copying files #80

tm-ms opened this issue Feb 19, 2020 · 7 comments
Assignees

Comments

@tm-ms
Copy link
Contributor

tm-ms commented Feb 19, 2020

JDK moronity strikes again. The following code snippet:

Path source = XIO.Path("source.txt");
Path target = XIO.Path("target.bak");
StorageLockedFile slf = StorageLockedFile.openLockedFile(source);
System.out.println("LockedFile: " + slf);
Files.copy(source, target);	

Causes the following exception with a completely wrong and useless message:
source.txt -> target.bak: Der Prozess kann nicht auf die Datei zugreifen, da ein anderer Prozess einen Teil der Datei gesperrt hat.
(I won't translate that here and showing system messages in a localized form is another moronity in itself, but whatever)

It seems that the recent switch from File to Path and the associated change from opening a FileChannel via an InputStream to the actually more modern and supposedly better FileChannel#open method caused this behavior.

Maybe Files#copy doesn't work if there is an open FileChannel oder an existing FileLock or whatever.
Or it need a "Create target file if it does not exist, yet"-option.
Whatever the reason is, it is definitely NOT that another process has locked any of the two files involved, because there is no such process.

If someone needs me, I'll be knee-deep in researching JDK idiocy once again...

@tm-ms tm-ms self-assigned this Feb 19, 2020
@tm-ms
Copy link
Contributor Author

tm-ms commented Feb 19, 2020

test
"You can't comment at this time."

@tm-ms
Copy link
Contributor Author

tm-ms commented Feb 20, 2020

Findings:

  • Having a FileChannel open is no problem.
  • But having a Lock created on the source file causes the exception.

So the locking mechanism is NOT about processes having locks that prevent other processes from using (even reading) a file.
It's not even about threads having locks.
It's actually just about if there EXISTS ANY lock on a file (a FileLock instance).

The JavaDoc of the moronic FileLock class (hardly any JavaDoc, hardly any useful methods, two methods that do the same, etc) talks about "Other programs", meaning processes (in professional terms, but whatever). This means that if it prevents the same process, the same THREAD even, from using the file without any mentioning, this is a bug. Either in botchy Reinhold's code itself or in the windows implementation (that maybe he wrote, too).

That also explains the plain simply wrong exception message: they (he?) just wrote buggy code and the exception is the result visible to the calling context.

So far regarding flaming JDK idiocy for about the 100th time.
Now how about fixing it?
In other words: "java copy locked file".
Google result:
Nothing.
Hardly anything and false positives at best.
One hit comes pretty close, but in the end, it's a false positive as well and it uses streams instead of FileChannels directly.
https://stackoverflow.com/questions/20471374/copying-files-with-file-locks-in-java

I don't want to go back to streams because they are "old", NIO is usally more efficient/fast and I read somewhere that creating a lock on a stream-based FileChannel only locks the stream and not the actual file. Which, of course, is another JDK idiocy and explains why other windows tools (like hexeditor) didn't give a sh*t about the lock I created via stream-based FileChannels back when initially developing and testing the storage file accessing.
So "new" FileChannels it has to be.

And that means, once again: to get a proper solution, the basic JDK functionality has to be replaced by a self-written solution.
I won't start using apache utils or guava stuff or whatever, because a) that would create unwanted huge dependencies for a tiny functionality and b) the idiocy there is usually similar to the JDK, just not as high.

@tm-ms
Copy link
Contributor Author

tm-ms commented Feb 20, 2020

ZJ suggested to just copy the method from Apache's FileUtils class to avoid the dependency for just one method. That seems to be a good idea.

@tm-ms
Copy link
Contributor Author

tm-ms commented Feb 20, 2020

I am not 100% sure if this is the most recent version of apache-io, but the source code for FileUtils#doCopyFile (the code that does the actual copying) just calls the same JDK method that the MicroStream framework currently calls, Files#copy
https://github.com/apache/commons-io/blob/master/src/main/java/org/apache/commons/io/FileUtils.java#L1293
Also, the rest of the code is exactely as doubtful as suspected:

  • preserveFileDate has a weird logic and according to its own JavaDoc is not reliable and not even queryable
  • they have wird file length checks that they themselves doubt

Does copying a locked source file really work with this? It's exactely the same plus weird extras.

@tm-ms tm-ms assigned zdenek-jonas and unassigned tm-ms Feb 20, 2020
@tm-ms
Copy link
Contributor Author

tm-ms commented Feb 20, 2020

The linked source code is not the current version.
In the current version (2.6), the method looks like this:

private static void doCopyFile(final File srcFile, final File destFile, final boolean preserveFileDate)
        throws IOException {
    if (destFile.exists() && destFile.isDirectory()) {
        throw new IOException("Destination '" + destFile + "' exists but is a directory");
    }
    try (FileInputStream fis = new FileInputStream(srcFile);
         FileChannel input = fis.getChannel();
         FileOutputStream fos = new FileOutputStream(destFile);
         FileChannel output = fos.getChannel()) {
        final long size = input.size(); // TODO See IO-386
        long pos = 0;
        long count = 0;
        while (pos < size) {
            final long remain = size - pos;
            count = remain > FILE_COPY_BUFFER_SIZE ? FILE_COPY_BUFFER_SIZE : remain;
            final long bytesCopied = output.transferFrom(input, pos, count);
            if (bytesCopied == 0) { // IO-385 - can happen if file is truncated after caching the size
                break; // ensure we don't loop forever
            }
            pos += bytesCopied;
        }
    }
    final long srcLen = srcFile.length(); // TODO See IO-386
    final long dstLen = destFile.length(); // TODO See IO-386
    if (srcLen != dstLen) {
        throw new IOException("Failed to copy full contents from '" +
                srcFile + "' to '" + destFile + "' Expected length: " + srcLen + " Actual: " + dstLen);
    }
    if (preserveFileDate) {
        destFile.setLastModified(srcFile.lastModified());
    }
}

This would either have to be overhauled a little or replaced by direct FileChannel opening without the streams detour. Except if that suffers from the bug, too, then streams as a fallback.

@tm-ms tm-ms assigned tm-ms and unassigned zdenek-jonas Feb 20, 2020
@tm-ms
Copy link
Contributor Author

tm-ms commented Feb 20, 2020

I tested around a little and found that the apache code is too weird to be used:

  • It uses old streams.
  • It does way too much logic in a hardcoded way that actually the calling context should care about.
  • It does the copying way too manually with its loop and corner case handling and all that.

All that is required for a working replacement for Files#copy is:

  • create a readable source FileChannel and a writeable target FileChannel
  • call either FileChannel#transferFrom or FileChannel#transferTo.

That's like 4 line of code, INCLUDING resource handling.

(As stated above, whereever I look, I only see weird, botchy, half-baked code that should better not be used).

So I extended the XIO utility class with a copyFile(Path, Path, OpenOption...) method.
Including complete JavaDoc.

In the simplest case, it suffices to call
XIO.copyFile(source, target);
This works even with an existing file lock,

If ensuring the target file or other special case handling is required, calls like
XIO.copyFile(source, XIO.ensureDirectoryAndFile(target));
or
XIO.copyFile(source, target, StandardOpenOption.CREATE, StandardOpenOption.TRUNCATE_EXISTING);
can be used.

I will replace all occurances of the buggy JDK Files#copy method with calls to the new method.

@tm-ms
Copy link
Contributor Author

tm-ms commented Feb 20, 2020

All occurances of JDK Files#copy have been replaced with XIO#copyFile.

@tm-ms tm-ms closed this as completed Feb 20, 2020
@fh-ms fh-ms transferred this issue from another repository Apr 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants