From 4213391a570853b450afe41deb345f478bb8a4e4 Mon Sep 17 00:00:00 2001 From: Josh Soref Date: Wed, 20 Mar 2019 16:26:03 -0400 Subject: [PATCH] [JENKINS-56643] Util.createSymlink: prefer Files.move(..., StandardCopyOption.ATOMIC_MOVE) Atomic moves mean that there is no time point during which the symlink will not exist. Otherwise, if there is a user trying to access the symlink and there is a process trying to update the symlink, there will be a window during which there is no symlink. When that happens, builds can randomly fail. Implementation: Introduce a private createSymlinkAtomic which will atomically create a symlink in the right place and return true, or return false leaving no changes to the file system. It does not throw exceptions. When it fails, the previous code is used instead. Changes to the existing code are designed to slightly clarify what objects are and reuse objects instead of creating them multiple times. --- core/src/main/java/hudson/Util.java | 73 +++++++++++++++++++++++++++-- 1 file changed, 69 insertions(+), 4 deletions(-) diff --git a/core/src/main/java/hudson/Util.java b/core/src/main/java/hudson/Util.java index dc3de475bf..e5b4c00c7d 100644 --- a/core/src/main/java/hudson/Util.java +++ b/core/src/main/java/hudson/Util.java @@ -85,6 +85,7 @@ import java.util.regex.Matcher; import java.util.regex.Pattern; import javax.annotation.CheckForNull; +import javax.annotation.CheckReturnValue; import javax.annotation.Nonnull; import javax.annotation.Nullable; import javax.crypto.SecretKey; @@ -1168,6 +1169,65 @@ public class Util { return createFileSet(baseDir,includes,null); } + private static void tryToDeleteSymlink(@Nonnull File symlink) { + if (!symlink.delete()) { + LogRecord record = new LogRecord(Level.FINE, "Failed to delete temporary symlink {0}"); + record.setParameters(new Object[]{symlink.getAbsolutePath()}); + LOGGER.log(record); + } + } + + private static void reportAtomicFailure(@Nonnull Path pathForSymlink, @Nonnull Exception ex) { + LogRecord record = new LogRecord(Level.FINE, "Failed to atomically create/replace symlink {0}"); + record.setParameters(new Object[]{pathForSymlink.toAbsolutePath().toString()}); + record.setThrown(ex); + LOGGER.log(record); + } + + /** + * Creates a symlink to targetPath at baseDir+symlinkPath. + * + * @param pathForSymlink + * The absolute path of the symlink itself as a path object. + * @param fileForSymlink + * The absolute path of the symlink itself as a file object. + * @param target + * The path that the symlink should point to. Usually relative to the directory of the symlink but may instead be an absolute path. + * @param symlinkPath + * Where to create a symlink in (relative to {@code baseDir}) + * + * Returns true on success + */ + @CheckReturnValue + private static boolean createSymlinkAtomic(@Nonnull Path pathForSymlink, @Nonnull File fileForSymlink, @Nonnull Path target, @Nonnull String symlinkPath) { + try { + File symlink = File.createTempFile("symtmp", null, fileForSymlink); + tryToDeleteSymlink(symlink); + Path tempSymlinkPath = symlink.toPath(); + Files.createSymbolicLink(tempSymlinkPath, target); + try { + Files.move(tempSymlinkPath, pathForSymlink, java.nio.file.StandardCopyOption.ATOMIC_MOVE); + return true; + } catch ( + UnsupportedOperationException | + SecurityException | + IOException ex) { + // If we couldn't perform an atomic move or the setup, we fall through to another approach + reportAtomicFailure(pathForSymlink, ex); + } + // If we didn't return after our atomic move, then we want to clean up our symlink + tryToDeleteSymlink(symlink); + } catch ( + SecurityException | + InvalidPathException | + UnsupportedOperationException | + IOException ex) { + // We couldn't perform an atomic move or the setup. + reportAtomicFailure(pathForSymlink, ex); + } + return false; + } + /** * Creates a symlink to targetPath at baseDir+symlinkPath. *

@@ -1182,16 +1242,21 @@ public class Util { */ public static void createSymlink(@Nonnull File baseDir, @Nonnull String targetPath, @Nonnull String symlinkPath, @Nonnull TaskListener listener) throws InterruptedException { + File fileForSymlink = new File(baseDir, symlinkPath); try { - Path path = fileToPath(new File(baseDir, symlinkPath)); + Path pathForSymlink = fileToPath(fileForSymlink); Path target = Paths.get(targetPath, MemoryReductionUtil.EMPTY_STRING_ARRAY); + if (createSymlinkAtomic(pathForSymlink, fileForSymlink, target, symlinkPath)) { + return; + } + final int maxNumberOfTries = 4; final int timeInMillis = 100; for (int tryNumber = 1; tryNumber <= maxNumberOfTries; tryNumber++) { - Files.deleteIfExists(path); + Files.deleteIfExists(pathForSymlink); try { - Files.createSymbolicLink(path, target); + Files.createSymbolicLink(pathForSymlink, target); break; } catch (FileAlreadyExistsException fileAlreadyExistsException) { if (tryNumber < maxNumberOfTries) { @@ -1212,7 +1277,7 @@ public class Util { return; } PrintStream log = listener.getLogger(); - log.printf("ln %s %s failed%n",targetPath, new File(baseDir, symlinkPath)); + log.printf("ln %s %s failed%n", targetPath, fileForSymlink); Functions.printStackTrace(e, log); } }