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 548c234d80b..454daaa1a22 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.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 @@ protected File download(Exception previousExceptionNotUsed) throws Exception { 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 00000000000..81da9c9b4a5 --- /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()); + } +}