Copilot commented on code in PR #2841:
URL: https://github.com/apache/karaf/pull/2841#discussion_r3897670458
##########
features/core/src/main/java/org/apache/karaf/features/internal/download/impl/SimpleDownloadTask.java:
##########
@@ -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) {
Review Comment:
`REPLACE_EXISTING` is not guaranteed when combined with `ATOMIC_MOVE`; the
`Files.move` contract says all other options are ignored and replacement of an
existing target is provider-specific. For example, a provider may throw
`FileAlreadyExistsException` after another download wins this race, so one of
the overlapping resolutions still fails. Handle that target-exists outcome as a
successful concurrent win (and clean up `tmpFile`), or otherwise implement
replacement in a way that preserves the intended behavior on providers that
reject atomic replacement.
##########
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.
Review Comment:
Discarding every writer exception makes this regression test pass vacuously
if the new move fails for all writers: the reader never sets `everSeen`, so the
final count remains zero. This also masks providers that reject `ATOMIC_MOVE`
when the target already exists. Capture the submitted futures (or failures) and
assert that every download completes successfully before accepting the
missing-file assertion.
This issue also appears on line 126 of the same file.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
