fix: fix downloading different URLs to same destination

Author: spikecurtis

Tests.Vpn.Service/DownloaderTest.cs

@@ -284,6 +284,34 @@ public async Task Download(CancellationToken ct)
         Assert.That(await File.ReadAllTextAsync(destPath, ct), Is.EqualTo("test"));
     }
 
+    [Test(Description = "Perform 2 downloads with the same destination")]
+    [CancelAfter(30_000)]
+    public async Task DownloadSameDest(CancellationToken ct)
+    {
+        using var httpServer = EchoServer();
+        var url0 = new Uri(httpServer.BaseUrl + "/test0");
+        var url1 = new Uri(httpServer.BaseUrl + "/test1");
+        var destPath = Path.Combine(_tempDir, "test");
+
+        var manager = new Downloader(NullLogger<Downloader>.Instance);
+        var startTask0 = manager.StartDownloadAsync(new HttpRequestMessage(HttpMethod.Get, url0), destPath,
+            NullDownloadValidator.Instance, ct);
+        var startTask1 = manager.StartDownloadAsync(new HttpRequestMessage(HttpMethod.Get, url1), destPath,
+            NullDownloadValidator.Instance, ct);
+        var dlTask0 = await startTask0;
+        await dlTask0.Task;
+        Assert.That(dlTask0.TotalBytes, Is.EqualTo(5));
+        Assert.That(dlTask0.BytesRead, Is.EqualTo(5));
+        Assert.That(dlTask0.Progress, Is.EqualTo(1));
+        Assert.That(dlTask0.IsCompleted, Is.True);
+        var dlTask1 = await startTask1;
+        await dlTask1.Task;
+        Assert.That(dlTask1.TotalBytes, Is.EqualTo(5));
+        Assert.That(dlTask1.BytesRead, Is.EqualTo(5));
+        Assert.That(dlTask1.Progress, Is.EqualTo(1));
+        Assert.That(dlTask1.IsCompleted, Is.True);
+    }
+
     [Test(Description = "Download with custom headers")]
     [CancelAfter(30_000)]
     public async Task WithHeaders(CancellationToken ct)
@@ -395,9 +423,9 @@ public void CancelledOuter(CancellationToken ct)
         var manager = new Downloader(NullLogger<Downloader>.Instance);
         // The "outer" Task should fail.
         var smallerCt = new CancellationTokenSource(TimeSpan.FromSeconds(1)).Token;
-        Assert.ThrowsAsync<TaskCanceledException>(
-            async () => await manager.StartDownloadAsync(new HttpRequestMessage(HttpMethod.Get, url), destPath,
-                NullDownloadValidator.Instance, smallerCt));
+        Assert.ThrowsAsync<TaskCanceledException>(async () => await manager.StartDownloadAsync(
+            new HttpRequestMessage(HttpMethod.Get, url), destPath,
+            NullDownloadValidator.Instance, smallerCt));
     }
 
     [Test(Description = "Timeout on response body")]

Vpn.Service/Downloader.cs

@@ -288,7 +288,9 @@ public async Task<DownloadTask> StartDownloadAsync(HttpRequestMessage req, strin
         {
             var task = _downloads.GetOrAdd(destinationPath,
                 _ => new DownloadTask(_logger, req, destinationPath, validator));
-            await task.EnsureStartedAsync(ct);
+            // EnsureStarted is a no-op if we didn't create a new DownloadTask.
+            // So, we will only remove the destination once for each time we start a new task.
+            task.EnsureStarted(tsk => _downloads.TryRemove(destinationPath, out _), ct);
 
             // If the existing (or new) task is for the same URL, return it.
             if (task.Request.RequestUri == req.RequestUri)
@@ -357,21 +359,19 @@ internal DownloadTask(ILogger logger, HttpRequestMessage req, string destination
                                                                   ".download-" + Path.GetRandomFileName());
     }
 
-    internal async Task<Task> EnsureStartedAsync(CancellationToken ct = default)
+    internal void EnsureStarted(Action<Task> continuation, CancellationToken ct = default)
     {
-        using var _ = await _semaphore.LockAsync(ct);
+        using var _ = _semaphore.Lock();
         if (Task == null!)
-            Task = await StartDownloadAsync(ct);
-
-        return Task;
+            Task = Start(ct).ContinueWith(continuation, CancellationToken.None);
     }
 
     /// <summary>
     ///     Starts downloading the file. The request will be performed in this task, but once started, the task will complete
     ///     and the download will continue in the background. The provided CancellationToken can be used to cancel the
     ///     download.
     /// </summary>
-    private async Task<Task> StartDownloadAsync(CancellationToken ct = default)
+    private async Task Start(CancellationToken ct = default)
     {
         Directory.CreateDirectory(_destinationDirectory);
 
@@ -398,8 +398,7 @@ private async Task<Task> StartDownloadAsync(CancellationToken ct = default)
                 throw new Exception("Existing file failed validation after 304 Not Modified", e);
             }
 
-            Task = Task.CompletedTask;
-            return Task;
+            return;
         }
 
         if (res.StatusCode != HttpStatusCode.OK)
@@ -432,11 +431,11 @@ private async Task<Task> StartDownloadAsync(CancellationToken ct = default)
             throw;
         }
 
-        Task = DownloadAsync(res, tempFile, ct);
-        return Task;
+        await Download(res, tempFile, ct);
+        return;
     }
 
-    private async Task DownloadAsync(HttpResponseMessage res, FileStream tempFile, CancellationToken ct)
+    private async Task Download(HttpResponseMessage res, FileStream tempFile, CancellationToken ct)
     {
         try
         {