search

codehaus-plexus / plexus-utils

Plexus Utils
https://codehaus-plexus.github.io/plexus-utils/
Apache License 2.0
35 stars 37 forks source link

race condition in FileUtils.copyFile() #47

Open gregallen opened 5 years ago

gregallen commented 5 years ago

in a parallel multi-module maven build, running copy-dependencies with a shared output directory...

more than one module may attempt to copy the file at once. The length check can then fail since it sees the size of the partially copied new file.

suggest using the return value from nio Files copyFile, which is the number of bytes copied. can be safely compared against the size of the source file.

also, you can surely rip out the old pre-nio code and drop java 6 support?

gregallen commented 5 years ago

also raised in Maven dependencies plugin: https://issues.apache.org/jira/browse/MDEP-633

michael-o commented 5 years ago

Can you provide a minimal viable example project for this? I'd assume even if you have one shared output dir the parallel builders build disjoint stuff.

gregallen commented 5 years ago

Just need a parent with two children Both share an ideally large dependency Build with -T2 clean dependency:copy Repeat until fail

gregallen commented 5 years ago 
$ more pom.xml */pom.xml | cat
::::::::::::::
pom.xml
::::::::::::::
<?xml version="1.0" encoding="UTF-8"?>
<project xmlns="http://maven.apache.org/POM/4.0.0"
         xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
         xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
    <modelVersion>4.0.0</modelVersion>

    <groupId>org.apache.maven</groupId>
    <artifactId>mdep-633</artifactId>
    <version>1.0-SNAPSHOT</version>
    <packaging>pom</packaging>

    <modules>
        <module>mod1</module>
        <module>mod2</module>
    </modules>

    <build>
        <plugins>
            <plugin>
                <artifactId>maven-dependency-plugin</artifactId>
                <version>3.1.1</version>
                <configuration>
                    <outputDirectory>${project.basedir}/../target</outputDirectory>
                </configuration>
            </plugin>
        </plugins>
    </build>

</project>
::::::::::::::
mod1/pom.xml
::::::::::::::
<?xml version="1.0" encoding="UTF-8"?>
<project xmlns="http://maven.apache.org/POM/4.0.0"
         xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
         xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
    <modelVersion>4.0.0</modelVersion>

    <groupId>org.apache.maven</groupId>
    <artifactId>mod1</artifactId>
    <version>1.0-SNAPSHOT</version>

    <parent>
        <groupId>org.apache.maven</groupId>
        <artifactId>mdep-633</artifactId>
        <version>1.0-SNAPSHOT</version>
    </parent>

    <dependencies>
        <dependency>
            <groupId>org.quickfixj</groupId>
            <artifactId>quickfixj-messages-all</artifactId>
            <version>2.1.0</version>
            <classifier>javadoc</classifier>
        </dependency>
    </dependencies>

</project>
::::::::::::::
mod2/pom.xml
::::::::::::::
<?xml version="1.0" encoding="UTF-8"?>
<project xmlns="http://maven.apache.org/POM/4.0.0"
         xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
         xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
    <modelVersion>4.0.0</modelVersion>

    <groupId>org.apache.maven</groupId>
    <artifactId>mod2</artifactId>
    <version>1.0-SNAPSHOT</version>

    <parent>
        <groupId>org.apache.maven</groupId>
        <artifactId>mdep-633</artifactId>
        <version>1.0-SNAPSHOT</version>
    </parent>

    <dependencies>
        <dependency>
            <groupId>org.quickfixj</groupId>
            <artifactId>quickfixj-messages-all</artifactId>
            <version>2.1.0</version>
            <classifier>javadoc</classifier>
        </dependency>
    </dependencies>

</project>
gregallen commented 5 years ago 
nice -19 mvn -T4 clean org.apache.maven.plugins:maven-dependency-plugin:copy-dependencies

[ERROR] Failed to execute goal org.apache.maven.plugins:maven-dependency-plugin:3.1.1:copy-dependencies (default-cli) on project mod1: Error copying artifact from /home/X/.m2/repository/org/quickfixj/quickfixj-messages-all/2.1.0/quickfixj-messages-all-2.1.0-javadoc.jar to /home/X/workspace-gitlab/my_sandbox/mdep633/mod1/../target/quickfixj-messages-all-2.1.0-javadoc.jar: Failed to copy full contents from /home/X/.m2/repository/org/quickfixj/quickfixj-messages-all/2.1.0/quickfixj-messages-all-2.1.0-javadoc.jar to /home/X/workspace-gitlab/my_sandbox/mdep633/mod1/../target/quickfixj-messages-all-2.1.0-javadoc.jar -> [Help 1]

gregallen commented 5 years ago 
Caused by: java.io.IOException: Failed to copy full contents from .m2/repository/org/quickfixj/quickfixj-messages-all/2.1.0/quickfixj-messages-all-2.1.0-javadoc.jar to my_sandbox/mdep633/mod1/../target/quickfixj-messages-all-2.1.0-javadoc.jar
    at org.codehaus.plexus.util.FileUtils.copyFile (FileUtils.java:1090)
    at org.apache.maven.plugins.dependency.AbstractDependencyMojo.copyFile (AbstractDependencyMojo.java:184)
    at org.apache.maven.plugins.dependency.fromDependencies.CopyDependenciesMojo.copyArtifact (CopyDependenciesMojo.java:249)
    at org.apache.maven.plugins.dependency.fromDependencies.CopyDependenciesMojo.doExecute (CopyDependenciesMojo.java:124)
    at org.apache.maven.plugins.dependency.AbstractDependencyMojo.execute (AbstractDependencyMojo.java:143)
    at org.apache.maven.plugin.DefaultBuildPluginManager.executeMojo (DefaultBuildPluginManager.java:137)
    at org.apache.maven.lifecycle.internal.MojoExecutor.execute (MojoExecutor.java:208)
    at org.apache.maven.lifecycle.internal.MojoExecutor.execute (MojoExecutor.java:154)
    at org.apache.maven.lifecycle.internal.MojoExecutor.execute5.jar
 (MojoExecutor.java:146)
    at org.apache.maven.lifecycle.internal.LifecycleModuleBuilder.buildProject (LifecycleModuleBuilder.java:117)
    at org.apache.maven.lifecycle.internal.builder.multithreaded.MultiThreadedBuilder$1.call (MultiThreadedBuilder.java:200)
    at org.apache.maven.lifecycle.internal.builder.multithreaded.MultiThreadedBuilder$1.call (MultiThreadedBuilder.java:196)
    at java.util.concurrent.FutureTask.run (FutureTask.java:266)
    at java.util.concurrent.Executors$RunnableAdapter.call (Executors.java:511)
    at java.util.concurrent.FutureTask.run (FutureTask.java:266)
    at java.util.concurrent.ThreadPoolExecutor.runWorker (ThreadPoolExecutor.java:1149)
    at java.util.concurrent.ThreadPoolExecutor$Worker.run (ThreadPoolExecutor.java:624)
    at java.lang.Thread.run (Thread.java:748)
gregallen commented 5 years ago

@michael-o please see test case above

michael-o commented 5 years ago

@gregallen Thanks for the case. This is, of course, a conceptual problem. I guess this case was never considered. How do you want to make it thread-safe? It need some locking mechanism. I do not unterstand how the amount of written bytes will help if two threads are writing the same moment with different regions. Do you have a PR you'd like to propose? I am all ears..

gregallen commented 5 years ago

@michael-o I suggest that the low-level file writing method should return the total number of bytes written, and that is what you should check against the size of the source file.

Do not attempt to check the size of the destination file, since another racing thread may have already "accidentally" deleted (or truncated) the newly copied file, and be in the middle of re-copying it.

It's not ideal that the file gets copied twice, but not a big deal. Not worth trying to devise some locking approach to avoid that. Instead just simplify the check IMHO

gregallen commented 5 years ago

Looks like org.codehaus.plexus.util.IOUtil#copy(java.io.InputStream, java.io.OutputStream, int) needs to change to return a long and ripple that return value up...

gregallen commented 5 years ago

OR, get rid of the check altogether - the javadoc for java.io.OutputStream#write(byte[], int, int) implies that the bytes WILL be written if no exception is thrown, and there is no exceptional code path back to the size check.

belingueres commented 5 years ago

@gregallen Is it an option to refactor the pom files so that each module copy files to its own directory, then merge those directories into one in a later phase? seems to me that the plugin is not designed to concurrently copy the same files into a common directory.

gregallen commented 5 years ago

no, that would not be possible. There are 150 modules, and 95% of the dependencies are identical. However, there are a few modules that require differing versions of some artifacts, so it is also not possible to do the copy from a single module (since that would only copy one version per artifact). The build runs with 20x parallelism, so the race condition is often seen

gregallen commented 5 years ago

What is the scenario in which the File.length() check adds value? In the out-of-diskspace case, doesn't an IOException get thrown?