Skip to content

Commit 3c245f2

Browse files
jbachorikjbachorik
committed
Improve TempLocationManagerTest.java stability
1 parent 85ac4c8 commit 3c245f2

File tree

2 files changed

+94
-53
lines changed

2 files changed

+94
-53
lines changed

‎internal-api/src/main/java/datadog/trace/util/TempLocationManager.java‎

Lines changed: 20 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -40,28 +40,27 @@ private static final class SingletonHolder {
4040
private static final TempLocationManager INSTANCE = new TempLocationManager();
4141
}
4242

43-
interface CleanupHook extends FileVisitor<Path> {
43+
interface CleanupHook {
4444
CleanupHook EMPTY = new CleanupHook() {};
4545

46-
@Override
47-
default FileVisitResult preVisitDirectory(Path dir, BasicFileAttributes attrs)
46+
default FileVisitResult preVisitDirectory(Path dir, BasicFileAttributes attrs, boolean timeout)
4847
throws IOException {
49-
return null;
48+
return timeout ? FileVisitResult.TERMINATE : FileVisitResult.CONTINUE;
5049
}
5150

52-
@Override
53-
default FileVisitResult visitFile(Path file, BasicFileAttributes attrs) throws IOException {
54-
return null;
51+
default FileVisitResult visitFile(Path file, BasicFileAttributes attrs, boolean timeout)
52+
throws IOException {
53+
return timeout ? FileVisitResult.TERMINATE : FileVisitResult.CONTINUE;
5554
}
5655

57-
@Override
58-
default FileVisitResult visitFileFailed(Path file, IOException exc) throws IOException {
59-
return null;
56+
default FileVisitResult visitFileFailed(Path file, IOException exc, boolean timeout)
57+
throws IOException {
58+
return timeout ? FileVisitResult.TERMINATE : FileVisitResult.CONTINUE;
6059
}
6160

62-
@Override
63-
default FileVisitResult postVisitDirectory(Path dir, IOException exc) throws IOException {
64-
return null;
61+
default FileVisitResult postVisitDirectory(Path dir, IOException exc, boolean timeout)
62+
throws IOException {
63+
return timeout ? FileVisitResult.TERMINATE : FileVisitResult.CONTINUE;
6564
}
6665

6766
default void onCleanupStart(long timeout, TimeUnit unit) {}
@@ -99,10 +98,11 @@ public FileVisitResult preVisitDirectory(Path dir, BasicFileAttributes attrs)
9998
if (isTimedOut()) {
10099
log.debug("Cleaning task timed out");
101100
terminated = true;
101+
cleanupTestHook.preVisitDirectory(dir, attrs, true);
102102
return FileVisitResult.TERMINATE;
103103
}
104104

105-
cleanupTestHook.preVisitDirectory(dir, attrs);
105+
cleanupTestHook.preVisitDirectory(dir, attrs, false);
106106

107107
if (dir.equals(baseTempDir)) {
108108
return FileVisitResult.CONTINUE;
@@ -128,9 +128,10 @@ public FileVisitResult visitFile(Path file, BasicFileAttributes attrs) throws IO
128128
if (isTimedOut()) {
129129
log.debug("Cleaning task timed out");
130130
terminated = true;
131+
cleanupTestHook.visitFile(file, attrs, true);
131132
return FileVisitResult.TERMINATE;
132133
}
133-
cleanupTestHook.visitFile(file, attrs);
134+
cleanupTestHook.visitFile(file, attrs, false);
134135
try {
135136
if (Files.getLastModifiedTime(file).toInstant().isAfter(cutoff)) {
136137
return FileVisitResult.SKIP_SUBTREE;
@@ -147,9 +148,10 @@ public FileVisitResult visitFileFailed(Path file, IOException exc) throws IOExce
147148
if (isTimedOut()) {
148149
log.debug("Cleaning task timed out");
149150
terminated = true;
151+
cleanupTestHook.visitFileFailed(file, exc, true);
150152
return FileVisitResult.TERMINATE;
151153
}
152-
cleanupTestHook.visitFileFailed(file, exc);
154+
cleanupTestHook.visitFileFailed(file, exc, false);
153155
// do not log files/directories removed by another process running concurrently
154156
if (!(exc instanceof NoSuchFileException) && log.isDebugEnabled()) {
155157
log.debug("Failed to delete file {}", file, exc);
@@ -162,9 +164,10 @@ public FileVisitResult postVisitDirectory(Path dir, IOException exc) throws IOEx
162164
if (isTimedOut()) {
163165
log.debug("Cleaning task timed out");
164166
terminated = true;
167+
cleanupTestHook.postVisitDirectory(dir, exc, true);
165168
return FileVisitResult.TERMINATE;
166169
}
167-
cleanupTestHook.postVisitDirectory(dir, exc);
170+
cleanupTestHook.postVisitDirectory(dir, exc, false);
168171
if (exc instanceof NoSuchFileException) {
169172
return FileVisitResult.CONTINUE;
170173
}

‎internal-api/src/test/java/datadog/trace/util/TempLocationManagerTest.java‎

Lines changed: 74 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -2,26 +2,29 @@
22

33
import static org.junit.jupiter.api.Assertions.assertEquals;
44
import static org.junit.jupiter.api.Assertions.assertFalse;
5+
import static org.junit.jupiter.api.Assertions.assertNotEquals;
56
import static org.junit.jupiter.api.Assertions.assertNotNull;
67
import static org.junit.jupiter.api.Assertions.assertThrows;
78
import static org.junit.jupiter.api.Assertions.assertTrue;
89

910
import datadog.trace.api.config.ProfilingConfig;
1011
import datadog.trace.bootstrap.config.provider.ConfigProvider;
11-
import datadog.trace.test.util.Flaky;
1212
import java.io.IOException;
1313
import java.nio.file.FileVisitResult;
1414
import java.nio.file.Files;
1515
import java.nio.file.Path;
1616
import java.nio.file.Paths;
1717
import java.nio.file.attribute.BasicFileAttributes;
1818
import java.nio.file.attribute.PosixFilePermissions;
19+
import java.time.Duration;
1920
import java.util.ArrayList;
2021
import java.util.List;
2122
import java.util.Properties;
2223
import java.util.UUID;
2324
import java.util.concurrent.Phaser;
2425
import java.util.concurrent.TimeUnit;
26+
import java.util.concurrent.TimeoutException;
27+
import java.util.concurrent.atomic.AtomicBoolean;
2528
import java.util.concurrent.locks.LockSupport;
2629
import java.util.stream.Stream;
2730
import org.junit.jupiter.api.Test;
@@ -145,33 +148,40 @@ void testConcurrentCleanup(String section) throws Exception {
145148

146149
Phaser phaser = new Phaser(2);
147150

151+
Duration phaserTimeout =
152+
Duration.ofSeconds(5); // wait at most 5 seconds for phaser coordination
153+
AtomicBoolean withTimeout = new AtomicBoolean(false);
148154
TempLocationManager.CleanupHook blocker =
149155
new TempLocationManager.CleanupHook() {
150156
@Override
151-
public FileVisitResult preVisitDirectory(Path dir, BasicFileAttributes attrs)
152-
throws IOException {
157+
public FileVisitResult preVisitDirectory(
158+
Path dir, BasicFileAttributes attrs, boolean timeout) throws IOException {
153159
if (section.equals("preVisitDirectory") && dir.equals(fakeTempDir)) {
154-
phaser.arriveAndAwaitAdvance();
155-
phaser.arriveAndAwaitAdvance();
160+
withTimeout.compareAndSet(false, timeout);
161+
arriveOrAwaitAdvance(phaser, phaserTimeout);
162+
arriveOrAwaitAdvance(phaser, phaserTimeout);
156163
}
157164
return null;
158165
}
159166

160167
@Override
161-
public FileVisitResult visitFile(Path file, BasicFileAttributes attrs)
168+
public FileVisitResult visitFile(Path file, BasicFileAttributes attrs, boolean timeout)
162169
throws IOException {
163170
if (section.equals("visitFile") && file.equals(fakeTempFile)) {
164-
phaser.arriveAndAwaitAdvance();
165-
phaser.arriveAndAwaitAdvance();
171+
withTimeout.compareAndSet(false, timeout);
172+
arriveOrAwaitAdvance(phaser, phaserTimeout);
173+
arriveOrAwaitAdvance(phaser, phaserTimeout);
166174
}
167175
return null;
168176
}
169177

170178
@Override
171-
public FileVisitResult postVisitDirectory(Path dir, IOException exc) throws IOException {
179+
public FileVisitResult postVisitDirectory(Path dir, IOException exc, boolean timeout)
180+
throws IOException {
172181
if (section.equals("postVisitDirectory") && dir.equals(fakeTempDir)) {
173-
phaser.arriveAndAwaitAdvance();
174-
phaser.arriveAndAwaitAdvance();
182+
withTimeout.compareAndSet(false, timeout);
183+
arriveOrAwaitAdvance(phaser, phaserTimeout);
184+
arriveOrAwaitAdvance(phaser, phaserTimeout);
175185
}
176186
return null;
177187
}
@@ -180,55 +190,63 @@ public FileVisitResult postVisitDirectory(Path dir, IOException exc) throws IOEx
180190
TempLocationManager mgr = instance(baseDir, true, blocker);
181191

182192
// wait for the cleanup start
183-
phaser.arriveAndAwaitAdvance();
184-
Files.deleteIfExists(fakeTempFile);
185-
phaser.arriveAndAwaitAdvance();
193+
if (arriveOrAwaitAdvance(phaser, phaserTimeout)) {
194+
Files.deleteIfExists(fakeTempFile);
195+
assertTrue(arriveOrAwaitAdvance(phaser, phaserTimeout));
196+
}
197+
assertFalse(
198+
withTimeout.get()); // if the cleaner times out it does not make sense to continue here
186199
mgr.waitForCleanup(30, TimeUnit.SECONDS);
187200

188201
assertFalse(Files.exists(fakeTempFile));
189202
assertFalse(Files.exists(fakeTempDir));
190203
}
191204

192-
@Flaky("https://datadoghq.atlassian.net/browse/PROF-11290")
193205
@ParameterizedTest
194206
@MethodSource("timeoutTestArguments")
195-
void testCleanupWithTimeout(boolean selfCleanup, boolean shouldSucceed, String section)
196-
throws Exception {
207+
void testCleanupWithTimeout(boolean shouldSucceed, String section) throws Exception {
197208
long timeoutMs = 10;
209+
AtomicBoolean withTimeout = new AtomicBoolean(false);
198210
TempLocationManager.CleanupHook delayer =
199211
new TempLocationManager.CleanupHook() {
200212
@Override
201-
public FileVisitResult preVisitDirectory(Path dir, BasicFileAttributes attrs)
202-
throws IOException {
213+
public FileVisitResult preVisitDirectory(
214+
Path dir, BasicFileAttributes attrs, boolean timeout) throws IOException {
215+
withTimeout.compareAndSet(false, timeout);
203216
if (section.equals("preVisitDirectory")) {
204-
LockSupport.parkNanos(timeoutMs * 1_000_000);
217+
waitFor(Duration.ofMillis(timeoutMs));
205218
}
206-
return TempLocationManager.CleanupHook.super.preVisitDirectory(dir, attrs);
219+
return TempLocationManager.CleanupHook.super.preVisitDirectory(dir, attrs, timeout);
207220
}
208221

209222
@Override
210-
public FileVisitResult visitFileFailed(Path file, IOException exc) throws IOException {
223+
public FileVisitResult visitFileFailed(Path file, IOException exc, boolean timeout)
224+
throws IOException {
225+
withTimeout.compareAndSet(false, timeout);
211226
if (section.equals("visitFileFailed")) {
212-
LockSupport.parkNanos(timeoutMs * 1_000_000);
227+
waitFor(Duration.ofMillis(timeoutMs));
213228
}
214-
return TempLocationManager.CleanupHook.super.visitFileFailed(file, exc);
229+
return TempLocationManager.CleanupHook.super.visitFileFailed(file, exc, timeout);
215230
}
216231

217232
@Override
218-
public FileVisitResult postVisitDirectory(Path dir, IOException exc) throws IOException {
233+
public FileVisitResult postVisitDirectory(Path dir, IOException exc, boolean timeout)
234+
throws IOException {
235+
withTimeout.compareAndSet(false, timeout);
219236
if (section.equals("postVisitDirectory")) {
220-
LockSupport.parkNanos(timeoutMs * 1_000_000);
237+
waitFor(Duration.ofMillis(timeoutMs));
221238
}
222-
return TempLocationManager.CleanupHook.super.postVisitDirectory(dir, exc);
239+
return TempLocationManager.CleanupHook.super.postVisitDirectory(dir, exc, timeout);
223240
}
224241

225242
@Override
226-
public FileVisitResult visitFile(Path file, BasicFileAttributes attrs)
243+
public FileVisitResult visitFile(Path file, BasicFileAttributes attrs, boolean timeout)
227244
throws IOException {
245+
withTimeout.compareAndSet(false, timeout);
228246
if (section.equals("visitFile")) {
229-
LockSupport.parkNanos(timeoutMs * 1_000_000);
247+
waitFor(Duration.ofMillis(timeoutMs));
230248
}
231-
return TempLocationManager.CleanupHook.super.visitFile(file, attrs);
249+
return TempLocationManager.CleanupHook.super.visitFile(file, attrs, timeout);
232250
}
233251
};
234252
Path baseDir =
@@ -244,16 +262,15 @@ public FileVisitResult visitFile(Path file, BasicFileAttributes attrs)
244262
boolean rslt =
245263
instance.cleanup((long) (timeoutMs * (shouldSucceed ? 20 : 0.5d)), TimeUnit.MILLISECONDS);
246264
assertEquals(shouldSucceed, rslt);
265+
assertNotEquals(shouldSucceed, withTimeout.get()); // timeout = !shouldSucceed
247266
}
248267

249268
private static Stream<Arguments> timeoutTestArguments() {
250269
List<Arguments> argumentsList = new ArrayList<>();
251-
for (boolean selfCleanup : new boolean[] {true, false}) {
252-
for (String intercepted :
253-
new String[] {"preVisitDirectory", "visitFile", "postVisitDirectory"}) {
254-
argumentsList.add(Arguments.of(selfCleanup, true, intercepted));
255-
argumentsList.add(Arguments.of(selfCleanup, false, intercepted));
256-
}
270+
for (String intercepted :
271+
new String[] {"preVisitDirectory", "visitFile", "postVisitDirectory"}) {
272+
argumentsList.add(Arguments.of(true, intercepted));
273+
argumentsList.add(Arguments.of(false, intercepted));
257274
}
258275
return argumentsList.stream();
259276
}
@@ -270,4 +287,25 @@ private TempLocationManager instance(
270287
return new TempLocationManager(
271288
ConfigProvider.withPropertiesOverride(props), withStartupCleanup, cleanupHook);
272289
}
290+
291+
private void waitFor(Duration timeout) {
292+
long target = System.nanoTime() + timeout.toNanos();
293+
while (System.nanoTime() < target) {
294+
long toSleep = target - System.nanoTime();
295+
if (toSleep > 0) {
296+
LockSupport.parkNanos(toSleep);
297+
}
298+
}
299+
}
300+
301+
private boolean arriveOrAwaitAdvance(Phaser phaser, Duration timeout) {
302+
try {
303+
System.err.println("===> waiting " + phaser.getPhase());
304+
phaser.awaitAdvanceInterruptibly(phaser.arrive(), timeout.toMillis(), TimeUnit.MILLISECONDS);
305+
System.err.println("===> done waiting " + phaser.getPhase());
306+
return true;
307+
} catch (InterruptedException | TimeoutException ignored) {
308+
return false;
309+
}
310+
}
273311
}

0 commit comments

Comments
 (0)