This is an automated email from the ASF dual-hosted git repository.
jbonofre pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/karaf.git
The following commit(s) were added to refs/heads/main by this push:
new 39b9149ff0 Use atomic move instead of delete-then-rename in
SimpleDownloadTask (#2841)
39b9149ff0 is described below
commit 39b9149ff02e2d64e87bbb0042aceef6d95d80d8
Author: Aditya Nikam <[email protected]>
AuthorDate: Thu Sep 10 20:21:20 2026 +0530
Use atomic move instead of delete-then-rename in SimpleDownloadTask (#2841)
SimpleDownloadTask.download() staged wrap:/blueprint:/spring: bundle
URLs into a temp file, then did:
if (file.exists() && !file.delete()) { throw ...; }
tmpFile.renameTo(file);
Two overlapping resolutions of the same URL (e.g. two feature
installs, each with their own DownloadManager -- dedup only happens
within one instance) can race: the second one's delete() removes the
first one's just-written file right as a third reader opens it,
producing an intermittent FileNotFoundException. This shows up as
needing multiple restarts before a container boots cleanly.
Replace the delete-then-rename with a single Files.move(...,
ATOMIC_MOVE), falling back to a plain move if the filesystem doesn't
support atomic moves. A concurrent reader then always sees either the
old or the new file, never a momentarily missing one.
Added SimpleDownloadTaskTest, which runs 200 rounds of 6 concurrent
downloads racing on the same destination while a reader thread polls
for the file transiently disappearing after having existed. On the
old delete-then-rename code this fails with hundreds of thousands of
missing-file observations; with the fix it's zero.
Fixes #2808
Signed-off-by: adityaanikam <[email protected]>
---
.../internal/download/impl/SimpleDownloadTask.java | 18 ++-
.../download/impl/SimpleDownloadTaskTest.java | 158 +++++++++++++++++++++
2 files changed, 171 insertions(+), 5 deletions(-)
diff --git
a/features/core/src/main/java/org/apache/karaf/features/internal/download/impl/SimpleDownloadTask.java
b/features/core/src/main/java/org/apache/karaf/features/internal/download/impl/SimpleDownloadTask.java
index 548c234d80..454daaa1a2 100644
---
a/features/core/src/main/java/org/apache/karaf/features/internal/download/impl/SimpleDownloadTask.java
+++
b/features/core/src/main/java/org/apache/karaf/features/internal/download/impl/SimpleDownloadTask.java
@@ -22,7 +22,9 @@ import java.io.IOException;
import java.io.InputStream;
import java.io.OutputStream;
import java.net.URL;
+import java.nio.file.AtomicMoveNotSupportedException;
import java.nio.file.Files;
+import java.nio.file.StandardCopyOption;
import java.util.concurrent.ScheduledExecutorService;
import org.apache.karaf.util.StreamUtils;
@@ -80,12 +82,18 @@ public class SimpleDownloadTask extends
AbstractRetryableDownloadTask {
StreamUtils.copy(is, os);
}
- if (file.exists() && !file.delete()) {
- throw new IOException("Unable to delete file: " +
file.toString());
- }
// check: this will move the file to CHILD_HOME root directory...
- if (!tmpFile.renameTo(file)) {
- throw new IOException("Unable to rename file " +
tmpFile.toString() + " to " + file.toString());
+ // Move atomically instead of delete-then-rename: two overlapping
downloads of the
+ // same URL (e.g. two feature installs, each with their own
DownloadManager -- dedup
+ // only happens within one instance) can otherwise race, with the
second download's
+ // delete() removing the first's just-written file while a third
reader has it open
+ // (gh-2808). Files.move(..., ATOMIC_MOVE) makes the destination
always either the old
+ // or the new content, never transiently missing.
+ try {
+ Files.move(tmpFile.toPath(), file.toPath(),
+ StandardCopyOption.REPLACE_EXISTING,
StandardCopyOption.ATOMIC_MOVE);
+ } catch (AtomicMoveNotSupportedException e) {
+ Files.move(tmpFile.toPath(), file.toPath(),
StandardCopyOption.REPLACE_EXISTING);
}
return file;
} catch (Exception ignore) {
diff --git
a/features/core/src/test/java/org/apache/karaf/features/internal/download/impl/SimpleDownloadTaskTest.java
b/features/core/src/test/java/org/apache/karaf/features/internal/download/impl/SimpleDownloadTaskTest.java
new file mode 100644
index 0000000000..81da9c9b4a
--- /dev/null
+++
b/features/core/src/test/java/org/apache/karaf/features/internal/download/impl/SimpleDownloadTaskTest.java
@@ -0,0 +1,158 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.karaf.features.internal.download.impl;
+
+import java.io.File;
+import java.io.FileOutputStream;
+import java.net.URL;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.Future;
+import java.util.concurrent.ScheduledExecutorService;
+import java.util.concurrent.ScheduledThreadPoolExecutor;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicInteger;
+
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+
+import static org.junit.Assert.assertEquals;
+
+public class SimpleDownloadTaskTest {
+
+ private Path tempDir;
+ private ScheduledExecutorService executorService;
+ private String previousKarafData;
+
+ @Before
+ public void setUp() throws Exception {
+ tempDir = Files.createTempDirectory("SimpleDownloadTaskTest");
+ executorService = new ScheduledThreadPoolExecutor(1);
+ previousKarafData = System.getProperty("karaf.data");
+ System.setProperty("karaf.data",
tempDir.resolve("karaf-data").toString());
+ }
+
+ @After
+ public void tearDown() throws Exception {
+ executorService.shutdownNow();
+ if (previousKarafData != null) {
+ System.setProperty("karaf.data", previousKarafData);
+ } else {
+ System.clearProperty("karaf.data");
+ }
+ deleteRecursively(tempDir);
+ }
+
+ // Reproduces the race in gh-2808: two overlapping downloads of the same
URL (e.g. two
+ // feature installs, each with their own DownloadManager) can start before
either has written
+ // the destination file. With the old delete-then-renameTo sequence,
whichever download loses
+ // the race deletes the file the other just wrote, and a concurrent reader
can observe the
+ // destination transiently missing. Files.move(..., ATOMIC_MOVE) closes
that window: the
+ // destination is always either the old or new content, never briefly
absent.
+ @Test
+ public void
concurrentDownloadsOfSameUrlNeverExposeATransientlyMissingFile() throws
Exception {
+ File basePath = tempDir.resolve("basePath").toFile();
+ basePath.mkdirs();
+
+ File source = tempDir.resolve("source.jar").toFile();
+ try (FileOutputStream os = new FileOutputStream(source)) {
+ os.write("content".getBytes(StandardCharsets.UTF_8));
+ }
+ String url = source.toURI().toURL().toString();
+ File destination = new File(basePath, Integer.toHexString(new
URL(url).toString().hashCode()) + "-source.jar");
+
+ int writers = 6;
+ int rounds = 200;
+ AtomicInteger missingFileObservations = new AtomicInteger();
+
+ ExecutorService pool = Executors.newFixedThreadPool(writers + 1);
+ try {
+ for (int round = 0; round < rounds; round++) {
+ Files.deleteIfExists(destination.toPath());
+
+ CountDownLatch ready = new CountDownLatch(writers);
+ CountDownLatch go = new CountDownLatch(1);
+ CountDownLatch done = new CountDownLatch(writers);
+ AtomicBoolean stopReader = new AtomicBoolean(false);
+
+ for (int i = 0; i < writers; i++) {
+ pool.submit(() -> {
+ ready.countDown();
+ awaitUninterruptibly(go);
+ try {
+ new SimpleDownloadTask(executorService, url,
basePath).download(null);
+ } catch (Exception ignore) {
+ // An individual attempt failing outright is not
the bug under test --
+ // only a *transiently missing* destination file
is.
+ } finally {
+ done.countDown();
+ }
+ });
+ }
+ Future<?> reader = pool.submit(() -> {
+ boolean everSeen = false;
+ while (!stopReader.get()) {
+ boolean exists = destination.exists();
+ if (exists) {
+ everSeen = true;
+ } else if (everSeen) {
+ missingFileObservations.incrementAndGet();
+ }
+ }
+ });
+
+ ready.await();
+ go.countDown();
+ done.await(10, TimeUnit.SECONDS);
+ stopReader.set(true);
+ reader.get(10, TimeUnit.SECONDS);
+ }
+ } finally {
+ pool.shutdownNow();
+ }
+
+ assertEquals(
+ "A concurrent reader observed the destination file transiently
missing after it had "
+ + "already been created once in the same round -- the
delete-then-rename race "
+ + "from gh-2808",
+ 0, missingFileObservations.get());
+ }
+
+ private static void awaitUninterruptibly(CountDownLatch latch) {
+ try {
+ latch.await();
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ throw new RuntimeException(e);
+ }
+ }
+
+ private static void deleteRecursively(Path path) throws Exception {
+ if (!Files.exists(path)) {
+ return;
+ }
+ Files.walk(path)
+ .sorted((a, b) -> b.compareTo(a))
+ .forEach(p -> p.toFile().delete());
+ }
+}
