diff --git a/internal-api/src/main/java/datadog/trace/util/TempLocationManager.java b/internal-api/src/main/java/datadog/trace/util/TempLocationManager.java index 0b0b2f113c2..c8129c68c47 100644 --- a/internal-api/src/main/java/datadog/trace/util/TempLocationManager.java +++ b/internal-api/src/main/java/datadog/trace/util/TempLocationManager.java @@ -40,28 +40,27 @@ private static final class SingletonHolder { private static final TempLocationManager INSTANCE = new TempLocationManager(); } - interface CleanupHook extends FileVisitor { + interface CleanupHook { CleanupHook EMPTY = new CleanupHook() {}; - @Override - default FileVisitResult preVisitDirectory(Path dir, BasicFileAttributes attrs) + default FileVisitResult preVisitDirectory(Path dir, BasicFileAttributes attrs, boolean timeout) throws IOException { - return null; + return timeout ? FileVisitResult.TERMINATE : FileVisitResult.CONTINUE; } - @Override - default FileVisitResult visitFile(Path file, BasicFileAttributes attrs) throws IOException { - return null; + default FileVisitResult visitFile(Path file, BasicFileAttributes attrs, boolean timeout) + throws IOException { + return timeout ? FileVisitResult.TERMINATE : FileVisitResult.CONTINUE; } - @Override - default FileVisitResult visitFileFailed(Path file, IOException exc) throws IOException { - return null; + default FileVisitResult visitFileFailed(Path file, IOException exc, boolean timeout) + throws IOException { + return timeout ? FileVisitResult.TERMINATE : FileVisitResult.CONTINUE; } - @Override - default FileVisitResult postVisitDirectory(Path dir, IOException exc) throws IOException { - return null; + default FileVisitResult postVisitDirectory(Path dir, IOException exc, boolean timeout) + throws IOException { + return timeout ? FileVisitResult.TERMINATE : FileVisitResult.CONTINUE; } default void onCleanupStart(long timeout, TimeUnit unit) {} @@ -99,10 +98,11 @@ public FileVisitResult preVisitDirectory(Path dir, BasicFileAttributes attrs) if (isTimedOut()) { log.debug("Cleaning task timed out"); terminated = true; + cleanupTestHook.preVisitDirectory(dir, attrs, true); return FileVisitResult.TERMINATE; } - cleanupTestHook.preVisitDirectory(dir, attrs); + cleanupTestHook.preVisitDirectory(dir, attrs, false); if (dir.equals(baseTempDir)) { return FileVisitResult.CONTINUE; @@ -128,9 +128,10 @@ public FileVisitResult visitFile(Path file, BasicFileAttributes attrs) throws IO if (isTimedOut()) { log.debug("Cleaning task timed out"); terminated = true; + cleanupTestHook.visitFile(file, attrs, true); return FileVisitResult.TERMINATE; } - cleanupTestHook.visitFile(file, attrs); + cleanupTestHook.visitFile(file, attrs, false); try { if (Files.getLastModifiedTime(file).toInstant().isAfter(cutoff)) { return FileVisitResult.SKIP_SUBTREE; @@ -147,9 +148,10 @@ public FileVisitResult visitFileFailed(Path file, IOException exc) throws IOExce if (isTimedOut()) { log.debug("Cleaning task timed out"); terminated = true; + cleanupTestHook.visitFileFailed(file, exc, true); return FileVisitResult.TERMINATE; } - cleanupTestHook.visitFileFailed(file, exc); + cleanupTestHook.visitFileFailed(file, exc, false); // do not log files/directories removed by another process running concurrently if (!(exc instanceof NoSuchFileException) && log.isDebugEnabled()) { log.debug("Failed to delete file {}", file, exc); @@ -162,9 +164,10 @@ public FileVisitResult postVisitDirectory(Path dir, IOException exc) throws IOEx if (isTimedOut()) { log.debug("Cleaning task timed out"); terminated = true; + cleanupTestHook.postVisitDirectory(dir, exc, true); return FileVisitResult.TERMINATE; } - cleanupTestHook.postVisitDirectory(dir, exc); + cleanupTestHook.postVisitDirectory(dir, exc, false); if (exc instanceof NoSuchFileException) { return FileVisitResult.CONTINUE; } diff --git a/internal-api/src/test/java/datadog/trace/util/TempLocationManagerTest.java b/internal-api/src/test/java/datadog/trace/util/TempLocationManagerTest.java index af9580f8981..c9bc74aa371 100644 --- a/internal-api/src/test/java/datadog/trace/util/TempLocationManagerTest.java +++ b/internal-api/src/test/java/datadog/trace/util/TempLocationManagerTest.java @@ -2,13 +2,13 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNotEquals; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; import datadog.trace.api.config.ProfilingConfig; import datadog.trace.bootstrap.config.provider.ConfigProvider; -import datadog.trace.test.util.Flaky; import java.io.IOException; import java.nio.file.FileVisitResult; import java.nio.file.Files; @@ -16,12 +16,15 @@ import java.nio.file.Paths; import java.nio.file.attribute.BasicFileAttributes; import java.nio.file.attribute.PosixFilePermissions; +import java.time.Duration; import java.util.ArrayList; import java.util.List; import java.util.Properties; import java.util.UUID; import java.util.concurrent.Phaser; import java.util.concurrent.TimeUnit; +import java.util.concurrent.TimeoutException; +import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.locks.LockSupport; import java.util.stream.Stream; import org.junit.jupiter.api.Test; @@ -145,33 +148,40 @@ void testConcurrentCleanup(String section) throws Exception { Phaser phaser = new Phaser(2); + Duration phaserTimeout = + Duration.ofSeconds(5); // wait at most 5 seconds for phaser coordination + AtomicBoolean withTimeout = new AtomicBoolean(false); TempLocationManager.CleanupHook blocker = new TempLocationManager.CleanupHook() { @Override - public FileVisitResult preVisitDirectory(Path dir, BasicFileAttributes attrs) - throws IOException { + public FileVisitResult preVisitDirectory( + Path dir, BasicFileAttributes attrs, boolean timeout) throws IOException { if (section.equals("preVisitDirectory") && dir.equals(fakeTempDir)) { - phaser.arriveAndAwaitAdvance(); - phaser.arriveAndAwaitAdvance(); + withTimeout.compareAndSet(false, timeout); + arriveOrAwaitAdvance(phaser, phaserTimeout); + arriveOrAwaitAdvance(phaser, phaserTimeout); } return null; } @Override - public FileVisitResult visitFile(Path file, BasicFileAttributes attrs) + public FileVisitResult visitFile(Path file, BasicFileAttributes attrs, boolean timeout) throws IOException { if (section.equals("visitFile") && file.equals(fakeTempFile)) { - phaser.arriveAndAwaitAdvance(); - phaser.arriveAndAwaitAdvance(); + withTimeout.compareAndSet(false, timeout); + arriveOrAwaitAdvance(phaser, phaserTimeout); + arriveOrAwaitAdvance(phaser, phaserTimeout); } return null; } @Override - public FileVisitResult postVisitDirectory(Path dir, IOException exc) throws IOException { + public FileVisitResult postVisitDirectory(Path dir, IOException exc, boolean timeout) + throws IOException { if (section.equals("postVisitDirectory") && dir.equals(fakeTempDir)) { - phaser.arriveAndAwaitAdvance(); - phaser.arriveAndAwaitAdvance(); + withTimeout.compareAndSet(false, timeout); + arriveOrAwaitAdvance(phaser, phaserTimeout); + arriveOrAwaitAdvance(phaser, phaserTimeout); } return null; } @@ -180,55 +190,63 @@ public FileVisitResult postVisitDirectory(Path dir, IOException exc) throws IOEx TempLocationManager mgr = instance(baseDir, true, blocker); // wait for the cleanup start - phaser.arriveAndAwaitAdvance(); - Files.deleteIfExists(fakeTempFile); - phaser.arriveAndAwaitAdvance(); + if (arriveOrAwaitAdvance(phaser, phaserTimeout)) { + Files.deleteIfExists(fakeTempFile); + assertTrue(arriveOrAwaitAdvance(phaser, phaserTimeout)); + } + assertFalse( + withTimeout.get()); // if the cleaner times out it does not make sense to continue here mgr.waitForCleanup(30, TimeUnit.SECONDS); assertFalse(Files.exists(fakeTempFile)); assertFalse(Files.exists(fakeTempDir)); } - @Flaky("https://datadoghq.atlassian.net/browse/PROF-11290") @ParameterizedTest @MethodSource("timeoutTestArguments") - void testCleanupWithTimeout(boolean selfCleanup, boolean shouldSucceed, String section) - throws Exception { + void testCleanupWithTimeout(boolean shouldSucceed, String section) throws Exception { long timeoutMs = 10; + AtomicBoolean withTimeout = new AtomicBoolean(false); TempLocationManager.CleanupHook delayer = new TempLocationManager.CleanupHook() { @Override - public FileVisitResult preVisitDirectory(Path dir, BasicFileAttributes attrs) - throws IOException { + public FileVisitResult preVisitDirectory( + Path dir, BasicFileAttributes attrs, boolean timeout) throws IOException { + withTimeout.compareAndSet(false, timeout); if (section.equals("preVisitDirectory")) { - LockSupport.parkNanos(timeoutMs * 1_000_000); + waitFor(Duration.ofMillis(timeoutMs)); } - return TempLocationManager.CleanupHook.super.preVisitDirectory(dir, attrs); + return TempLocationManager.CleanupHook.super.preVisitDirectory(dir, attrs, timeout); } @Override - public FileVisitResult visitFileFailed(Path file, IOException exc) throws IOException { + public FileVisitResult visitFileFailed(Path file, IOException exc, boolean timeout) + throws IOException { + withTimeout.compareAndSet(false, timeout); if (section.equals("visitFileFailed")) { - LockSupport.parkNanos(timeoutMs * 1_000_000); + waitFor(Duration.ofMillis(timeoutMs)); } - return TempLocationManager.CleanupHook.super.visitFileFailed(file, exc); + return TempLocationManager.CleanupHook.super.visitFileFailed(file, exc, timeout); } @Override - public FileVisitResult postVisitDirectory(Path dir, IOException exc) throws IOException { + public FileVisitResult postVisitDirectory(Path dir, IOException exc, boolean timeout) + throws IOException { + withTimeout.compareAndSet(false, timeout); if (section.equals("postVisitDirectory")) { - LockSupport.parkNanos(timeoutMs * 1_000_000); + waitFor(Duration.ofMillis(timeoutMs)); } - return TempLocationManager.CleanupHook.super.postVisitDirectory(dir, exc); + return TempLocationManager.CleanupHook.super.postVisitDirectory(dir, exc, timeout); } @Override - public FileVisitResult visitFile(Path file, BasicFileAttributes attrs) + public FileVisitResult visitFile(Path file, BasicFileAttributes attrs, boolean timeout) throws IOException { + withTimeout.compareAndSet(false, timeout); if (section.equals("visitFile")) { - LockSupport.parkNanos(timeoutMs * 1_000_000); + waitFor(Duration.ofMillis(timeoutMs)); } - return TempLocationManager.CleanupHook.super.visitFile(file, attrs); + return TempLocationManager.CleanupHook.super.visitFile(file, attrs, timeout); } }; Path baseDir = @@ -244,16 +262,15 @@ public FileVisitResult visitFile(Path file, BasicFileAttributes attrs) boolean rslt = instance.cleanup((long) (timeoutMs * (shouldSucceed ? 20 : 0.5d)), TimeUnit.MILLISECONDS); assertEquals(shouldSucceed, rslt); + assertNotEquals(shouldSucceed, withTimeout.get()); // timeout = !shouldSucceed } private static Stream timeoutTestArguments() { List argumentsList = new ArrayList<>(); - for (boolean selfCleanup : new boolean[] {true, false}) { - for (String intercepted : - new String[] {"preVisitDirectory", "visitFile", "postVisitDirectory"}) { - argumentsList.add(Arguments.of(selfCleanup, true, intercepted)); - argumentsList.add(Arguments.of(selfCleanup, false, intercepted)); - } + for (String intercepted : + new String[] {"preVisitDirectory", "visitFile", "postVisitDirectory"}) { + argumentsList.add(Arguments.of(true, intercepted)); + argumentsList.add(Arguments.of(false, intercepted)); } return argumentsList.stream(); } @@ -270,4 +287,25 @@ private TempLocationManager instance( return new TempLocationManager( ConfigProvider.withPropertiesOverride(props), withStartupCleanup, cleanupHook); } + + private void waitFor(Duration timeout) { + long target = System.nanoTime() + timeout.toNanos(); + while (System.nanoTime() < target) { + long toSleep = target - System.nanoTime(); + if (toSleep > 0) { + LockSupport.parkNanos(toSleep); + } + } + } + + private boolean arriveOrAwaitAdvance(Phaser phaser, Duration timeout) { + try { + System.err.println("===> waiting " + phaser.getPhase()); + phaser.awaitAdvanceInterruptibly(phaser.arrive(), timeout.toMillis(), TimeUnit.MILLISECONDS); + System.err.println("===> done waiting " + phaser.getPhase()); + return true; + } catch (InterruptedException | TimeoutException ignored) { + return false; + } + } }